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

xnotify.cpp: SRWLOCK to protect the at-thread-exit data instead of an indexed CRITICAL_SECTION #4408

Merged
merged 4 commits into from Feb 27, 2024

Conversation

AlexGuteniev
Copy link
Contributor

@AlexGuteniev AlexGuteniev commented Feb 18, 2024

Towards #3521. Fixes #273.

After finally making mutex constructor and destructor constexpr, it is time that we start using usual mutexes!

@AlexGuteniev AlexGuteniev requested a review from a team as a code owner February 18, 2024 11:19
@github-actions github-actions bot added this to Initial Review in Code Reviews Feb 18, 2024
@StephanTLavavej StephanTLavavej added the performance Must go faster label Feb 18, 2024
@StephanTLavavej StephanTLavavej self-assigned this Feb 18, 2024
@frederick-vs-ja

This comment was marked as resolved.

@StephanTLavavej StephanTLavavej removed their assignment Feb 22, 2024
@StephanTLavavej StephanTLavavej moved this from Initial Review to Ready To Merge in Code Reviews Feb 22, 2024
@StephanTLavavej StephanTLavavej self-assigned this Feb 23, 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 changed the title xnotify.cpp: SRWLOCK to protect the new handler instead of an indexed CRITICAL_SECTION xnotify.cpp: SRWLOCK to protect the at-thread-exit data instead of an indexed CRITICAL_SECTION Feb 23, 2024
@StephanTLavavej
Copy link
Member

Fixed copy-pasted title.

@@ -13,7 +13,7 @@

_STD_BEGIN

constexpr int _Max_lock = 8; // must be power of two
constexpr int _Max_lock = 8; // must be power of two; TRANSITION, ABI: may be less now
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No change requested: It would actually be ABI-safe to reduce _Max_lock to 4, assuming that nobody external was ever hijacking our internal APIs for their own nefarious purposes. I might do this in a followup PR. I noticed it too late to update this comment and it's not worth resetting testing.

(The remaining _LOCK_LOCALE, _LOCK_STREAM, and _LOCK_DEBUG constants must remain unchanged so that old-compiled code and new-compiled code can agree on what they're locking. We're just fortunate that they all happened to be less than 4.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. As vNext is clearly not happening soon, we might want to keep some mutexes around for C++26 features or whatever.

@StephanTLavavej
Copy link
Member

I've pushed a merge with main to resolve a trivial conflict with #4407 in <yvals.h>.

@StephanTLavavej StephanTLavavej merged commit 85c6ecd into microsoft:main Feb 27, 2024
35 checks passed
Code Reviews automation moved this from Ready To Merge to Done Feb 27, 2024
@StephanTLavavej
Copy link
Member

Thanks for finding a nice way to clean up this old code! 🧹 🚀 😻

@AlexGuteniev AlexGuteniev deleted the slim_atexit branch February 27, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

xlock.cpp: Should _Lock_at_thread_exit_mutex() be noexcept?
3 participants