Rewrite docs for fetch_update for clarity #136036

hkBst · 2025-01-25T02:32:33Z

~~In particular also uses the same names for the orderings as compare_exchange does to reduce confusion as per #89116.~~

rustbot · 2025-01-25T02:32:40Z

r? @thomcc

rustbot has assigned @thomcc.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

hkBst · 2025-01-26T11:33:01Z

Once the proposed text replacement is approved, I will copy it to the other two places that define fetch_update.

bors · 2025-01-28T18:15:26Z

☔ The latest upstream changes (presumably #136185) made this pull request unmergeable. Please resolve the merge conflicts.

Sky9x · 2025-01-29T20:06:11Z

library/core/src/sync/atomic.rs

                                   mut f: F) -> Result<$int_type, $int_type>
            where F: FnMut($int_type) -> Option<$int_type> {
-                let mut prev = self.load(fetch_order);
+                let mut prev = self.load(failure);


perhaps a comment explaining why we are loading with the 'failure' order could be added here?

I'm not sold on this particular naming, but this change tries to bring this into line with compare_exchange's use of the terms. It explains the terms like this: "success describes the required ordering for the read-modify-write operation that takes place if the comparison with current succeeds. failure describes the required ordering for the load operation that takes place when the comparison fails.". Perhaps update_order and load_order are better terms?

The existing set_order and fetch_order are probably fine. It would indeed seem weird to immediately load a value using failure.

Alright, I'll go the other way then, and eliminate the success and failure orders from compare_exchange*. But, euhm, perhaps in a separate PR...

Sky9x · 2025-01-29T20:08:06Z

library/core/src/sync/atomic.rs

+            /// Note: susceptible to the [ABA Problem](https://en.wikipedia.org/wiki/ABA_problem).
            ///
            /// **Note**: This method is only available on platforms that support atomic operations on
            #[doc = concat!("[`", $s_int_type, "`].")]
            ///
-            /// # Considerations
-            ///
-            /// This method is not magic; it is not provided by the hardware.
-            /// It is implemented in terms of
-            #[doc = concat!("[`", stringify!($atomic_type), "::compare_exchange_weak`],")]
-            /// and suffers from the same drawbacks.
-            /// In particular, this method will not circumvent the [ABA Problem].
-            ///
-            /// [ABA Problem]: https://en.wikipedia.org/wiki/ABA_problem
-            ///


Why was this changed? IMO the longer version is better.

The implementation using compare_exchange_weak is now explicitly described. This makes it unnecessary to say it is not magic, because that is now obvious. Also, users are more likely to look at the docs for compare_exchange_weak. That leaves the "in particular ... ABA problem", which I've shortened to its core, which is that this method is "susceptible to the ABA Problem".

The explicit description of how the implementation uses compare_exchange_weak is also why I removed the copy of the description of the constraints on the memory orderings which comes directly from using compare_exchange_weak. Thanks for reviewing!

Agreed with @Sky9x, this section should remain. It's one thing to explain that this uses compare_exchange_weak, another thing to confirm that it doesn't somehow do some tricks to avoid compare_exchange_weak's limitations.

Plus I have definitely seen this specific section get linked around.

Very well. This section also got a significant improvement in #142252, so my reasons for removing it don't seem to apply anymore.

bors · 2025-06-13T08:34:09Z

☔ The latest upstream changes (presumably #142432) made this pull request unmergeable. Please resolve the merge conflicts.

library/core/src/sync/atomic.rs

tgross35 · 2025-06-16T03:32:12Z

library/core/src/sync/atomic.rs

+            /// If the closure returns Some(new value), then this method calls
+            #[doc = concat!("[`", stringify!($atomic_type), "::compare_exchange_weak`]")]
+            /// to try to store the new value.
+            /// If storing a new value fails, because another thread changed the current value,


Suggested change

/// If storing a new value fails, because another thread changed the current value,

/// If storing a new value fails (due to another thread changing the current value)

Suggested reword to avoid using comma+"because" for an interjection

The parenthetical is indeed probably more appropriate here. I'm not sure I understand the difference in nuance between "because"/"due to", but I'll copy that too. If you have any articulable insight there, I'd love to hear it.

If you have any articulable insight there, I'd love to hear it.

Not at all 😆 I probably just rewrote it a few times before finalizing the suggestion, feel free to pick either.

So disappointing ;P

tgross35 · 2025-06-16T03:49:33Z

library/core/src/sync/atomic.rs

+            /// then the given closure will be called again on the new current value
+            /// (that was just returned by compare_exchange_weak),
+            /// until either the closure returns None,
+            /// or compare_exchange_weak succeeds in storing a new value.
+            /// Returns `Ok(previous value)` if it ever succeeds in storing a new value.
+            /// Takes a success and a failure [`Ordering`] to pass on to compare_exchange_weak,
+            /// and also uses the failure ordering for the initial load.


Suggested change

/// then the given closure will be called again on the new current value

/// (that was just returned by compare_exchange_weak),

/// until either the closure returns None,

/// or compare_exchange_weak succeeds in storing a new value.

/// Returns `Ok(previous value)` if it ever succeeds in storing a new value.

/// Takes a success and a failure [`Ordering`] to pass on to compare_exchange_weak,

/// and also uses the failure ordering for the initial load.

/// then the given closure will be called again on the new current value,

/// which was just returned by `compare_exchange_weak`.

/// This will repeat until either the closure returns None,

/// or `compare_exchange_weak` succeeds in storing a new value. The return value is

/// `Ok(previous value)` if a new value was ever successfully stored, otherwise

/// `Err(current_value)`

///

/// Takes a success and a failure [`Ordering`] to pass on to `compare_exchange_weak`,

/// and also uses the failure ordering for the initial load.

Few suggestions:

Split the long sentence and make the CEW call part of the main flow.

Mention the Err condition right next to Ok.

Split the ordering discussion to a new paragraph.

compare_exchange_weak needs backticks

Also please make sure to rewrap doc comments to 100 chars before merge, or as far as is possible with the scattered #[doc] attributes.

* Split the long sentence and make the CEW call part of the main flow.

Done.

* Mention the `Err` condition right next to `Ok`.

Done. I think I was afraid of the repetition, but it's probably better this way.

* Split the ordering discussion to a new paragraph.

Done.

* `compare_exchange_weak` needs backticks

Definitely.

Also please make sure to rewrap doc comments to 100 chars before merge, or as far as is possible with the scattered #[doc] attributes.

Once this is nearing completion, I'll do that, if you really insist.

On rereading the new text, the ambiguity of "This will repeat" seems unescapable; even knowing the form of the answer, I cannot answer the question "what will repeat" with a quote from the text, and so "This" seems to be a dangling pointer. Probably because the previous sentence does not contain the act of storing anymore. This is probably why I wrote such a long sentence originally.

Going back to longer sentence, but with mention of CEW also parenthesized, so the main structure is clearer.

tgross35 · 2025-06-16T04:00:50Z

library/core/src/sync/atomic.rs

+            /// Note: susceptible to the [ABA Problem](https://en.wikipedia.org/wiki/ABA_problem).
            ///
            /// **Note**: This method is only available on platforms that support atomic operations on
            #[doc = concat!("[`", $s_int_type, "`].")]
            ///
-            /// # Considerations
-            ///
-            /// This method is not magic; it is not provided by the hardware.
-            /// It is implemented in terms of
-            #[doc = concat!("[`", stringify!($atomic_type), "::compare_exchange_weak`],")]
-            /// and suffers from the same drawbacks.
-            /// In particular, this method will not circumvent the [ABA Problem].
-            ///
-            /// [ABA Problem]: https://en.wikipedia.org/wiki/ABA_problem
-            ///


Agreed with @Sky9x, this section should remain. It's one thing to explain that this uses compare_exchange_weak, another thing to confirm that it doesn't somehow do some tricks to avoid compare_exchange_weak's limitations.

Plus I have definitely seen this specific section get linked around.

tgross35 · 2025-06-16T04:04:56Z

library/core/src/sync/atomic.rs

                while let Some(next) = f(prev) {
-                    match self.compare_exchange_weak(prev, next, set_order, fetch_order) {
+                    match self.compare_exchange_weak(prev, next, success, failure) {
                        x @ Ok(_) => return x,
                        Err(next_prev) => prev = next_prev
                    }


Please make sure to update update and try_update as well once this is closer to ready #135894

Definitely!

tgross35 · 2025-06-16T04:11:54Z

library/core/src/sync/atomic.rs

                                   mut f: F) -> Result<$int_type, $int_type>
            where F: FnMut($int_type) -> Option<$int_type> {
-                let mut prev = self.load(fetch_order);
+                let mut prev = self.load(failure);


The existing set_order and fetch_order are probably fine. It would indeed seem weird to immediately load a value using failure.

hkBst · 2025-06-16T04:33:55Z

@tgross35, thanks for reviewing! I'll look at each of your points soon.

hkBst · 2025-06-17T08:51:34Z

@fu5ha Since you recently worked on these docs (in #142252), I would be very happy to hear your views.

fu5ha

Nice. I think this is definitely clearer than the current docs. I have a few suggestions. Also note that once we're happy with the wording, this needs to be duplicated to the AtomicPtr docs separately as well, since they're different than all the atomic ints implemented thru the macro.

fu5ha · 2025-06-17T11:18:42Z

library/core/src/sync/atomic.rs

-            /// Note: This may call the function multiple times if the value has been changed from other threads in
-            /// the meantime, as long as the function returns `Some(_)`, but the function will have been applied
-            /// only once to the stored value.
+            /// If the closure ever returns None, this method will immediately return `Err(current value)`.


I think the order of these two sentences should be swapped, putting the success case first.

I originally ordered these sentences this way, because the None case is so much simpler than the Some case, which requires clarification for the fail-to-store scenario.

fu5ha · 2025-06-17T11:19:57Z

library/core/src/sync/atomic.rs

-            /// the meantime, as long as the function returns `Some(_)`, but the function will have been applied
-            /// only once to the stored value.
+            /// If the closure ever returns None, this method will immediately return `Err(current value)`.
+            /// If the closure returns Some(new value), then this method calls


Suggested change

/// If the closure returns Some(new value), then this method calls

/// If the closure returns `Some(updated_value)`, then this method calls

There are 6 uses of "new value" (including this one), which is used consistently for the value that comes from the closure and is attempted to be stored. On first glance it does not seem clear which of those should be changed. But if I assume all should be changed, for consistency, some of these substitutions seem awkward.

fu5ha · 2025-06-17T11:20:31Z

library/core/src/sync/atomic.rs

-            /// Note: This may call the function multiple times if the value has been changed from other threads in
-            /// the meantime, as long as the function returns `Some(_)`, but the function will have been applied
-            /// only once to the stored value.
+            /// If the closure ever returns None, this method will immediately return `Err(current value)`.


Suggested change

/// If the closure ever returns None, this method will immediately return `Err(current value)`.

/// If the closure ever returns `None`, this method will immediately return `Err(loaded_value)`.

Right, the issue you're identifying is that you can never really be sure whether the value you have is current or not, only that you loaded it.

Current usage is "Loads the current value ..." and "... return Err(current value)" plus 3 more. Your suggestion taken literally is "Loads the current value ..." and "... return Err(loaded_value)" in which loaded_value seems to not have an antecedent any more. And if I take it more consistently, it only seems to become worse: "Loads the loaded_value ..." and "... return Err(loaded_value)".

Your critique is valid, but I don't think this solution works.

fu5ha · 2025-06-17T11:26:10Z

library/core/src/sync/atomic.rs

+            /// If storing a new value fails (due to another thread changing the current value),
+            /// then the given closure will be called again on the new current value
+            /// (returned by `compare_exchange_weak`), until either the closure returns None,
+            /// or `compare_exchange_weak` succeeds in storing a new value.


I think we should try to talk about the semantics a bit more rather than directly to the implementation, something like

If the value of the atomic observed when attempting to store the updated_value is not equal to the value from the initial load, then the store will not happen. Instead, the closure will be invoked again, this time with the new value observed during the store attempt. This pattern will continue in a loop until either the closure returns None or the store succeeds.

Hmm, that seems very attractive. I will spend some time exploring this.

After trying, unfortunately I'm not sure how to use this.

fu5ha · 2025-06-17T11:32:24Z

library/core/src/sync/atomic.rs

+            /// If the closure returns Some(new value), then this method calls
+            #[doc = concat!("[`", stringify!($atomic_type), "::compare_exchange_weak`]")]
+            /// to try to store the new value.


Perhaps explain the semantics first, I do think a callout to the implementation is appropriate in parens here though.

If the closure returns Some(updated_value), the implementation will attempt to store the updated_value to the atomic. This store will only occur if the value of the atomic is still equal to the value of the initial load (see compare_exchange_weak).

I do like this, but I'm not sure how to explain the two ordering arguments that are essentially for passing on to compare_exchange_weak, when compare_exchange_weak's involvement is not crystal clear.

I will think about it some more.

hkBst · 2025-06-17T18:20:08Z

@fu5ha Thanks for reviewing. I apologize for not being good enough to make much use of your suggestions. I hope you won't hold it against me.

hkBst · 2025-06-17T18:21:40Z

@tgross35 Are there any egregious line wrapping problems that I should fix, before copying this?

rustbot assigned thomcc Jan 25, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 25, 2025

This comment has been minimized.

Sign in to view

hkBst force-pushed the patch-26 branch from 772fc85 to bbe765e Compare January 26, 2025 11:27

hkBst force-pushed the patch-26 branch from bbe765e to 1a0d867 Compare January 28, 2025 18:25

hkBst marked this pull request as ready for review January 29, 2025 05:39

Sky9x reviewed Jan 29, 2025

View reviewed changes

hkBst requested a review from Sky9x February 5, 2025 13:27

tgross35 requested changes Jun 16, 2025

View reviewed changes

rust-cloud-vms bot force-pushed the patch-26 branch from 1a0d867 to e0a5c9a Compare June 17, 2025 08:46

This comment has been minimized.

Sign in to view

rust-cloud-vms bot force-pushed the patch-26 branch from e0a5c9a to 00468e3 Compare June 17, 2025 08:46

fu5ha suggested changes Jun 17, 2025

View reviewed changes

Rewrite docs for fetch_update for clarity

9b60854

rust-cloud-vms bot force-pushed the patch-26 branch from 00468e3 to 9b60854 Compare June 17, 2025 18:12

	/// If storing a new value fails, because another thread changed the current value,
	/// If storing a new value fails (due to another thread changing the current value)

	/// If the closure returns Some(new value), then this method calls
	/// If the closure returns `Some(updated_value)`, then this method calls

	/// If the closure ever returns None, this method will immediately return `Err(current value)`.
	/// If the closure ever returns `None`, this method will immediately return `Err(loaded_value)`.

Rewrite docs for fetch_update for clarity #136036

Are you sure you want to change the base?

Rewrite docs for fetch_update for clarity #136036

Conversation

hkBst commented Jan 25, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Jan 25, 2025

Uh oh!

This comment has been minimized.

hkBst commented Jan 26, 2025

Uh oh!

bors commented Jan 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hkBst Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hkBst Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors commented Jun 13, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hkBst commented Jun 16, 2025

Uh oh!

This comment has been minimized.

hkBst commented Jun 17, 2025

Uh oh!

fu5ha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hkBst commented Jan 25, 2025 •

edited by rustbot

Loading

hkBst Jun 17, 2025 •

edited

Loading

hkBst Jan 30, 2025 •

edited

Loading