perf(semaphore): streamline permit distribution to waiting acquirers - #335
BewareMyPower wants to merge 10 commits into
Conversation
| } | ||
|
|
||
| struct PanicGuard { | ||
| first: Option<Box<dyn Any + Send>>, |
There was a problem hiding this comment.
I don't think we would like to have such a dynamic thing.
There was a problem hiding this comment.
I make the guard inline now. Based on the current design, the dynamic dyn Any + Send is necessary because catch_unwind call on a closure returns Result<(), Box<dyn Any + Send>>.
`insert_permits_with_lock` woke each batch of wakers while it was still distributing permits, so a wake callback that panicked had to be caught, its payload retained in a `PanicGuard`, and rethrown once the release finished. Collect every waker first and notify after the accounting is complete instead. A wake panic can no longer distort the distribution, and `PanicGuard`, the per-batch `catch_unwind`, and the `will_spill` chunk boundary all go away. Holding every waker until the release finishes lets the batch outgrow its inline storage on wide releases, and dropping the chunk boundary costs more than the removed `catch_unwind`. Measured against the previous commit (`divan`, `release_to_waiters`, six runs): 1 waiter 10.5 -> 11.2 ns, 2 waiters 13.8 -> 14.7 ns, 32 waiters 153 -> 188 ns, 256 waiters 1.11 -> 1.80 us. The changelog entry from the previous commit is dropped because the net effect against the latest release is no longer a clear improvement.
This reverts commit 45e202b. Holding every waker until the release finishes spills the batch to the heap on wide releases, and dropping the chunk boundary cost more than the removed `catch_unwind`; against the previous commit (`divan`, `release_to_waiters`, six runs) 32 waiters went 153 -> 188 ns and 256 waiters 1.11 -> 1.80 us.
The payload only needs to survive until the release completes, so a local `Option<Box<dyn Any + Send>>` and a trailing `resume_unwind` express the same thing as the guard type without the `Drop` impl. Rethrowing on the normal path instead of from `Drop` also removes an abort: if the permit-overflow `assert!` fired after a wake panic had been recorded, the guard resumed the retained payload while the thread was already unwinding. The retained payload is now dropped during that unwind, so the overflow panic reaches the caller.
|
The explicit loop is easier to follow, and it preserves the first-panic behavior across batches. Tests on Rust 1.86 and 1.98, Miri, and feature checks passed locally; the benchmarks improved too. Small nit: the description still mentions |
|
PR description is updated now. P.S. I told my agent to only generate a new summary without running |
tisonkun
left a comment
There was a problem hiding this comment.
After a closer look I consider the let mut first_panic = None; is the second implementation for what wake_all delievered already. The back and forth catch_unwind/rewind seems putting more indirection.
Then I think the main branch version would be better in this case. Sorry to provide wrong insight.
Summary
Rework how
Semaphoredistributes released permits to waiting acquirers so the distribution no longer runs insideDropduring unwinding.insert_permits_with_lockcollects up toWakerBatch::STACK_SIZEwakers under the wait-queue lock, releases the lock, and hands the batch towake_allby mutable reference, repeating until the permits are exhausted.wake_allkeeps owning the "wake every waker, keep the first panic" policy. The loop retains the first panic payload in a local, keeps distributing on the normal path, and rethrows it withresume_unwindonce the release is complete.&mut batchinstead of the owned batch avoids copying the inline waker storage intowake_allfor every batch of 32 waiters.Semaphore::releasekeeps its documented contract: added permits remain available, every eligible waiter is still notified, and the first wake panic still reaches the caller.Design Notes
The previous implementation passed a lazily refilling iterator to
wake_alland relied onWakeRemaining::dropto keep pulling batches during unwinding, so a panicking wake callback could not stop the remaining permits from being distributed. That is correct, but it runs the permit accounting, the lock acquisition, and the wait-list mutations while the thread is unwinding — the hardest state to reason about, and the reason the loop read as an iterator closure rather than as the distribution itself.Keeping the loop on the normal path makes the failure behavior explicit. Only the wake callbacks stay inside
catch_unwind, and the retained payload is a plain local rather than a guard type: rethrowing from the normal path also means a later panic (the permit-overflowassert!) no longer resumes the retained payload while the thread is already unwinding.Performance
cargo bench -p benchmarks --bench benchmarks -- release_to_waiters, median of six runs with the revisions interleaved in both orders and a fresh compile per revision (mainis the released implementation; absolute values are from one machine):The 1- and 2-waiter rows are stable across runs and reproduce an 18-20% reduction in release-to-waiter latency. The 32- and 256-waiter rows move by a few percent, within the run-to-run spread on the development machine, and are best read as neutral to slightly better.
Validation
cargo x test,cargo x lint, andcargo x check(all 27 feature combinations under-D warnings).internal::semaphore::tests::panicking_wakes_preserve_permits_and_the_first_paniccovers the panic path: 65 waiters, two panicking wake callbacks, all 65 woken exactly once, remaining permits correct, first panic preserved.