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

Potential to fix memory ordering for value #1160

Open
wang384670111 opened this issue Apr 23, 2024 · 2 comments
Open

Potential to fix memory ordering for value #1160

wang384670111 opened this issue Apr 23, 2024 · 2 comments

Comments

@wang384670111
Copy link
Contributor

wang384670111 commented Apr 23, 2024

self.value.fetch_add(ONE_INACTIVE, Ordering::SeqCst);

let old_value = Counters::new(self.value.fetch_sub(ONE_INACTIVE, Ordering::SeqCst));

let old_value = Counters::new(self.value.fetch_sub(ONE_SLEEPING, Ordering::SeqCst));

SeqCst is overly restrictive. I think that the memory ordering can be appropriately modified.

In counters model, value is used for multithreading counting purposes, so using Relaxed ordering is sufficient.

(happy to make a PR if this looks reasonable)

@cuviper
Copy link
Member

cuviper commented Apr 23, 2024

Have you read the nearby README? There is some discussion of why we're using SeqCst:
https://github.com/rayon-rs/rayon/blob/8ccfda38d55e96c5bf50d004f2df95d07468dd4a/rayon-core/src/sleep/README.md

So we are relying on memory fencing effects beyond the counter values themselves. It might still be possible that some of this could be weakened, but we need to be very careful about that.

@wang384670111
Copy link
Contributor Author

wang384670111 commented Apr 24, 2024

Thanks for your reply! I miss some important information and I am a bit wrong, but I still believe that it is possible to weaken here, probably not Relaxed, I'll re-analyze the code again. :D

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

No branches or pull requests

2 participants