Skip to content
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

Implement synchronization primitive fallbacks for Windows XP/Vista #27036

Closed
wants to merge 1 commit into from

Conversation

MrAlert
Copy link
Contributor

@MrAlert MrAlert commented Jul 14, 2015

Since Windows Vista has partial support for SRWLocks (the
TryAcquireSRWLock* functions are missing,) the fallback functions
for the synchronization primitives are grouped such that if any of
the functions are unavailable, all of the fallback functions will be
used instead.

This at least partially addresses #26654.

Since Windows Vista has partial support for SRWLocks (the
`TryAcquireSRWLock*` functions are missing,) the fallback functions
for the synchronization primitives are grouped such that if any of
the functions are unavailable, all of the fallback functions will be
used instead.
@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@MrAlert
Copy link
Contributor Author

MrAlert commented Jul 14, 2015

One issue I haven't been able to resolve with this is that the test sync::condvar::tests::wait_timeout_with fails when using the fallbacks. (Tested on both Windows XP and Windows 7 by forcing them on.) All other tests under sync pass.

@nagisa
Copy link
Member

nagisa commented Jul 14, 2015

cc @retep998 @alexcrichton

@alexcrichton
Copy link
Member

Nice work! This is a pretty meaty implementation with unfortunately very few tests, so I'm hoping that we can restructure things to make it work out a bit nicer. Can you start out by putting all implementation details in their own module? For example something like src/libstd/sys/windows/fallback.rs. The shims in c.rs should be as small as possible, ideally just one line function calls to other in other modules (with perhaps some casts). This module should then also have a full suite of unit tests exercising the implementation whenever we build the standard test suite on Windows.

Also, can you investigate the performance implications of this custom implementation? I would expect the fallback implementation of a CRITICAL_SECTION for Mutexes may be more performant, but it'd be nice to get some numbers both ways.

@MrAlert
Copy link
Contributor Author

MrAlert commented Jul 16, 2015

Got a mutex benchmark at https://gist.github.com/MrAlert/f4317ec44768284670bf but I'm having trouble running it on XP with the old implementation because the benchmark/test infrastructure depends on condition variables for something.

running 36 tests
test mutex_1t_100c_25d ... thread '<main>' panicked at 'condition variables not available', src/libstd\sys/windows\c.rs:494
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: "PoisonError { inner: ..}"', src/libcore\result.rs:731

@alexcrichton
Copy link
Member

You can run the benchmark on a newer Windows version, right? (e.g. just make sure the wires are in place to use the fallback even if the new primitives are available)

@MrAlert
Copy link
Contributor Author

MrAlert commented Jul 17, 2015

Well, yes, but the condition variable implementation expects SRWLocks, not critical sections...though I guess if the mutexes are forced to use critical sections unconditionally, I could make it use SleepConditionVariableCS instead of SleepConditionVariableSRW...

@alexcrichton
Copy link
Member

Hm, I believe that reentrant mutexes on windows are implemented with critical sections, so perhaps those could be temporarily exposed to benchmark?

@MrAlert
Copy link
Contributor Author

MrAlert commented Jul 17, 2015

Alright. I think I got something working with mutexes forced to critical sections. At the very least, stage1 rustc is running without issues.

@bors
Copy link
Contributor

bors commented Sep 28, 2015

☔ The latest upstream changes (presumably #28689) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to send a new PR with my comments addressed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants