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

Relax orderings in std::sync::once #44331

Closed
wants to merge 1 commit into from
Closed

Relax orderings in std::sync::once #44331

wants to merge 1 commit into from

Conversation

jeehoonkang
Copy link
Contributor

Currently, in std::sync::once, every access to the shared objects are SeqCst, while it doesn't need to be. I relaxed orderings to Acquire, Release, and AcqRel.

r? @alexcrichton

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton
Copy link
Member

Thanks for the PR! This has actually been proposed before and my own personal opinions at least haven't changed much.

Mind reading over some of the discussion there and posting your thoughts on it?

@arielb1 arielb1 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Sep 5, 2017
@jeehoonkang
Copy link
Contributor Author

@alexcrichton Thank you! I haven't notice that there was a closed PR on this issue. I will discuss further in that issue.

@aidanhs
Copy link
Member

aidanhs commented Sep 13, 2017

Hi @jeehoonkang @alexcrichton, just wondering what the status of this PR is after the (brief) exchange on #31650 - is this waiting on review, or are we looking for a bit more justifying background?

@alexcrichton
Copy link
Member

@jeehoonkang do you think SeqCst as-is today is wrong? (in the LLVM implementation rather than the standard) If not, do you think this has a technical benefit over the current construction?

@jeehoonkang
Copy link
Contributor Author

@aidanhs @alexcrichton The current implementation with SeqCst is correct. What I want to argue is that the reason it is correct is exactly the same as the reason a version with Release and Acquire is correct, because currently LLVM's SeqCst is broken and it doesn't give you a better reasoning principle than Release and Acquire.

That said, I kinda agree with the conclusion made in #31650 that we keep SeqCst. Without a formal proof, it would be just safer to keep the strongest ordering, which is SeqCst. If performance difference is not that great, I agree that we don't merge it now.

I agree to close this PR. Maybe later we can reopen, once someone clearly explains why Release and Acquire is safe in once.

@alexcrichton
Copy link
Member

Ok sounds good to me, thanks regardless @jeehoonkang !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants