Use semaphores for thread parking on Apple platforms #102773
Conversation
rustbot
added
the
T-libs
|
Hey! It looks like you've submitted a new PR for the library teams! If this PR contains changes to any Examples of
|
rust-highfive
added
the
S-waiting-on-review
There was a problem hiding this comment.
I prefer us using dispatch semaphores to mach ones here. Mach semaphores are much heavier weight, and I'm not sure it's as well-integrated into the QOS system as dispatch_semaphore (I think that stuff is done somewhat manually from libdispatch).
The impl looks like a nearly complete match to the futex version (makes sense given that the futex is being used as a semaphore). That makes it pretty easy to review, not that it's unclear anyway.
I'm very happy to see another platform have the SeqCst barrier removed -- not because of performance though, but because we don't guarantee it's SeqCst, so I'd been a bit worried that people would start to rely on park() providing that to avoid races! This impl provides correct/optimal orderings that we guarantee, which is nice to see.
I'd like to avoid breaking miri for now, so won't R+ quite yet (and there are a few other nits). I may also run some benchmarks in the meantime (I know usync has benchmarks and heavily leverages stdlib parking, so I'm curious if it's impacted -- there are probably others too). A concern is that dispatch_semaphores are... quite heavyweight. For one, they're fair, which (at least if you implement a Mutex using a semaphore and counter), often makes their use come with a bit of a throughput loss...
I hope not though, it's a really nice impl with several benefits!
|
|
||
| #[link(name = "System", kind = "dylib")] | ||
| extern "C" { | ||
| fn dispatch_time(when: dispatch_time_t, delta: i64) -> dispatch_time_t; |
There was a problem hiding this comment.
Arguably these bindings should be in libc. I guess a reason not to put it there is that libdispatch can be used on other OSes (linux, windows, freebsd, ...), but needs a separate lib -- it's only part of libc libSystem on darwin-based OSes. Anyway, this is tiny, so I'm not that concerned.
| // decrementing the count because of a timeout, it means another | ||
| // thread is about to call semaphore_signal. We must wait for that | ||
| // to happen to ensure the semaphore count is reset. | ||
| while dispatch_semaphore_wait(self.semaphore, DISPATCH_TIME_FOREVER) != 0 {} |
There was a problem hiding this comment.
Note that this is technically a priority hole: if a high-priority thread uses park_timeout while a low priority thread tries to unpark it, a medium-priority thread could prevent the unpark from completing, so the high-priority thread would wait indefinitely. I doubt that anyone relies on this, but still... By the way, libdispatch itself is vulnerable to this.
There was a problem hiding this comment.
Yeah, that's a fundamental issue with stuff like semaphores.
|
I'm not able to find any changes when benchmarking parker throughput before/after, and this is significantly cleaner (so maybe we'd want it anyway) so we should be good to go. @bors r+ |
bors
added
S-waiting-on-bors
Use semaphores for thread parking on Apple platforms Currently we use a mutex-condvar pair for thread parking on Apple systems. Unfortunately, `pthread_cond_timedwait` uses the real-time clock for measuring time, which causes problems when the system time changes. The parking implementation in this PR uses a semaphore instead, which measures monotonic time by default, avoiding these issues. As a further benefit, this has the potential to improve performance a bit, since `unpark` does not need to wait for a lock to be released. Since the Mach semaphores are poorly documented (I could not find availability or stability guarantees for instance), this uses a [dispatch semaphore](https://developer.apple.com/documentation/dispatch/dispatch_semaphore?language=objc) instead. While it adds a layer of indirection (it uses Mach semaphores internally), the overhead is probably negligible. Tested on macOS 12.5. r? `@thomcc`
Use semaphores for thread parking on Apple platforms Currently we use a mutex-condvar pair for thread parking on Apple systems. Unfortunately, `pthread_cond_timedwait` uses the real-time clock for measuring time, which causes problems when the system time changes. The parking implementation in this PR uses a semaphore instead, which measures monotonic time by default, avoiding these issues. As a further benefit, this has the potential to improve performance a bit, since `unpark` does not need to wait for a lock to be released. Since the Mach semaphores are poorly documented (I could not find availability or stability guarantees for instance), this uses a [dispatch semaphore](https://developer.apple.com/documentation/dispatch/dispatch_semaphore?language=objc) instead. While it adds a layer of indirection (it uses Mach semaphores internally), the overhead is probably negligible. Tested on macOS 12.5. r? ``@thomcc``
Use semaphores for thread parking on Apple platforms Currently we use a mutex-condvar pair for thread parking on Apple systems. Unfortunately, `pthread_cond_timedwait` uses the real-time clock for measuring time, which causes problems when the system time changes. The parking implementation in this PR uses a semaphore instead, which measures monotonic time by default, avoiding these issues. As a further benefit, this has the potential to improve performance a bit, since `unpark` does not need to wait for a lock to be released. Since the Mach semaphores are poorly documented (I could not find availability or stability guarantees for instance), this uses a [dispatch semaphore](https://developer.apple.com/documentation/dispatch/dispatch_semaphore?language=objc) instead. While it adds a layer of indirection (it uses Mach semaphores internally), the overhead is probably negligible. Tested on macOS 12.5. r? ```@thomcc```
Use semaphores for thread parking on Apple platforms Currently we use a mutex-condvar pair for thread parking on Apple systems. Unfortunately, `pthread_cond_timedwait` uses the real-time clock for measuring time, which causes problems when the system time changes. The parking implementation in this PR uses a semaphore instead, which measures monotonic time by default, avoiding these issues. As a further benefit, this has the potential to improve performance a bit, since `unpark` does not need to wait for a lock to be released. Since the Mach semaphores are poorly documented (I could not find availability or stability guarantees for instance), this uses a [dispatch semaphore](https://developer.apple.com/documentation/dispatch/dispatch_semaphore?language=objc) instead. While it adds a layer of indirection (it uses Mach semaphores internally), the overhead is probably negligible. Tested on macOS 12.5. r? ````@thomcc````
Use semaphores for thread parking on Apple platforms Currently we use a mutex-condvar pair for thread parking on Apple systems. Unfortunately, `pthread_cond_timedwait` uses the real-time clock for measuring time, which causes problems when the system time changes. The parking implementation in this PR uses a semaphore instead, which measures monotonic time by default, avoiding these issues. As a further benefit, this has the potential to improve performance a bit, since `unpark` does not need to wait for a lock to be released. Since the Mach semaphores are poorly documented (I could not find availability or stability guarantees for instance), this uses a [dispatch semaphore](https://developer.apple.com/documentation/dispatch/dispatch_semaphore?language=objc) instead. While it adds a layer of indirection (it uses Mach semaphores internally), the overhead is probably negligible. Tested on macOS 12.5. r? `````@thomcc`````
Rollup of 6 pull requests Successful merges: - rust-lang#102773 (Use semaphores for thread parking on Apple platforms) - rust-lang#102884 (resolve: Some cleanup, asserts and tests for lifetime ribs) - rust-lang#102954 (Add missing checks for `doc(cfg_hide(...))`) - rust-lang#102998 (Drop temporaries created in a condition, even if it's a let chain) - rust-lang#103003 (Fix `suggest_floating_point_literal` ICE) - rust-lang#103041 (Update cargo) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Currently we use a mutex-condvar pair for thread parking on Apple systems. Unfortunately,
pthread_cond_timedwaituses the real-time clock for measuring time, which causes problems when the system time changes. The parking implementation in this PR uses a semaphore instead, which measures monotonic time by default, avoiding these issues. As a further benefit, this has the potential to improve performance a bit, sinceunparkdoes not need to wait for a lock to be released.Since the Mach semaphores are poorly documented (I could not find availability or stability guarantees for instance), this uses a dispatch semaphore instead. While it adds a layer of indirection (it uses Mach semaphores internally), the overhead is probably negligible.
Tested on macOS 12.5.
r? @thomcc