julia icon indicating copy to clipboard operation
julia copied to clipboard

add support for indexing in `@atomic` macro

Open kalmarek opened this issue 1 year ago • 12 comments

Following the discussion in #54642

Implemented:

  • [x] modifyindex_atomic!, swapindex_atomic!, replaceindex_atomic! for GenericMemory
  • [x] getindex_atomic, setindex_atomic!, setindexonce_atomic! for GenericMemory
  • [x] add support for references in @atomic macros
  • [x] add support for vararg indices in @atomic macros
  • [x] tests
  • [x] update docstrings with example usage
  • [ ] update Atomics section of the manual (?)
  • [x] news

@oscardssmith @vtjnash

New @atomic transformations implemented here:

julia> @macroexpand (@atomic a[i1,i2])
:(Base.getindex_atomic(a, :sequentially_consistent, i1, i2))

julia> @macroexpand (@atomic order a[i1,i2])
:(Base.getindex_atomic(a, order, i1, i2))

julia> @macroexpand (@atomic a[i1,i2] = 2.0)
:(Base.setindex_atomic!(a, :sequentially_consistent, 2.0, i1, i2))

julia> @macroexpand (@atomic order a[i1,i2] = 2.0)
:(Base.setindex_atomic!(a, order, 2.0, i1, i2))

julia> @macroexpand (@atomicswap a[i1,i2] = 2.0)
:(Base.swapindex_atomic!(a, :sequentially_consistent, 2.0, i1, i2))

julia> @macroexpand (@atomicswap order a[i1,i2] = 2.0)
:(Base.swapindex_atomic!(a, order, 2.0, i1, i2))

julia> @macroexpand (@atomic a[i1,i2] += 2.0)
:((Base.modifyindex_atomic!(a, :sequentially_consistent, +, 2.0, i1, i2))[2])

julia> @macroexpand (@atomic order a[i1,i2] += 2.0)
:((Base.modifyindex_atomic!(a, order, +, 2.0, i1, i2))[2])

julia> @macroexpand (@atomiconce a[i1,i2] = 2.0)
:(Base.setindexonce_atomic!(a, :sequentially_consistent, :sequentially_consistent, 2.0, i1, i2))

julia> @macroexpand (@atomiconce o1 o2 a[i1,i2] = 2.0)
:(Base.setindexonce_atomic!(a, o1, o2, 2.0, i1, i2))

julia> @macroexpand (@atomicreplace a[i1,i2] (2.0=>3.0))
:(Base.replaceindex_atomic!(a, :sequentially_consistent, :sequentially_consistent, 2.0, 3.0, i1, i2))

julia> @macroexpand (@atomicreplace o1 o2 a[i1,i2] (2.0=>3.0))
:(Base.replaceindex_atomic!(a, o1, o2, 2.0, 3.0, i1, i2))

kalmarek avatar Jun 06 '24 09:06 kalmarek

other than the comment above this looks good! (it is missing an implementation of getindex_atomic which we need because the regular getindex doesn't have a place to pass an order)

oscardssmith avatar Jun 06 '24 12:06 oscardssmith

@oscardssmith I'm not sure about the semantics of the suggested getindex_atomic. One can write a completely generic

function getindex_atomic(mem::GenericMemory, i::Int, order=default_access_order(mem))
    memref = memoryref(mem, i, @_boundscheck)
    return memoryrefget(memref, i, order, @_boundscheck)
end

but there's nothing atomic about the function, except the order argument. Is this what you meant?

kalmarek avatar Jun 07 '24 08:06 kalmarek

The point is just that (but probably restricted to AtomicMemory since

julia> m = Memory{Int}(undef, 5);

julia> Base.memoryrefget(memoryref(m, 1), :acquire, false)
ERROR: ConcurrencyViolationError("memoryrefget: non-atomic memory cannot be accessed atomically")
Stacktrace:
 [1] top-level scope
   @ REPL[3]:1

The reason we need this function is for the user to be able to provide a stricter order than :monotonic

oscardssmith avatar Jun 07 '24 12:06 oscardssmith

@oscardssmith thanks for the clarification;

Alongside modifyindex_atomic!, swapindex_atomic! and replaceindex_atomic! I've also implemented

  • getindex_atomic(::GenericMemory, i, ...)
  • setindex_atomic!(::GenericMemory, i, ...) and
  • setindexonce_atomic!(::GenericMemory, i, ...).

I added some converts (val isa T ? val : convert(T, val)::T) those *index methods and the support for indexing in @atomic macro with the semantics as in the opening comment.

Please have a look and comment!

I'll write/update docstrings once the code is approved.

kalmarek avatar Jun 07 '24 14:06 kalmarek

This looks great to me! We definitely need more tests (and review from @vtjnash to make sure that all of the default orders etc are correct).

oscardssmith avatar Jun 07 '24 14:06 oscardssmith

the default orderings are the ones from the existing cases of @atomic macro.

kalmarek avatar Jun 07 '24 18:06 kalmarek

@vtjnash could you have a look at the proposed changes? In particular the choices of orderings for the @atomic macro. Do we need even more tests?

@oscardssmith what kind of docs should be written?

To me the docstring of the atomic macro is already too long, asking rather for a paragraph in the atomics part of the manual.

kalmarek avatar Jun 18 '24 14:06 kalmarek

I think this does need some docs under the @atomic macro, e.g.

  @atomic m[i] = new
  @atomic m[i] += addend
  @atomic :release m[i] = new
  @atomic :acquire_release m[i] += addend

but I agree with you that the docstring is already pretty long, but I kind of think that it is necessary to at least list the transforms that the macro does.

oscardssmith avatar Jun 18 '24 14:06 oscardssmith

@oscardssmith @vtjnash I've updated docstrings and added NEWS entry. Please let me know if something else needs to be done/changed (i.e. please review :).

To me it seems ready to merge!

kalmarek avatar Jun 19 '24 19:06 kalmarek

Would be nice to check that Atomix.jl still works and can be used as a backwards compatible layer to get this functionality on older Julia versions.

vchuravy avatar Jun 27 '24 19:06 vchuravy

@vchuravy I was actually thinking about spinning this functionality into a small package for julia-1.11, but merging into Atomix.jl would be even better. My only hesitation is that this package has not seen any activity in 2 years and was mostly developed by the brilliant tkf who left the community. Is it worth the effort?

kalmarek avatar Jun 28 '24 14:06 kalmarek

Atomix is actively used and @maleadt or I have commit privileges.

vchuravy avatar Jun 28 '24 19:06 vchuravy

@oscardssmith @vtjnash please let me know what else need to be done here.

@nsajko, since you have access to lables I think that needs news, needs docs and needs tests are satisfied and could be removed.

kalmarek avatar Jul 02 '24 09:07 kalmarek

IMO this is ready to merge, but I do want @vtjnash to have a last lock

oscardssmith avatar Jul 02 '24 11:07 oscardssmith

Thanks so much for doing this!

oscardssmith avatar Jul 07 '24 21:07 oscardssmith

Awesome!

vtjnash avatar Jul 08 '24 07:07 vtjnash