Repository navigation
Windows: Don't make RwLock writers race readers - #165337
R3nzTheCodeGOD wants to merge 1 commit into
Conversation
rustbot
added
S-waiting-on-review
|
Thanks for the pull request, and welcome! The Rust Project has assigned @ChrisDenton (or someone else) to review your changes, you should hear from them (or someone else) within the next two weeks. Please see the contribution instructions and our LLM policy for more information. |
| writer_notify: Futex, | ||
| // The number of writers going to sleep on `writer_notify`, for `wake_writer`. | ||
| #[cfg(target_os = "windows")] | ||
| blocked_writers: Futex, |
There was a problem hiding this comment.
This doesn't need to be futex-sized, but it must be large enough to hold the number of possible threads in the system if every one of them tried to write-lock at once. I suggest using AtomicU32 explicitly, that's large enough on Windows (thread IDs are 32-bit, so there cannot be more than
There was a problem hiding this comment.
The fix in general looks good but I'm a little uncomfortable sprinkling #[cfg(windows)] about the place, especially as this affects other platforms. Maybe this can be done using a separate wrapper type (e.g. BlockedWriters) that's just a ZST on most platforms? Or some way to make it easy to opt platforms in/out of this specifically.
| if self.state.compare_exchange(state, READERS_WAITING, Relaxed, Relaxed).is_err() { | ||
| // Acquire, so that `wake_writer` can't see a `blocked_writers` count from | ||
| // before the unlock of a writer that has already left. | ||
| if self.state.compare_exchange(state, READERS_WAITING, Acquire, Relaxed).is_err() { |
There was a problem hiding this comment.
I wouldn't have thought so but does this change have any measurable effect on Linux?
There was a problem hiding this comment.
No. x86_64 generates the same code for all of std. On aarch64 it's just __aarch64_cas4_relax -> __aarch64_cas4_acq on the path where readers and writers are both waiting. I compared the asm but didn't run aarch64. Can make the Acquire conditional if you prefer.
`WakeByAddressSingle` doesn't report whether it woke anything, so `futex_wake` is always false on Windows and `wake_writer_or_readers` wakes a writer and the readers together. The writer usually loses. Count the writers going to sleep on `writer_notify` and treat a non-zero count as a wakeup that reached a writer. Write unlocks also wake the readers, so that they don't have to wait for a whole queue of writers.
2fc9036 to
aefbd97
Compare
|
Done. Windows only for now. FreeBSD, DragonFly and Fuchsia would just need adding to the |
|
Thanks, I'll hand this over to joboet to double check the correctness r? joboet |
|
|
Fixes #165314
The futex
RwLockis meant to prefer writers. #95801 does that by waking a writer first and only waking readers iffutex_wakesays there was no writer to wake.WakeByAddressSingledoesn't report that, so on Windowsfutex_wakeis always false and we wake both and let them race, which is the thing #95801 set out to avoid. The writer usually loses.Just returning
truedoesn't work:WRITERS_WAITINGcan be stale, and then nothing ever wakes the readers. So this counts the writers going to sleep onwriter_notify. If the count isn't zero, a writer still has to retry the lock. It'll either take it or find it held, and either way whoever unlocks will seeREADERS_WAITING.The compare-exchange before
wake_writerhad to becomeAcquire. WithRelaxedit could see the unlock of a writer that had already left and still read the old count, which leaves the readers asleep for good. Miri finds this if you force the interleaving, and a litmus test of just this ordering on an Apple M4 saw the old count 718829 times in 2.9 billion runs withRelaxedand never in 2.8 billion withAcquire. It can't happen on x86, where the generated code doesn't change.I also made write unlocks wake the readers even when a writer is waiting, since otherwise a queue of writers can keep them out for as long as it lasts. That's the one place this differs from Linux, and it's easy to drop if you'd rather match it exactly. With 4 writers and 12 readers it gives 1.3 to 3 times the reads of the count alone, for about half the writes.
On a Ryzen 5 5600 with Windows 10, before and after:
That last row is the real cost. Reads were only that fast because writers barely got in, and total operations drop 60%. Read-heavy
HashMapcache loads kept their throughput, though reader p99 got worse in some of them, mostly with more threads than CPUs. In real code Typst doesn't contend at all, so nothing changes, and the OpenTelemetry SDK gained 14-32% in a tight loop and nothing once there was work between calls.RwLock<T>grows by 4 bytes unlessTis aligned to 8. The uncontended paths are effectively unchanged. The queue-basedRwLockis the obvious alternative and loses much less under heavy writes, but it makes uncontended reads 32% slower../x test library/stdand Miri pass, but only on x86_64. I don't have Windows on ARM. A copy of the algorithm ran 460 million mixed operations on 4 to 200 threads on the M4 without hanging, but that says little, since theRelaxedversion passes that stress test too. FreeBSD, DragonFly and Fuchsia have the samefutex_wakebehavior, and I've left them alone.r? ChrisDenton