Improve the documentation of atomic::fence #147887
Conversation
Attempt to "fix" two flaws of the current documentation: 1. The over-emphasis of fence - fence synchronization, relegating atomic - fence and fence - atomic synchronization to second fiddle. 2. The lack of explanation as to how to properly perform atomic - fence and fence - atomic synchronization. It does so by first making it clear that there are 3 different ways to use an atomic fence, then presenting a full example for each usecase, noting the particular position of the fence with regard to the atomic operation, and rounding up with generic notes.
rustbot
added
S-waiting-on-review
|
rustbot has assigned @Mark-Simulacrum. Use |
| /// | | ||
| /// | | ||
| /// -------------> Y if m.load(Relaxed) == 3 { | ||
| /// B fence(Acquire); |
There was a problem hiding this comment.
In this case, and the fence - atomic case below, it's unclear whether the arrow should come from/point to the fence instead.
The happens-before relationship established is between X and B, but it may be worth emphasizing the X - Y link which is crucial for the fence to work.
This comment has been minimized.
This comment has been minimized.
library/core/src/sync/atomic.rs
Outdated
| /// An atomic operation 'X' with (at least) [`Release`] ordering semantics on some atomic object | ||
| /// 'm' on thread 1 is paired on thread 2 with an atomic read 'Y' with any order on 'm' followed by | ||
| /// a fence 'B' with (at least) [`Acquire`] ordering semantics. This provides a happens-before | ||
| /// dependence between X and B. |
There was a problem hiding this comment.
Maybe we can copy the cppreference on this?
https://en.cppreference.com/w/cpp/atomic/atomic_thread_fence.html
In particular, this language seems to miss "Y reads the value written by X (or the value would be written by release sequence headed by X if X were a release operation)" -- it's sort of implied by the example, but stating it explicitly feels better. It's also maybe obvious (otherwise it's not clear what would establish a relation between a particular release operation and particular acquire fence), but having a precise rule set seems better than prose? I find the bullets in the reference relatively easier to read than the language here.
There was a problem hiding this comment.
(I'm not sure what license that language is under, so copying may not be an option).
There was a problem hiding this comment.
Interesting. I tend to find the language on cppreference too "formal" for my taste.
I would say for me it's the difference between the "Guide-level" and "Reference-level" portions of Rust RFCs: the language here is guide-level, whereas the explanation on cppreference is reference-level, a formal specification of sort.
I do think having a "formal" specification is a good thing, but I find the esoteric language makes it hard to approach, and raises more questions than it answers -- what's a "release sequence headed by"? -- and thus I much prefer the Rust documentation (on fence, or Ordering) as it's so much more approachable, due to its use of familiar language.
With the ongoing work on the Rust specification, may I suggest leaving a more familiar language in the documentation, and later on linking to the specification for the more formal definition?
This way we can have our cake and eat it too.
library/core/src/sync/atomic.rs
Outdated
| /// An atomic operation 'X' with (at least) [`Release`] ordering semantics on some atomic object | ||
| /// 'm' on thread 1 is paired on thread 2 with an atomic read 'Y' with any order on 'm' followed by | ||
| /// a fence 'B' with (at least) [`Acquire`] ordering semantics. This provides a happens-before | ||
| /// dependence between X and B. |
There was a problem hiding this comment.
| /// An atomic operation 'X' with (at least) [`Release`] ordering semantics on some atomic object | |
| /// 'm' on thread 1 is paired on thread 2 with an atomic read 'Y' with any order on 'm' followed by | |
| /// a fence 'B' with (at least) [`Acquire`] ordering semantics. This provides a happens-before | |
| /// dependence between X and B. | |
| /// * An atomic operation 'X' with (at least) [`Release`] ordering semantics on some atomic object | |
| /// 'm' on thread 1 is paired with | |
| /// * an atomic read 'Y' on thread 2 with any order on 'm' | |
| //// * followed by a fence 'B' with (at least) [`Acquire`] ordering semantics. | |
| /// This provides a happens-before dependence between X and B. |
Can we reformat something like this for the statements? I think splitting it up help make the readability better.
There was a problem hiding this comment.
I love the idea of introducing structure to better highlight the various steps.
I wasn't quite sure how to introduce structure, so I've structured each of the 3 "chapters" in a slightly different way.
I personally prefer the 3rd way (fence-fence) which clearly isolates the operation on either thread, while I feel that the 1st way (atomic-fence) which puts the "on thread1" and "is paired on thread 2" in the steps is the less clear.
I'm looking forward to your opinion, and suggestions.
Mark-Simulacrum
added
S-waiting-on-author
|
@rustbot review |
rustbot
added
S-waiting-on-review
4 participants
Attempt to "fix" two flaws of the current documentation:
It does so by first making it clear that there are 3 different ways to use an atomic fence, then presenting a full example for each usecase, noting the particular position of the fence with regard to the atomic operation, and rounding up with generic notes.