add support for indexing in `@atomic` macro
Following the discussion in #54642
Implemented:
- [x]
modifyindex_atomic!,swapindex_atomic!,replaceindex_atomic!forGenericMemory - [x]
getindex_atomic,setindex_atomic!,setindexonce_atomic!forGenericMemory - [x] add support for references in
@atomicmacros - [x] add support for vararg indices in
@atomicmacros - [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))
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 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?
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 thanks for the clarification;
Alongside modifyindex_atomic!, swapindex_atomic! and replaceindex_atomic! I've also implemented
getindex_atomic(::GenericMemory, i, ...)setindex_atomic!(::GenericMemory, i, ...)andsetindexonce_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.
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).
the default orderings are the ones from the existing cases of @atomic macro.
@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.
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 @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!
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 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?
Atomix is actively used and @maleadt or I have commit privileges.
@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.
IMO this is ready to merge, but I do want @vtjnash to have a last lock
Thanks so much for doing this!
Awesome!