RFC: Add Atomic{Ptr} support and tests

[dillondaudert]

This defines the following atomic operations on pointers via Atomic{Ptr}:

get_index(::Atomic{Ptr})
set_index!(::Atomic{Ptr}, ::Ptr)
atomic_cas!(::Atomic{Ptr}, ::Ptr, ::Ptr)
atomic_xchg!(::Atomic{Ptr}, ::Ptr)
atomic_max!(::Atomic{Ptr}, ::Ptr}
atomic_min!(::Atomic{Ptr}, ::Ptr}
atomic_add!(::Atomic{Ptr}, ::Integer)
atomic_sub!(::Atomic{Ptr}, ::Integer)

Fixes #29943, and possibly addresses #32455.

This still needs NEWS.

[NHDaly]

This is awesome. Thanks for doing this, Dillon! :) Looking forward to it!

This will probably also need NEWS as well, I would think, but you may want to wait for someone else to confirm that.

[dillondaudert]

I defined atomic_add!/sub!(::Atomic{Ptr}, ::Integer) separately since the signature is different enough (everything else follows the pattern (::Atomic{V}, V)).

[NHDaly]

:) Thanks for doing this Dillon, it looks good to me!
(Other than the annoying thing I just noticed about the "logical ops". i've left a comment.)

I think this would be ready for review from a maintainer.

[dillondaudert]

I think this is ready for review. The build error looks unrelated.

[StefanKarpinski]

@vtjnash, are you the right person to review this?

[dillondaudert]

I can remove the logical ops defined on Atomic{Ptr}, per @NHDaly 's suggestion, if reviewers think that's appropriate.

[dillondaudert]

I went ahead and excluded defining the logical operations on Atomic{Ptr}.

[NHDaly]

Thanks, looks great!

[dillondaudert]

Bump

[JeffBezanson]

This should probably be typ <: Integer || typ <: Ptr. Otherwise it includes the type Union{Integer, Ptr}.

[JeffBezanson]

Welcome, @dillondaudert , and thanks for working on this. This looks basically correct, but I'm not sure what the use cases are. #29943 asks for it for implementing lock-free data structures, but I doubt it's actually useful for that since Ptrs are not tracked by the GC.

[dillondaudert]

Could you elaborate your thoughts on why not being tracked by the GC makes atomic Ptr not useful for lock-free data structures? I admit I don't know all the subtleties and consequences around that.

I assumed since Ptr existed at all, it would be useful to have atomic operations for it. If that's not the case, should instead they be defined for Ref? I thought I read someone propose that in an issue at some point.

[ghost]

I could maybe imagine this being useful for something like Blobs.jl, making use of the atomic add/subtract. But yeah, otherwise simple pointer assignment is of course already atomic. Maybe we would use compare and swap in something like Blobs as well? I admit I haven't thought about it too much. Definitely agree with Jeff's assessment that this alone won't be terribly useful for lock-free datastructures. It would probably be most useful to just support arbitrary types, with separate support for stack vs heap allocated types?

…

On Wed, Dec 18, 2019, 11:18 AM Dillon Daudert ***@***.***> wrote: Could you elaborate your thoughts on why not being tracked by the GC makes atomic Ptr not useful for lock-free data structures? I admit I don't know all the subtleties and consequences around that. I assumed since Ptr existed at all, it would be useful to have atomic operations for it. If that's not the case, should instead they be defined for Ref? I thought I read someone propose that in an issue at some point. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#33869?email_source=notifications&email_token=AKSS5LGGPWD2LJDPOBLPRZ3QZG2RNA5CNFSM4JOHGT6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHE565A#issuecomment-566878068>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKSS5LEJKRBBHDCRMHMGK3TQZG2RNANCNFSM4JOHGT6A> .

[oxinabox]

But yeah, otherwise simple pointer assignment is of course already atomic.

No, it isn't.
I don't have a source handy but I found one for when @iamed2 made that claim. I can dig though LibPQ PRs and find it again tomorrow.
Pointer assignment is not atomic.
The CPU can act mid-word.

---

Yes, we likely also want this for Ref.
The TODO note in the source code only requested for Ptr.
Also seemed like that would be easier from working out what Todo with llvm.

[vtjnash]

But yeah, otherwise simple pointer assignment is of course already atomic.

It's unclear exactly what you intended to mean here. The simple assignment isn't "atomic" because that's not a property of the assignment but of the serializing side-effects that occur simultaneously (in the compiler and/or hardware). OTOH, it's pretty much unheard of for a processor do have a cache line smaller than a pointer word size, so the write of a pointer-sized word to an address with valid alignment is usually free of data races (torn writes). It's important to distinguish though that the Julia unsafe_load intrinsics though do not require alignment, and thus are NOT safe to use for threaded read/writes (the similar operation on a normal Julia object and array references should be fine).

I assumed since Ptr existed at all, it would be useful to have atomic operations for it

Yes, but maybe worth clarifying that's not what this PR does. It's been my goal for some time now to provide new Core.Intrinsics for doing atomic versions of each of the existing ops (e.g. fieldref, arrayref, pointerref, etc.) and then deprecate this file (it's inefficient and awkward). I'm happy to have others contribute towards the work to make it happen sooner, though I'd encourage anyone thinking of working in that direction to make a Julep first so we can review the design before they put a lot of effort into coding something.

[dillondaudert]

I'd be happy to help get proper atomic (pointer) operations implemented in Julia. At this point, I don't believe I'm familiar enough with Core.Intrinsics (or the internals in general) well enough to make a design proposal. Since the changes in this PR aren't the right way to go about this, I'll close it.

[oxinabox]

This remains an operation we should have though?