Repository navigation
perf(waitgroup): streamline completion for participants and observers - #421
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 |
|
Thanks for running this — exactly the missing datapoint. Native x86_64 Linux showing the PR consistently ahead in both orderings settles the last open question for me: across PR-vs-main four-observer runs, arm64 macOS (M4 Max in #395, M5 in my table above) and now native x86_64 Linux are all ahead, and the only deficit on record remains the arm64 Linux VM control in #395 — which now reads as VM- or arm64-specific rather than a property of the implementation. Thanks again for taking the time to benchmark it <3 |
Waiter registration split observer state across a submodule, overloaded `Registration::Set(None)` as the unregistered state, and duplicated single-slot waker replacement logic while leaving `internal::mutex` gated on `waitgroup` without any remaining caller.
Fold waiter storage into `waitgroup::mod` with explicit `Unregistered`, `Inline`, and `Overflow(WakerToken)` states backed by `register_waker` and `WakerSet::{insert, update, remove}`. Restore `WaitGroup` and `Wait` unwind-safety auto-traits via `PhantomData<State>`, remove the invalid unwind-safety test, drop the unused `internal::mutex` feature gate, and align module documentation and the changelog entry with current conventions.
The shared-drain benchmark maintained a separate submodule with custom atomic waker plumbing to assert single wakeups inside a timing loop, and `tasks-4-workers-4w` primarily measured Tokio task spawning rather than `WaitGroup` coordination. Fold `drain-4-observers` into `benches/ecosystem/waitgroup.rs` on top of the shared `bench_context` helpers and drop the noisy task-spawning scenario so the suite focuses on `WaitGroup` coordination costs.
Add `crate::internal::rwlock::RwLock` alongside `crate::internal::mutex::Mutex` to handle lock-poisoning recovery consistently, and use it for `WaitGroup` waiter registration instead of open-coding `PoisonError::into_inner` in `waitgroup`.
Use `do_work().await` directly in the `WaitGroup` module example without redundant `JoinHandle` awaits or inline-const atomic arrays, and replace the inline-const atomic array in the `Barrier` module example with a single arrival counter.
|
Thanks for your contribution @nanami-0713! |
Summary
Continues #395. Fixes #397.
State::refsacross all liveWaitGrouphandles so cloning and dropping worker handles update only the participant count, while eachWaitfuture owns an individual reference.WakerSetlazily when multiple observers register concurrently, backed byWakerSet::{insert, update, remove}andregister_waker.crate::internal::rwlock::RwLock, whose uncontended state stays inline on Darwin wherestd::sync::Mutexallocates apthread_mutex_ton first lock (tracked globally in Evaluate an inline lock for internal::mutex::Mutex on Apple targets #425).WaitGroupintegration tests for concurrent registration, slot reuse, racing completion, and reentrant wake/drop paths, wirewaitgroup_testintocargo x miri, addwait-after-work,nested-4-workers, anddrain-4-observersecosystem benchmarks, and simplify theWaitGroupandBarriermodule examples.Benchmarks
Original PR measurement on Apple M5 (10 cores), macOS 27.0.1,
rustc 1.99.1stable, withcargo x bench --bench ecosystem -- waitgroup --repeat 10:629e073A
#[global_allocator]probe counts three allocations (208 B) percomplete-waiterround onmain(629e073) — the state allocation, Darwin'spthread_mutex_tboxing on the first lock, and the firstArena<Waker>insert — and one 56 B allocation per round on this PR.Updated apples-to-apples comparison on
HEAD(63f7c76) vsmain(8ee2520) on Apple M4 Max (aarch64-apple-darwin), macOS 26.6.2,rustc 1.101.0-nightly (8006091eb 2026-10-17),--repeat 10:8ee2520(ms)63f7c76(ms)waitgroup/complete-waiterwaitgroup 0.1.2: 68 (67..70)waitgroup/complete-4-workerswaitgroup 0.1.2: 87 (86..89)waitgroup/wait-after-workwaitgroup 0.1.2: 90 (89..94)waitgroup/nested-4-workerswaitgroup 0.1.2: 88 (87..92)waitgroup/drain-4-observersWaitgroupRsShared: 592 (588..597) /TokioTaskTracker: 381 (379..383)8ee2520(ms)63f7c76(ms)primitives::waitgroup::wait::complete_waiter_batch/1primitives::waitgroup::wait::complete_waiter_batch/4primitives::waitgroup::wait::complete_waiter_batch/16Validated on
63f7c76withcargo x lint,cargo test --doc --all-features -p asyncband, Miri onasyncband --lib --all-featuresandwaitgroup_test, and the ecosystem/primitives benchmarks above.