-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Overhaul condition_variable
#4720
Merged
StephanTLavavej
merged 13 commits into
microsoft:main
from
StephanTLavavej:condition_variable-overhaul
Jun 18, 2024
Merged
Overhaul condition_variable
#4720
StephanTLavavej
merged 13 commits into
microsoft:main
from
StephanTLavavej:condition_variable-overhaul
Jun 18, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
`~condition_variable()` can be defaulted, making it trivially destructible. `~condition_variable_any()` can also be defaulted. It's destroying a `shared_ptr`, so it's not trivially destructible. Stop declaring `_Cnd_destroy_in_situ()` in xthreads.h. Note that this doesn't affect the dllexport surface, as we previously ensured that `_CRTIMP2_PURE` always appears on both declarations and definitions. Comment the definition of `_Cnd_destroy_in_situ()` in cond.cpp as preserved for bincompat.
This follows the example of `_Mtx_internal_imp_t` and `_Stl_critical_section` above. Define `_Stl_condition_variable` mirroring `Concurrency::details::stl_condition_variable_win7`. We can mirror `CONDITION_VARIABLE m_condition_variable = CONDITION_VARIABLE_INIT;` with `void* _Win_cv = nullptr;` because `CONDITION_VARIABLE` is just a struct wrapping `void*` (so it has the same size and alignment) and `CONDITION_VARIABLE_INIT` just initializes it to null. Then, use the `union` pattern to keep the size and alignment of `_Cnd_internal_imp_t` unchanged, while initializing `_Stl_condition_variable _Stl_cv{};` with a default member initializer, so its two pointers will be null. (The size is unchanged because `_Cv_storage` is at least `2 * sizeof(void*)`. The alignment is unchanged because everything is `void*`-aligned here.) Finally, add empty braces when we have `_Cnd_internal_imp_t _Cnd_storage{};` data members. These aren't necessary, but they serve as a reminder that we're getting null instead of garbage, and this is consistent with how we have `_Mtx_internal_imp_t _Mtx_storage{};`. Note that I chose these names to avoid confusion between the different layers: * `_Stl_cv` stores two pointers, including the unused vptr * `_Win_cv` directly mirrors `CONDITION_VARIABLE`
…PR_MUTEX_CONSTRUCTOR` as the escape hatch. We just changed `condition_variable_any` and `condition_variable` to statically initialize `_Cnd_internal_imp_t`. `condition_variable`'s default constructor can now be defaulted. In xthreads.h, declare `_Cnd_init_in_situ()` only when needed for the escape hatch. (Again, this doesn't affect bincompat, the definition still has `_CRTIMP2_PURE`.) Comment `_Cnd_init_in_situ()` as preserved for bincompat and the escape hatch. Its only caller is `_Cnd_init()` immediately below, which is only called by `_Thrd_create()`, which is preserved for bincompat. We need the escape hatch for the same reason that `mutex` does. When a user hasn't followed our documented restrictions for binary compatibility, and mixes new headers with a very old STL DLL (older than VS 2022 17.8, which was the release that shipped GH 3770), the headers will initialize the vptr to null, but the old STL DLL will want to actually use the vptr.
Constructing `_Cnd_internal_imp_t` will now perform the initialization that we need.
We declared them in xthreads.h (guarded by `_CRTBLD`) because they were defined by cond.cpp and used by cthread.cpp. As usual, dropping these declarations won't affect bincompat, as the definitions are consistently marked `_CRTIMP2_PURE`. Mark the definitions as preserved for bincompat. In cthread.cpp `_Thrd_create()` (itself preserved for bincompat), we don't need to dynamically allocate and deallocate a condition variable. Locally initializing `_Cnd_internal_imp_t cond_var{};` and taking its address is sufficient.
We declared them in xthreads.h (guarded by `_CRTBLD`) because they were defined by mutex.cpp and used by cthread.cpp. As usual, dropping these declarations won't affect bincompat, as the definitions are consistently marked `_CRTIMP2_PURE`. Mark the definition of `_Mtx_init()` as preserved for bincompat. `_Mtx_destroy()` was already marked. In cthread.cpp `_Thrd_create()` (itself preserved for bincompat), we don't need to dynamically allocate and deallocate a mutex. Locally initializing `_Mtx_internal_imp_t mtx_var{};`, taking its address, and calling `_Mtx_init_in_situ()` is sufficient. We don't need to call `_Mtx_destroy_in_situ()` (which isn't declared). It's a no-op except for a debug check "mutex destroyed while busy". `std::mutex` doesn't bother with that anymore, and we're definitely calling `_Mtx_unlock()` so we're fine.
And update its comment accordingly.
…member functions. This wasn't dllexported, and the changes are limited to stl/src, so there's no bincompat impact. I chose the naming scheme `_Primitive_wait` to avoid confusion with `_Cnd_wait` which does more work. The functions don't need to be extern "C", because they don't interact with user code. (They do need to be ugly to avoid conflicts during static linking.) They have to be `inline`. I also marked them as `noexcept`, although that wasn't necessary. Because they aren't member functions anymore, I have to define `_Primitive_wait_for()` before `_Primitive_wait()` can call it.
Now that the originally-ConcRT-derived layer has been eradicated, we don't need to make every caller reach into `_Mtx_t` internals. This also increases consistency with the `_Cnd_t cond` parameter.
I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
I had to push an additional commit to silence a false positive warning from static analysis. |
barcharcraz
approved these changes
Jun 14, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
This improves how we initialize
condition_variable
andcondition_variable_any
. It also eliminates layers of confusing machinery likeConcurrency::details::stl_condition_variable_win7
. I've been careful to preserve bincompat at each step. (All of the otherConcurrency::details
machinery was dllexported, but not this one.) This also respects the escape hatch macro_DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR
, for users who aren't following our documented binary compatibility requirements and are mixing new STL headers with a very old STL DLL (older than VS 2022 17.8), so it doesn't affect that situation any further.Commits
mutex.cpp doesn't use anything from primitives.hpp.
primitives.hpp doesn't need
<type_traits>
.Stop calling no-op
_Cnd_destroy_in_situ()
.~condition_variable()
can be defaulted, making it trivially destructible. (Note that this also makestimed_mutex
andrecursive_timed_mutex
trivially destructible.)~condition_variable_any()
can also be defaulted. It's destroying ashared_ptr
, so it's not trivially destructible.Stop declaring
_Cnd_destroy_in_situ()
in xthreads.h. Note that this doesn't affect the dllexport surface, as we previously ensured that_CRTIMP2_PURE
always appears on both declarations and definitions.Comment the definition of
_Cnd_destroy_in_situ()
in cond.cpp as preserved for bincompat.Statically initialize
_Cnd_internal_imp_t
.This follows the example of
_Mtx_internal_imp_t
and_Stl_critical_section
above.Define
_Stl_condition_variable
mirroringConcurrency::details::stl_condition_variable_win7
.We can mirror
CONDITION_VARIABLE m_condition_variable = CONDITION_VARIABLE_INIT;
withvoid* _Win_cv = nullptr;
becauseCONDITION_VARIABLE
is just a struct wrappingvoid*
(so it has the same size and alignment) andCONDITION_VARIABLE_INIT
just initializes it to null.Then, use the
union
pattern to keep the size and alignment of_Cnd_internal_imp_t
unchanged, while initializing_Stl_condition_variable _Stl_cv{};
with a default member initializer, so its two pointers will be null. (The size is unchanged because_Cv_storage
is at least2 * sizeof(void*)
. The alignment is unchanged because everything isvoid*
-aligned here.)Finally, add empty braces when we have
_Cnd_internal_imp_t _Cnd_storage{};
data members. These aren't necessary, but they serve as a reminder that we're getting null instead of garbage, and this is consistent with how we have_Mtx_internal_imp_t _Mtx_storage{};
.Note that I chose these names to avoid confusion between the different layers:
_Stl_cv
stores two pointers, including the unused vptr_Win_cv
directly mirrorsCONDITION_VARIABLE
Now we can stop calling
_Cnd_init_in_situ()
, with_DISABLE_CONSTEXPR_MUTEX_CONSTRUCTOR
as the escape hatch.We just changed
condition_variable_any
andcondition_variable
to statically initialize_Cnd_internal_imp_t
.condition_variable
's default constructor can now be defaulted.In xthreads.h, declare
_Cnd_init_in_situ()
only when needed for the escape hatch. (Again, this doesn't affect bincompat, the definition still has_CRTIMP2_PURE
.)Comment
_Cnd_init_in_situ()
as preserved for bincompat and the escape hatch. Its only caller is_Cnd_init()
immediately below, which is only called by_Thrd_create()
, which is preserved for bincompat.We need the escape hatch for the same reason that
mutex
does. When a user hasn't followed our documented restrictions for binary compatibility, and mixes new headers with a very old STL DLL (older than VS 2022 17.8, which was the release that shipped #3770), the headers will initialize the vptr to null, but the old STL DLL will want to actually use the vptr. The escape hatch makes the headers call into the old STL DLL to set up a real vptr.Simplify the definition of
_Cnd_init_in_situ()
.Constructing
_Cnd_internal_imp_t
will now perform the initialization that we need.Stop calling
_Cnd_init()
and_Cnd_destroy()
.We declared them in xthreads.h (guarded by
_CRTBLD
) because they were defined by cond.cpp and used by cthread.cpp. As usual, dropping these declarations won't affect bincompat, as the definitions are consistently marked_CRTIMP2_PURE
.Mark the definitions as preserved for bincompat.
In cthread.cpp
_Thrd_create()
(itself preserved for bincompat), we don't need to dynamically allocate and deallocate a condition variable. Locally initializing_Cnd_internal_imp_t cond_var{};
and taking its address is sufficient.Stop calling
_Mtx_init()
and_Mtx_destroy()
.We declared them in xthreads.h (guarded by
_CRTBLD
) because they were defined by mutex.cpp and used by cthread.cpp. As usual, dropping these declarations won't affect bincompat, as the definitions are consistently marked_CRTIMP2_PURE
.Mark the definition of
_Mtx_init()
as preserved for bincompat._Mtx_destroy()
was already marked.In cthread.cpp
_Thrd_create()
(itself preserved for bincompat), we don't need to dynamically allocate and deallocate a mutex. Locally initializing_Mtx_internal_imp_t mtx_var{};
, taking its address, and calling_Mtx_init_in_situ()
is sufficient. We don't need to call_Mtx_destroy_in_situ()
(which isn't declared). It's a no-op except for a debug check "mutex destroyed while busy".std::mutex
doesn't bother with that anymore, and we're definitely calling_Mtx_unlock()
so we're fine.Declare
_Mtx_init_in_situ()
only when needed.And update its comment accordingly.
Replace
Concurrency::details::stl_condition_variable_win7
with non-member functions.This wasn't dllexported, and the changes are limited to stl/src, so there's no bincompat impact.
I chose the naming scheme
_Primitive_wait
to avoid confusion with_Cnd_wait
which does more work.The functions don't need to be
extern "C"
, because they don't interact with user code. (They do need to be ugly to avoid conflicts during static linking.)They have to be
inline
. I also marked them asnoexcept
, although that wasn't necessary.Because they aren't member functions anymore, I have to define
_Primitive_wait_for()
before_Primitive_wait()
can call it.Simplify
_Primitive_wait[_for]()
by taking_Mtx_t mtx
.Now that the originally-ConcRT-derived layer has been eradicated, we don't need to make every caller reach into
_Mtx_t
internals. This also increases consistency with the_Cnd_t cond
parameter.Skip 3 libcxx tests for Clang until LLVM-94907 is merged.