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

Make ~mutex trivial #4390

Merged
merged 1 commit into from
Feb 16, 2024
Merged

Conversation

CaseyCarter
Copy link
Member

By simply removing ~_Mutex_base. This destructor has been only a debug check since GH-3770. Losing the debug check is a small price to pay to elide the destructor call, doubly so for static storage duration mutexes that now won't need dynamic initializers to register with at_exit.

_Mtx_destroy and _Mtx_destroy_in_situ are only called by APIs preserved for binary compatibility, so mark them as also "preserved for binary compatibility".

By simply removing `~_Mutex_base`. This destructor has been only a debug check since microsoftGH-3770. Losing the debug check is a small price to pay to elide the destructor call, doubly so for static storage duration mutexes that now won't need dynamic initializers to register with `at_exit`.

`_Mtx_destroy` and `_Mtx_destroy_in_situ` are only called by APIs preserved for binary compatibility, so mark them as also "preserved for binary compatibility".
@CaseyCarter CaseyCarter added the performance Must go faster label Feb 13, 2024
@CaseyCarter CaseyCarter requested a review from a team as a code owner February 13, 2024 22:07
@@ -45,10 +45,6 @@ public:
}
#endif // ^^^ !defined(_DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR) ^^^

~_Mutex_base() noexcept {
_Mtx_destroy_in_situ(_Mymtx());
Copy link
Member Author

@CaseyCarter CaseyCarter Feb 13, 2024

Choose a reason for hiding this comment

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

I could be convinced to retain this debug check for ~recursive_mutex, which feels less performance-sensitive to me. Thoughts, comments?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with dropping it.

@@ -45,10 +45,6 @@ public:
}
#endif // ^^^ !defined(_DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR) ^^^

~_Mutex_base() noexcept {
_Mtx_destroy_in_situ(_Mymtx());
Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with dropping it.

@StephanTLavavej StephanTLavavej self-assigned this Feb 15, 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 merged commit b99404e into microsoft:main Feb 16, 2024
37 checks passed
@StephanTLavavej
Copy link
Member

Thanks for this trivial PR with nontrivial impact! 😹 🚀 🎉

@CaseyCarter CaseyCarter deleted the trivial-mutex-change branch February 16, 2024 21:20
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.

3 participants