Skip to content

Windows Mutex implementation should be reevaluated on modern Windows targets #101621

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

Closed
Kixiron opened this issue Sep 9, 2022 · 4 comments
Closed
Labels
A-atomic Area: Atomics, barriers, and sync primitives O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@Kixiron
Copy link
Member

Kixiron commented Sep 9, 2022

Currently the windows Mutex implementation has this comment at the top talking about its locking approach (using SRWLock instead of CriticalSection) with the justification that SRWLock performs better on Windows 7 and Windows 8.
However, in the (seven) years since, Microsoft has ended their Windows 7 support in 2020, Windows 10 became the dominant windows version and Windows 11 was released in 2021.
In light of these massive shifts in the windows ecosystem, our mutex implementation's speed should probably be reevaluated on the more modern Windows 10 and 11 platforms

@ChrisDenton ChrisDenton added O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 9, 2022
@ChrisDenton
Copy link
Member

For background, there's some old history here. Firefox and Chrome switched to SRWLOCK circa 2017.

It would be good to have some modern benchmarks.

@ChrisDenton
Copy link
Member

cc @m-ou-se for tips on benchmarking mutex implementations.

@thomcc
Copy link
Member

thomcc commented Sep 9, 2022

SRWLOCK was still faster in win10 when I checked a while ago (using a modification of the parking_lot benchmarks (which themselves are a port of webkits mutex benchmarks). This was with InitializeCriticalSection rather than one of the initializers that configures a spin count, so that may impact things.

FWIW, CRITICALSECTION doesn't have a static initializer (AFAIK), and can't be moved (according to https://docs.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-initializecriticalsectionex), so we might have to use lazy initialization like on pthreads.

I think the comment is just a bit stale, but it doesn't hurt to re-evaluate.


That said, I would be interested in seeing how the futex-based locks perform compare to SRWLOCK, since windows does have futex operations and could plausibly use them.

@workingjubilee workingjubilee added the A-atomic Area: Atomics, barriers, and sync primitives label Mar 19, 2023
@ChrisDenton
Copy link
Member

Closing this as we've now switched to using futex-based locks due to an issue with the OS SRWLOCK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-atomic Area: Atomics, barriers, and sync primitives O-windows Operating system: Windows T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants