Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite docs for fetch_update for clarity #136036

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 19 additions & 31 deletions library/core/src/sync/atomic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3102,37 +3102,25 @@ macro_rules! atomic_int {
unsafe { atomic_xor(self.v.get(), val, order) }
}

/// Fetches the value, and applies a function to it that returns an optional
/// new value. Returns a `Result` of `Ok(previous_value)` if the function returned `Some(_)`, else
/// `Err(previous_value)`.
///
/// 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.
///
/// `fetch_update` takes two [`Ordering`] arguments to describe the memory ordering of this operation.
/// The first describes the required ordering for when the operation finally succeeds while the second
/// describes the required ordering for loads. These correspond to the success and failure orderings of
#[doc = concat!("[`", stringify!($atomic_type), "::compare_exchange`]")]
/// respectively.
///
/// Using [`Acquire`] as success ordering makes the store part
/// of this operation [`Relaxed`], and using [`Release`] makes the final successful load
/// [`Relaxed`]. The (failed) load ordering can only be [`SeqCst`], [`Acquire`] or [`Relaxed`].
/// Loads the current value, applies a closure to it, and optionally tries to store a new 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
#[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,
/// 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.
///
/// 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
///
Comment on lines +3119 to -3135
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@hkBst hkBst Jan 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

/// # Examples
///
/// ```rust
Expand All @@ -3149,13 +3137,13 @@ macro_rules! atomic_int {
#[$cfg_cas]
#[cfg_attr(miri, track_caller)] // even without panics, this helps for Miri backtraces
pub fn fetch_update<F>(&self,
set_order: Ordering,
fetch_order: Ordering,
success: Ordering,
failure: Ordering,
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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
}
Expand Down
Loading