Add std.atomic support

[rymrg]

Add std.atomic struct as a better interface to core.atomic.

@atilaneves Please CR and let me know what's missing here.

[dlang-bot]

Thanks for your pull request and interest in making D better, @rymrg! We are looking forward to reviewing it, and you should be hearing from a maintainer soon. Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#10864"

[atilaneves]

This LGTM but I'd like another pair of eyes looking at this. Anyone you can think of?

[thewilsonator]

Enforce space after cast(...)
grep -nrE '[^"]cast\([^)]*?\)[[:alnum:]]' $(find etc std -name '*.d') ; test $? -eq 1
std/atomic.d:110:	rlx = cast(int)core.atomic.MemoryOrder.raw,
std/atomic.d:115:	acq = cast(int)core.atomic.MemoryOrder.acq,
std/atomic.d:120:	rel = cast(int)core.atomic.MemoryOrder.rel,
std/atomic.d:125:	acq_rel = cast(int)core.atomic.MemoryOrder.acq_rel,
std/atomic.d:130:	seq = cast(int)core.atomic.MemoryOrder.seq,

[thewilsonator]

Not quite sure what it wants with respect to documentation and unittests. I guess you need Params: and Returns: sections?

std/atomic.d(13:8)[warn]: Public declaration 'Atomic' has no documented example.
std/atomic.d(13:15)[warn]: Template parameters T isn't documented in the `Params` section.
std/atomic.d(13:18)[warn]: If constraints should have the same indentation as the function
std/atomic.d(35:21)[warn]: Template parameters mo isn't documented in the `Params` section.
std/atomic.d(41:25)[warn]: Template parameters mo isn't documented in the `Params` section.
std/atomic.d(41:49)[warn]: Parameter newVal isn't documented in the `Params` section.
std/atomic.d(50:2)[warn]: A public function needs to contain a `Returns` section.
std/atomic.d(50:21)[warn]: Template parameters mo isn't documented in the `Params` section.
std/atomic.d(50:45)[warn]: Parameter mod isn't documented in the `Params` section.
std/atomic.d(56:2)[warn]: A public function needs to contain a `Returns` section.
std/atomic.d(56:21)[warn]: Template parameters mo isn't documented in the `Params` section.
std/atomic.d(56:45)[warn]: Parameter mod isn't documented in the `Params` section.
std/atomic.d(62:2)[warn]: A public function needs to contain a `Returns` section.
std/atomic.d(62:25)[warn]: Template parameters mo isn't documented in the `Params` section.
std/atomic.d(62:49)[warn]: Parameter desired isn't documented in the `Params` section.
std/atomic.d(68:2)[warn]: A public function needs to contain a `Returns` section.
std/atomic.d(68:23)[warn]: Template parameters mo isn't documented in the `Params` section.
std/atomic.d(68:57)[warn]: Template parameters fmo isn't documented in the `Params` section.
std/atomic.d(68:82)[warn]: Parameter oldVal isn't documented in the `Params` section.
std/atomic.d(68:92)[warn]: Parameter newVal isn't documented in the `Params` section.
std/atomic.d(74:27)[warn]: Template parameters mo isn't documented in the `Params` section.
std/atomic.d(74:61)[warn]: Template parameters fmo isn't documented in the `Params` section.
std/atomic.d(74:86)[warn]: Parameter oldVal isn't documented in the `Params` section.
std/atomic.d(75:6)[warn]: Parameter newVal isn't documented in the `Params` section.
std/atomic.d(81:2)[warn]: A public function needs to contain a `Returns` section.
std/atomic.d(81:22)[warn]: Template parameters op isn't documented in the `Params` section.
std/atomic.d(81:28)[warn]: Parameter rhs isn't documented in the `Params` section.
std/atomic.d(87:2)[warn]: A public function needs to contain a `Returns` section.
std/atomic.d(87:19)[warn]: Template parameters op isn't documented in the `Params` section.
std/atomic.d(92:4)[warn]: Public declaration 'opUnary' is undocumented.
std/atomic.d(97:11)[warn]: Public declaration 'opUnary' is undocumented.
std/atomic.d(104:6)[warn]: Public declaration 'MemoryOrder' is undocumented.
std/atomic.d(222:1)[warn]: A unittest should be annotated with at least @safe or @system

[rymrg]

Thanks. I hope I didn't miss anything.

[thewilsonator]

Two more

std/atomic.d(24:8)[warn]: Public declaration 'Atomic' has no documented example.
std/atomic.d(189:6)[warn]: Public declaration 'MemoryOrder' has no documented example.

[TurkeyMan]

I'm usually sceptical of tools like this. I can't see anything obviously wrong with the implementation, but in my experience, this sort of thing tends to encourage usage by non-experts in ways that invite broken code.

For my money, I don't see anything wrong with a call to cas in the proper situation. Tools like this hide the detail, and if you're writing atomic code, the detail is EVERYTHING. For one thing; use of operators inhibits the MemoryOrder parameters, which usually means the operation defaults to sequentually consistent, which is almost never what you actually want; it's a lazy default which is excessively conservative. Tools like this also tend to appear around the place and/or as members of other things. As soon as there's 2 such things in existence, there's a good chance you're writing bugs in ways the author probably doesn't understand.

Anyway, I can't see anything obviously wrong here (aside from my comments around shared and the questionable tests), and there are other things like this in existence; I just probably wouldn't.

The use of shared is still a complete disaster zone in D, and we should work on that. The usage of shared shown here is meaningless at best, or broken more likely.

[rymrg]

@

Two more

std/atomic.d(24:8)[warn]: Public declaration 'Atomic' has no documented example.
std/atomic.d(189:6)[warn]: Public declaration 'MemoryOrder' has no documented example.

I tried to get the atomic example to fit. If it doesn't work I'm not sure what the syntax should be. For MemoryOrder, it's an enum. How do you propose writing an example for enum?

[rymrg]

Thank you for your review! I was hoping to get one.

I'm usually sceptical of tools like this. I can't see anything obviously wrong with the implementation, but in my experience, this sort of thing tends to encourage usage by non-experts in ways that invite broken code.

From my point of view, the current core.atomic is error prone for experts and pure error for non-experts thinking they can make use of it. For example, with core.atomic is it really easy to mix non-atomic access with atomic access to a memory location rendering the atomic access meaningless as it's going to end up with the catch-fire semantics (undefined behavior).

For my money, I don't see anything wrong with a call to cas in the proper situation. Tools like this hide the detail, and if you're writing atomic code, the detail is EVERYTHING. For one thing; use of operators inhibits the MemoryOrder parameters, which usually means the operation defaults to sequentually consistent, which is almost never what you actually want; it's a lazy default which is excessively conservative. Tools like this also tend to appear around the place and/or as members of other things. As soon as there's 2 such things in existence, there's a good chance you're writing bugs in ways the author probably doesn't understand.

I don't see a problem calling cas either. In fact it is a requirement for lock-free algorithms, so I'm not sure what you claim here. The atomic struct has two purposes. The first being a useful interface for experts preventing non atomic access to a location with a nice syntax (all the explicit forms) -- the case where you do want all these details. The other case is to offer an easy way to not use weak memory access and instead use sequentially consistent semantics which are easier to reason about.

By defaulting to sequentially consistent, we nudge non-experts to a memory model they can reason about (even if it might be tricky for them). It reduces the amount of undefined behavior as well as unexpected behaviors they'll have in their code. Additionally, since combining two different libraries making use of atomics can introduce new bugs as it can introduce different synchronization paths than when the library executes alone, it is safer to default to SC as combining two concurrent libraries won't have problems of unexpected synchronization.

Regretfully I don't have the citations in an easily search-able place so I won't write any numbers, but there are cases when SC access ends up being faster than non SC one. Others question if the programmer overhead required for WMM is worth the benefit of code being faster.

Anyway, I can't see anything obviously wrong here (aside from my comments around shared and the questionable tests), and there are other things like this in existence; I just probably wouldn't.

The use of shared is still a complete disaster zone in D, and we should work on that. The usage of shared shown here is meaningless at best, or broken more likely.

I wasn't sure about shared semantics in D enough and hoped this code review will enlighten me. I was under the impression that an atomic will be shared but still allow access. I can remove all the shared from the code until we figure this one out.

Thanks again!

[TurkeyMan]

For example, with core.atomic is it really easy to mix non-atomic access with atomic access to a memory location rendering the atomic access meaningless as it's going to end up with the catch-fire semantics (undefined behavior).

Can you show an example of this? If you have shared int or shred(int)*, it should be impossible to read/write to that thing without some special mechanism (ie, using core.atomic). Granted shared is broken, but we fixed it 6-7 years ago (pending enabling by default), and we shouldn't lean in on the assumption that shared will be broken forever.

I don't see a problem calling cas either. In fact it is a requirement for lock-free algorithms, so I'm not sure what you claim here.

I mean just call core.atomic.cas, no tool is needed to wrap up that (often) one isolated call.

The atomic struct has two purposes. The first being a useful interface for experts preventing non atomic access to a location with a nice syntax (all the explicit forms) -- the case where you do want all these details

shared should prevent non-atomic access to shared things; so this is a moot point.

The other case is to offer an easy way to not use weak memory access and instead use sequentially consistent semantics which are easier to reason about.

Yeah, no; you lost me here instantly. I've fixed a lot of broken lock-free code; and it's the most excruciating thing you can ever be tasked to do. If the author is casually deploying sequentially consistent marks to atomic operations "because it's easier to reason about"; the implication is, quite strictly, that they don't know or aren't confident what they're doing. That person SHOULD NOT BE WRITING A SINGLE LINE OF ATOMIC CODE UNDER ANY CIRCUMSTANCES. Only disaster will follow.

By defaulting to sequentially consistent, we nudge non-experts to a memory model they can reason about (even if it might be tricky for them). It reduces the amount of undefined behavior as well as unexpected behaviors they'll have in their code.

I don't agree at all; it probably only makes it harder to find their bugs because they become slightly less probable. Again; you don't encourage an amateur author to write lock-free code with a hope and a prayer that they can do this right... they almost certainly can't.

Additionally, since combining two different libraries making use of atomics can introduce new bugs as it can introduce different synchronization paths than when the library executes alone, it is safer to default to SC as combining two concurrent libraries won't have problems of unexpected synchronization.

I don't follow. I'm not actually sure what you're trying to say here. I'm not aware of lock-free tools that are properly wrapped and isolated interacting with eachother in any dangerous ways. Are there examples of this you can show? It's news to me.

Regretfully I don't have the citations in an easily search-able place so I won't write any numbers, but there are cases when SC access ends up being faster than non SC one. Others question if the programmer overhead required for WMM is worth the benefit of code being faster.

Yeah sorry, I don't know here. I never heard this, and I'm sceptical of this claim. If it's true, then I don't understand the mechanism where this could be true, and it adds weight to my assertion that casual users shouldn't be doing this... (which apparently includes me)

I wasn't sure about shared semantics in D enough and hoped this code review will enlighten me. I was under the impression that an atomic will be shared but still allow access. I can remove all the shared from the code until we figure this one out.

You should compile with -preview=nosharedaccess, then the language will enforce the safety guarantees you feel like you're missing. It is not valid to read or write to shared memory in an unprotected way under any circumstances. If you write:

shared int x;
x = 10; // ERROR: can't write to shared memory (this is what you're worried about right?)
int y = x; // ERROR: can't read from shared memory

x.atomicStore!(MemoryOrder.relaxed)(10); // valid, atomicStore accepts a shared ref

[TurkeyMan]

I'd like someone else to make a call on whether they agree or disagree that a tool like this should exist. @rymrg If you feel strongly that it should exist, then I'd like to see you correct the use of shared to properly represent the future of the language; enable -preview=nosharedaccess. Fix so that the internal val is shared, and all the user facing methods and stack values in tests are not.

I also want tests to be valid and sensible/useful things to present to readers in their own right. It's easy to mislead or mis-educate users with bad sample code.

[rikkimax]

Overall I'm not happy with either solution. Yes atomics are best represented with a storage class & type qualifier and warrants one, but shared isn't purely atomics, it is unspecified what synchronizes it and that is a pretty big problem given that we have synchronized statements.

If you can't have a dedicated storage class & type qualifier, the next best thing is a struct such as this.

As for nosharedaccess if the ecosystem is ready for it to be turned on, please let me know and I'll present it at a monthly meeting to do so.

[TurkeyMan]

synchronised should be deleted from the language asap.

I don't know where nosharedaccess rests in the broader ecosystem; I've had it enabled in all my code for 6-7 years or whatever. Reality is, any code that relies on traditional shared semantics is basically a bug.

If this is introduced; maybe it should be added to core.atomic instead of adding a distinct thing?

[rikkimax]

synchronized statements are kinda necessary for thread safety wrt. blocking for web development. But overall, there is a giant ball of problems wrt. storage class version of it.

Agreed, moving it to core.atomic would be a better place for it.

[rymrg]

synchronized statements are kinda necessary for thread safety wrt. blocking for web development. But overall, there is a giant ball of problems wrt. storage class version of it.

Agreed, moving it to core.atomic would be a better place for it.

@atilaneves, I cannot remember, was it your idea or someone else that this goes into std?

[TurkeyMan]

synchronized statements are kinda necessary for thread safety wrt. blocking for web development.

Write a synchronisation tool and use it. No need for a language keyword, and it's almost always inferior to something application specific.

[rikkimax]

synchronized statements are kinda necessary for thread safety wrt. blocking for web development.

Write a synchronisation tool and use it. No need for a language keyword, and it's almost always inferior to something application specific.

I'm not talking about the mutex, we need to be able to model the act of synchronization the lock+unlock. Can't have a lock being held with a yield that takes place. So there has to be language support and for us right now that is synchronized statements.

[TurkeyMan]

I'm not talking about the mutex, we need to be able to model the act of synchronization the lock+unlock. Can't have a lock being held with a yield that takes place. So there has to be language support and for us right now that is synchronized statements.

Okay, there's context here that I'm missing. I don't really care about that right now though, so please don't enlighten me :P

[rymrg]

Yeah, no; you lost me here instantly. I've fixed a lot of broken lock-free code; and it's the most excruciating thing you can ever be tasked to do. If the author is casually deploying sequentially consistent marks to atomic operations "because it's easier to reason about"; the implication is, quite strictly, that they don't know or aren't confident what they're doing. That person SHOULD NOT BE WRITING A SINGLE LINE OF ATOMIC CODE UNDER ANY CIRCUMSTANCES. Only disaster will follow.

I know the feeling but I still think sometimes SC is the correct approach (just don't ever mix SC and non SC access). Similarly, when teaching you start with SC then maybe in more advanced courses go for weak memory.

I don't follow. I'm not actually sure what you're trying to say here. I'm not aware of lock-free tools that are properly wrapped and isolated interacting with eachother in any dangerous ways. Are there examples of this you can show? It's news to me.

Please ignore it, I was conflating different correctness criterion.

Yeah sorry, I don't know here. I never heard this, and I'm sceptical of this claim. If it's true, then I don't understand the mechanism where this could be true, and it adds weight to my assertion that casual users shouldn't be doing this... (which apparently includes me)

I don't understand either. I did reproduce it for some code but did not have the time to fully investigate it.

[atilaneves]

synchronized statements are kinda necessary for thread safety wrt. blocking for web development. But overall, there is a giant ball of problems wrt. storage class version of it. Agreed, moving it to core.atomic would be a better place for it.

@atilaneves, I cannot remember, was it your idea or someone else that this goes into std?

I think it was me, yes.