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

Use Acquire ordering for initialization check #610

Merged
merged 1 commit into from Jan 10, 2024
Merged

Use Acquire ordering for initialization check #610

merged 1 commit into from Jan 10, 2024

Conversation

AngelicosPhosphoros
Copy link
Contributor

@AngelicosPhosphoros AngelicosPhosphoros commented Jan 2, 2024

Reasoning is provided in a comment.

Reasoning is provided in the code comment.
@AngelicosPhosphoros
Copy link
Contributor Author

This generates different code than current for aarch64-apple-darwin, allowing it to do more instruction reordering for unrelated memory accesses.

@Thomasdezeeuw
Copy link
Collaborator

It seems the ordering was changed from Acquire to SeqCst in commit 28ce2f8, specifically: 28ce2f8#diff-b1a35a68f14e696205874893c07fd24fdb88882b47c23cc0e0c80a30c7d53759R552, from pr #12.

@sfackler you wrote the code, but the commit message doesn't mention why. Do you know of a good reason not to merge this?

@AngelicosPhosphoros
Copy link
Contributor Author

@Thomasdezeeuw It seems that @sfackler wouldn't respond.

Anyway, this old code was very different (e.g. it uses refcounting, and with too strong orderings too, Arc, for example, uses mostly acquire and relaxed orderings).

I wrote the comment in code that explains why Acquire ordering is correct and enough. With atomics, in general, it is preferable to use weakest as possible but still correct ordering because stronger ordering does make whole CPU slower, especially, if called frequently.

@sfackler
Copy link
Member

sfackler commented Jan 9, 2024

If I remember correctly, it was Alex’s general preference to use stronger orderings to avoid potential correctness issues. I wouldn’t read to much into it if people are confident that a weaker ordering is sufficient.

@Thomasdezeeuw
Copy link
Collaborator

Thanks @sfackler

Then I think we can merge this.

@Thomasdezeeuw Thomasdezeeuw merged commit 7cb6a01 into rust-lang:master Jan 10, 2024
14 checks passed
@Thomasdezeeuw
Copy link
Collaborator

Thanks @AngelicosPhosphoros

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

Successfully merging this pull request may close these issues.

None yet

3 participants