Repository navigation
perf(waitgroup): streamline completion for participants and observers - #421
nanami-0713 wants to merge 4 commits into
Conversation
| #[inline(never)] | ||
| fn finish(&mut self) { | ||
| self.waiters.wake_all(); | ||
| self.release_observer(); |
There was a problem hiding this comment.
Could a panic in wake_all() prevent release_observer() from running and end up leaking the state? Maybe we should ensure cleanup still happens if a waker panics.
There was a problem hiding this comment.
Walking the path: Waiters::wake_all detaches the slots under the write lock and wakes after unlocking, so a panicking wake callback runs with no internal lock held. If one still panics:
- The observers after the panicking one never receive their wake. That is inherent to a panic mid-notification and predates this change — the previous implementation also
mem::take-ed the waker collection under the lock and calledwake_alloutside it, so a panic in the same place stranded the remaining observers identically. - What this change does alter is reclamation: with count-based ownership the panicking path skips
release_observer, so the state allocation is not reclaimed while unwinding, where the previousArc-based implementation would have freed it.
That difference lives entirely on a path the repository's waker contract excludes: executor waker operations, including waking and dropping, are expected not to panic, and primitives deliberately add no custom panic recovery (AGENTS.md "Waker Contract", decision #257). A drop guard in finish that releases the observer count on unwind would be exactly that recovery machinery, and the contract's review guidance is to not require it absent an explicit local contract.
|
i also think earlier PR showed a small performance regression on Linux in the four-observer case. It would be worth rerunning the benchmark to check whether thats still present. |
|
On the four-observer Linux point: the only Linux four-observer data I can find is the arm64 Linux VM control documented in #395 — a standalone driver repeating the same four-observer lifecycle, with ten-run medians of 159 ms on main vs 164 ms on the revised head I reran the comparison on hardware I have (Apple M5, 10 cores, macOS 27.0.1, rustc 1.99.1 stable), back-to-back in one session,
So on arm64 macOS the revised implementation is clearly ahead on the four-observer drain, matching #395's own M4 Max numbers for the same comparison (294 → 230/221 ms); the small deficit exists only in the Linux VM control. I don't have Linux hardware to rerun that control. The scenario is now in-tree, so a Linux check is one command away for anyone who has the hardware: cargo x bench --bench ecosystem -- waitgroup/drain-4-observers --repeat 10Happy to fold in a Linux datapoint if someone can run it. |
|
@nanami-0713 totaly missed it, thanks for clarifying : ) |
|
Ran the four-observer benchmark on native x86_64 Linux (Intel), using 2.5M rounds and
Used Hope this helps with the Linux benchmark comparison <3 |
Summary
Continues #395 on top of current main (
629e073). The four commits rebase cleanly:git range-diffshows three commits identical and only the CHANGELOG entry moving to the current Unreleased section. Fixes #397.The change, as sketched in #395:
RwLock, whose uncontended state stays inline on Darwin, where locking a standard mutex allocates a pthread mutex on first use.Measured on Apple M5 (10 cores), macOS 27.0.1, rustc 1.99.1 stable, with
cargo x bench --bench ecosystem -- waitgroup --repeat 10:629e073A
#[global_allocator]probe counts three allocations (208 B) percomplete-waiterround on main — the state allocation, Darwin's pthread-mutex boxing on the first lock, and the firstArena<Waker>insert — and one allocation per round on this PR: 56 B, 16 B above waitgroup's 40 B, covering the inline first-waker slot.cargo x test(including the 15 waitgroup integration tests) andcargo x lintpass on the rebased branch.