New mutex and condition variable implementation on Windows #550
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
This uses SRWLock on Vista and above, and uses a hand-rolled implementation on Windows XP that uses Events (and a spinlock, if this is a multi-core system). Since SRWLocks aren't recursive, ReMutexWin32Impl has been added to implement recursive mutices, using old-fashioned critical sections. MutexImpl now has a constexpr constructor on all implementations.
See #550. Now that we use the Vista API for condition variables, the only place where the distinction is still relevant is Windows XP, and it's just not worth it for that one corner case.
This is possible now that all MutexImpl have a constexpr constructor.
MutexImpl guarantees nowadays to be initialized at constant init time, so this is safer on Windows, where we can't rely on magic statics.
rdb
force-pushed
the
win32-new-mutex-impl
branch
from
5a08a5c
to
11b5f09
Compare
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.
This PR replaces Panda's Mutex and ConditionVar implementation on Windows to no longer use critical sections.
Windows Vista introduced slim reader/writer locks and proper condition variables that have various advantages over critical sections. They are quite slim (8 bytes instead of 40), can be more efficient (uses keyed events, not full-fledged event objects), don't require setup/teardown calls and can in fact be zero-initialized at constant init time.
That last point is particularly interesting because there are various places in Panda where we use static mutexes that we have to jump through hoops to initialize properly, especially because we can't use magic statics (#548). This change would allow us to fix those problems more neatly.
Unlike critical sections, SRWLocks are non-recursive, but I think this is a good thing. The recursive nature of the current MutexWin32Impl on Windows can hide bugs that would otherwise occur on another platform. We will still use critical sections for ReMutex.
The big catch is that we can't use them on Windows XP. To remedy this, I've hand-rolled a mutex implementation for Windows XP that satisfies the same constraints. I'm pretty sure that my implementation is fairly similar to Windows XP's implementation of critical sections, except being zero-initialized, using only 16 bytes and without support for recursion.
At static init time, Panda will initialize function pointers to the right implementation. When we can drop Windows XP (#548) we can just trivially change this to directly wire it to the SRW functions.
As for condition variables, we currently have two implementations on Windows; one that is slower but has full broadcast semantics (ConditionVarFull), and one that is faster but doesn't have notify_all. I would propose we get rid of the limited version; now that we can use the Vista primitives, the only case where this distinction is still relevant is in the Windows XP compatibility emulation, and I would prefer to sacrifice a bit of performance in this tiny corner case so that we don't have to keep the awkwardness of two separate implementations with subtly different semantics.