Skip to content

All atomic exchanges with _Api_level should be acq_rel#4288

Merged
StephanTLavavej merged 1 commit intomicrosoft:mainfrom
SiliconA-Z:release
Jan 11, 2024
Merged

All atomic exchanges with _Api_level should be acq_rel#4288
StephanTLavavej merged 1 commit intomicrosoft:mainfrom
SiliconA-Z:release

Conversation

@SiliconA-Z
Copy link
Contributor

@SiliconA-Z SiliconA-Z 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.

@SiliconA-Z SiliconA-Z requested a review from a team as a code owner December 24, 2023 19:02
@SiliconA-Z SiliconA-Z force-pushed the release branch 4 times, most recently from 0a52f4f to 5bde50e Compare December 25, 2023 16:40
@CaseyCarter CaseyCarter added the enhancement Something can be improved label Jan 3, 2024
@SiliconA-Z SiliconA-Z 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
@SiliconA-Z SiliconA-Z requested a review from CaseyCarter January 4, 2024 18:39
@SiliconA-Z
Copy link
Contributor Author

@CaseyCarter Fixed!

@SiliconA-Z SiliconA-Z 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
@SiliconA-Z SiliconA-Z 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
@SiliconA-Z SiliconA-Z 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 added performance Must go faster and removed enhancement Something can be improved labels Jan 11, 2024
@StephanTLavavej StephanTLavavej merged commit 1fbc77c into microsoft:main Jan 11, 2024
@StephanTLavavej
Copy link
Member

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

@SiliconA-Z SiliconA-Z 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