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
Windows: Implement condvar, mutex and rwlock using futex #121956
Conversation
r? @Nilstrieb rustbot has assigned @Nilstrieb. Use r? to explicitly pick a reviewer |
The Miri subtree was changed cc @rust-lang/miri |
This comment has been minimized.
This comment has been minimized.
508a095
to
f15b4f7
Compare
0aa6381
to
2c7e068
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Miri shim LGTM :)
Incidentally @joboet, I would consider this a successful test case for the sys restructuring. Reusing the futex implementations on Windows was super easy 😁 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the changes look good to me, but I'm not comfortable approving Windows futex.rs because I don't know very much about Windows and/or the futex interface. maybe @joboet is?
Well, the Windows equivalent: `WaitOnAddress`, `WakeByAddressSingle` and `WakeByAddressAll`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I'll take a look!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
r=me after fixing the nit, looks great!
Switching sync implementations is a big enough deal that I think this should prefer to avoid being rolled up, if possible. @bors r=joboet rollup=iffy |
☀️ Test successful - checks-actions |
Finished benchmarking commit (3314d5c): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 646.206s -> 646.923s (0.11%) |
Since rust-lang/rust#121956 we don't need it any more, and we are generally short on Windows staff so reducing the amount of code we have to test and maintain sounds like a good idea. The InitOnce stuff is still used by `thread_local_key::StaticKey`.
windows: remove support for slim rwlock Since rust-lang/rust#121956 we don't need it any more, and we are generally short on Windows staff so reducing the amount of code we have to test and maintain sounds like a good idea. The InitOnce stuff is still used by `thread_local_key::StaticKey` on 64bit windows-gnu.
Since rust-lang#121956 we don't need it any more, and we are generally short on Windows staff so reducing the amount of code we have to test and maintain sounds like a good idea. The InitOnce stuff is still used by `thread_local_key::StaticKey`.
windows: remove support for slim rwlock Since rust-lang#121956 we don't need it any more, and we are generally short on Windows staff so reducing the amount of code we have to test and maintain sounds like a good idea. The InitOnce stuff is still used by `thread_local_key::StaticKey` on 64bit windows-gnu.
windows: remove support for slim rwlock Since rust-lang/rust#121956 we don't need it any more, and we are generally short on Windows staff so reducing the amount of code we have to test and maintain sounds like a good idea. The InitOnce stuff is still used by `thread_local_key::StaticKey` on 64bit windows-gnu.
Well, the Windows equivalent:
WaitOnAddress
,WakeByAddressSingle
andWakeByAddressAll
.Note that Windows flavoured futexes can be different sizes (1, 2, 4 or 8 bytes). I took advantage of that in the
Mutex
implementation.I also edited the Mutex implementation a bit more than necessary. I was having trouble keeping in my head what 0, 1 and 2 meant so I replaced them with consts.
I think we're maybe spinning a bit much.
WaitOnAddress
seems to be looping quite a bit too. But for now I've keep the implementations the same. I do wonder if it'd be worth reducing or removing our spinning on Windows.This also adds a new shim to miri, because of course it does.
Fixes #121949