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

Fine-tune the ordering for received #1068

Open
wang384670111 opened this issue Jan 7, 2024 · 2 comments
Open

Fine-tune the ordering for received #1068

wang384670111 opened this issue Jan 7, 2024 · 2 comments

Comments

@wang384670111
Copy link

if !self.received.swap(true, Ordering::SeqCst) {

if !self.received.swap(true, Ordering::SeqCst) {

self.received.load(Ordering::SeqCst)

I believe that using a Release operation in swap and an Acquire operation in load can effectively synchronize with the writing of the message to memory. Using SeqCst is not necessary in this context.

(glad to make a pull request if this seems sensible)

@wang384670111
Copy link
Author

wang384670111 commented Jan 7, 2024

Similarly, in wake.rs, using SeqCst is unnecessary. Using Acquire/Release would be sufficient.

self.is_empty.store(
inner.selectors.is_empty() && inner.observers.is_empty(),
Ordering::SeqCst,
);

self.is_empty.store(
inner.selectors.is_empty() && inner.observers.is_empty(),
Ordering::SeqCst,
);

if !self.is_empty.load(Ordering::SeqCst) {

if !self.is_empty.load(Ordering::SeqCst) {

self.is_empty.store(
inner.selectors.is_empty() && inner.observers.is_empty(),
Ordering::SeqCst,
);

self.is_empty.store(
inner.selectors.is_empty() && inner.observers.is_empty(),
Ordering::SeqCst,
);

self.is_empty.store(
inner.selectors.is_empty() && inner.observers.is_empty(),
Ordering::SeqCst,
);

self.is_empty.store(
inner.selectors.is_empty() && inner.observers.is_empty(),
Ordering::SeqCst,
);

@ibraheemdev
Copy link
Contributor

The SeqCst throughout crossbeam-channel is necessary to establish a total order between send/recv and is_empty. Without it the optimistic check in https://github.com/crossbeam-rs/crossbeam/blob/master/crossbeam-channel/src/waker.rs#L222 would be incoherent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants