Abandon Windows-internal size optimizations for mutex and condition_variable#5030
Merged
StephanTLavavej merged 1 commit intomicrosoft:mainfrom Oct 30, 2024
Merged
Conversation
Member
Author
|
Marking as blocked until @barcharcraz can validate this with a Windows build. |
Contributor
|
Isn't there mix-and-match scenario by already shipped DLLs with new DLLs mixing? |
CaseyCarter
approved these changes
Oct 21, 2024
Member
Author
|
Windows DLLs have C/COM/WinRT/etc. binary-stable interfaces, never involving STL types. (And if they had been exporting their internal |
Member
Author
|
Charlie reported her build results - x86 passed, x64 failed for unrelated reasons. That's good enough for me. |
Member
Author
|
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
Contributor
|
Thanks for closing this window for ODR violations! 🪟 |
This file contains hidden or 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
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.
For ages, we've had an "optimization" in the machinery for
mutexandcondition_variable, reducing their bloated sizes for Windows-internal builds. This is a relic of when we switched between different implementations for XP, Vista, and Win7; the largest size consumption was for the ConcRT-powered XP implementation. Windows could always assume it was the latest Windows, so it never needed that switching.Unfortunately, the world is not as simple as we believed it to be. We thought that Windows was built consistently with a macro identifying it as Windows-internal, and that such object files were never mixed with ordinary object files (built with public VS). First, the macro scheme either changed or we didn't fully understand it in the first place (
_CRT_WINDOWSvs.UNDOCKED_WINDOWS_UCRT), and second there's a lot of mixing between Windows-internal code and public-VS code via vcpkg and possibly other scenarios. See #4294 and #4301 for previous history here.As @barcharcraz and @CaseyCarter noted to me, the fact that we can't properly ship a
#pragma detect_mismatchto verify representation consistency is a severe problem. We keep getting reports from Windows devs who are encountering mismatch scenarios, and while they've been messing with their build settings to get around this, it's a strong indication that we should abandon the attempt.This PR makes Windows-internal code behave exactly the same as public-VS code has always behaved, getting the unfortunately-bloated sizes. (Due to a long series of cleanups, made possible by dropping XP/Vista support, we now initialize only two pointers of this bloated space, and then we actually use only one, so it isn't as bad as it was before.)
There is no escape hatch for Windows-internal code - we're going to try to rip off the bandage with no escape hatch. (An escape hatch would just lead to more mismatch problems.) If the increased sizes cause performance regressions, that's an indication that they should be directly using Windows synchronization primitives. If there's mismatch caused by picking up this change in a non-simultaneous manner (e.g. updated Windows-internal "LKG" compiler, but using old public VS with the Windows-internal macros), the fix is to pick up the latest toolsets (possibly requiring backports on our side) to consistently get this change.
Note that this does not affect normal VS users.
Instead of mentioning the whole history in a comment (which is essentially irrelevant for understanding the state of the code after this change), or even mentioning that the constants could be reduced to
2 * sizeof(void*)if we could break ABI, I've gone further and noted the change that we actually need to make in vNext - ripping out this entire layer of machinery and using only one pointer. This was essentially implied by the existing TRANSITION comment about unused vptrs, but now we're clearly stating it.