-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
sync::watch
: Use Acquire/Release memory ordering instead of SeqCst
#6018
sync::watch
: Use Acquire/Release memory ordering instead of SeqCst
#6018
Conversation
b361d8f
to
9f1723b
Compare
I am not sure why |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not particularly familiar with the watch
channel internals, but my assumption is that SeqCst
is used here to form a total order between the state
, the ref count, and the atomics used in Notify
and RwLock
. If the loom
tests pass after making this change, though, it's possible that such a total ordering is not actually necessary to ensure correctness.
However, I would recommend also running the sync_watch.rs
benchmarks before and after this change, preferably on an ARM machine (or other architecture with weak memory ordering --- on x86, all memory operations are, essentially, implicitly Acquire
/Release
). While SeqCst
operations have a performance cost, they can also have a performance benefit in some cases --- sometimes, a more expensive SeqCst
operation is actually cheaper if it succeeds on the first try and doesn't require some operation to be retried multiple times until the state of multiple atomics becomes consistent. So, it's usually worthwhile to do some before/after benchmarking when making these kind of changes.
Thanks for the PR! I hope my comment gives you some useful context for this change! :)
Thank you for these insights! I wasn't aware of these possible side effects. The version is always incremented within an exclusive lock scope. Only the Unfortunately, I don't have access to an ARM machine locally so I can't run the benchmarks myself. I am aware that on x86 the proposed change has no effect. I barely use |
5d67b2c
to
e5daae7
Compare
Yeah, that all makes sense. I'm certainly not at all opposed to this change --- I was just curious about whether the benchmarks had been run. If |
tokio/src/sync/watch.rs
Outdated
// Use `Release` ordering to ensure that storing the version | ||
// state is seen by the receiver side that uses `Acquire` for | ||
// loading the state. | ||
self.0.fetch_or(CLOSED_BIT, Ordering::Release); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, it would arguably make sense to use something stronger than relaxed for ref_count_rx
. Currently this assert could fail:
// thread 1
GLOBAL_BOOL.store(true, Relaxed);
drop(receiver);
// thread 2
if sender.is_closed() {
assert!(GLOBAL_BOOL.load(Relaxed));
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both the sender and all receivers could be dropped independently in any order by different threads. I can't imagine how to link the sender-side closed state (refcount) with the receiver-side closed state (version bit) when using separate atomics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either way, it's a separate question from what you're fixing here.
Co-authored-by: Alice Ryhl <[email protected]>
sync::watch
: Use Acquire/Release memory ordering instead of SeqCst
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just one nit.
…watch-acquire-release-state
Motivation
SeqCst
is overly restrictive.Solution
Use
Release
(Sender
: store / single writer) andAcquire
(Receiver
: load / multiple readers) memory ordering for a safe handover.