Skip to content

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
Contributor

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
Contributor

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