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

All atomic exchanges with _Api_level should be acq_rel #4288

Merged
merged 1 commit into from
Jan 11, 2024

Conversation

AZero13
Copy link
Contributor

@AZero13 AZero13 commented Dec 24, 2023

As _Api_level is not always accessed with sequential ordering, and _Level is thread-local, acq_rel is good enough for all compare_exchange successes. However, this means we need to explicitly mention acquire for the failure case.

@AZero13 AZero13 requested a review from a team as a code owner December 24, 2023 19:02
@AZero13 AZero13 force-pushed the release branch 3 times, most recently from 0a52f4f to 5bde50e Compare December 25, 2023 16:40
stl/src/tzdb.cpp Outdated Show resolved Hide resolved
@CaseyCarter CaseyCarter added the enhancement Something can be improved label Jan 3, 2024
@AZero13 AZero13 changed the title _Init_icu_functions should use memory_order_release and memory_order_consume rather than memory_order_acq_rel All atomic exchanges with _Api_level should be acq_rel Jan 4, 2024
@AZero13 AZero13 requested a review from CaseyCarter January 4, 2024 18:39
@AZero13
Copy link
Contributor Author

AZero13 commented Jan 4, 2024

@CaseyCarter Fixed!

@AZero13 AZero13 changed the title All atomic exchanges with _Api_level should be acq_rel All atomic exchanges with _Api_level should be acquire or release Jan 4, 2024
@AZero13 AZero13 changed the title All atomic exchanges with _Api_level should be acquire or release All atomic exchanges with _Api_level should be acq_rel Jan 4, 2024
@AZero13 AZero13 force-pushed the release branch 2 times, most recently from 3888812 to ce0f6e1 Compare January 5, 2024 01:26
As _Api_level is not always accessed with sequential ordering, and _Level is thread-local, acq_rel is good enough for all compare_exchange successes. However, this means we need to explicitly mention acquire for the failure case.
@frederick-vs-ja
Copy link
Contributor

Hmm... I wonder what happened to your environment. Is there anything enforcing to force-push a single commit?

@StephanTLavavej
Copy link
Member

Thanks, this is a good change, and is consistent with the very similar code in tzdb.cpp:

STL/stl/src/tzdb.cpp

Lines 57 to 58 in 87580ca

while (!_Icu_functions._Api_level.compare_exchange_weak(
_Level, _Icu_api_level::_Detecting, _STD memory_order_acq_rel)) {

I'd still like to understand why you keep force-pushing and ignoring our requests to avoid doing so, but that shouldn't block merging this change. At least GitHub started providing a "Compare" button, although it's still harder to see what's changed over time than with the ordinary commits that everyone else uses.

@StephanTLavavej StephanTLavavej self-assigned this Jan 11, 2024
@StephanTLavavej
Copy link
Member

I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed.

@StephanTLavavej StephanTLavavej removed the enhancement Something can be improved label Jan 11, 2024
@StephanTLavavej StephanTLavavej added the performance Must go faster label Jan 11, 2024
@StephanTLavavej StephanTLavavej merged commit 1fbc77c into microsoft:main Jan 11, 2024
35 checks passed
@StephanTLavavej
Copy link
Member

Thanks for improving the consistency of these atomic operations by avoiding full sequential consistency! 😹 ☢️ 🎉

@AZero13 AZero13 deleted the release branch January 12, 2024 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants