Skip to content

perf(completion): make terminal waits lock-free - #288

Merged
tisonkun merged 3 commits into
apache:mainfrom
ryux1:perf/246-completion-atomic-status
Sep 10, 2026
Merged

perf(completion): make terminal waits lock-free#288
tisonkun merged 3 commits into
apache:mainfrom
ryux1:perf/246-completion-atomic-status

Conversation

@ryux1

@ryux1 ryux1 commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Closes #246.

Summary

  • Make completed and abandoned Completion waits lock-free with one OnceLock<Option<T>>; pending registration remains cancellable.
  • Reuse WakerSet::register, consolidate overlapping lifecycle tests, and cover terminal reads, pending repolls/cancellation, and notification fan-out in benchmarks.

Design Notes

An uninitialized cell means pending; a stored Some(value) means completed and a stored None means abandoned. The single completer detaches the waiter set before initializing the cell, so observing a terminal result also observes waiter detachment. Pending polls and cancellation recheck the cell under the waiter mutex. Waker cloning follows the existing short-lock registration policy; replaced/cancelled wakers are dropped and notifications run after unlocking.

Local measurements compare main (f056962) with this branch on Apple M4 Max, macOS 26.6.2, Rust 1.98.0. Both use the same benchmark harness, excluding observer-container construction and handle cloning from fan-out. Values are medians of six per-run medians in nanoseconds, with run order balanced across main, an intermediate AtomicBool variant, and this implementation.

Benchmark main This branch Change
ready_wait 6.117 3.107 -49.2%
abandoned_wait 6.139 2.738 -55.4%
cancel_pending 11.700 12.150 +3.8%
repoll_pending 5.280 5.492 +4.0%
complete_then_wait 65.965 62.005 -6.0%
notify_pending 94.505 90.465 -4.3%
fanout/2 174.150 164.350 -5.6%
fanout/8 420.250 387.500 -7.8%
fanout/32 1292.000 1164.000 -9.9%

Terminal reads improve substantially; pending cancellation and repolling pay roughly 0.45 ns and 0.21 ns respectively. Small fan-out differences are noisy. Completion/notification throughput is comparable to the AtomicBool variant, without maintaining a separate terminal flag.

Validation: cargo +stable x test and cargo +1.86.0 x test (484 tests each), cargo x check, cargo x lint, cargo +stable x bench --no-run, and all 14 Completion integration tests under Miri with strict provenance, symbolic alignment, and seeds 0..4.

@ryux1
ryux1 force-pushed the perf/246-completion-atomic-status branch from 8748fa7 to 5e2ac0a Compare September 7, 2026 21:39
@ryux1

ryux1 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto the current main (64d7a94) and resolved the changelog overlap, preserving the new MPSC entries alongside this PR’s completion entry. The full project gates pass on head 7010f81: cargo x check, cargo x test, cargo x lint, and cargo x miri. This is mergeable and ready for review.

@ryux1
ryux1 force-pushed the perf/246-completion-atomic-status branch from 5e2ac0a to 7010f81 Compare September 8, 2026 04:56
@QwQBiG

QwQBiG commented Sep 10, 2026

Copy link
Copy Markdown
Member

@ryux1 Thanks for your contribution.

CI is green now. I also tested 7010f81 locally on Linux (WSL): cargo x test and cargo x check passed, along with the Completion and trait tests on Rust 1.86.

Seven additional local tests covering reentrant waker callbacks, panic cleanup, and completion/cancellation races passed on stable, Rust 1.86, and under Miri with four seeds. I didn't find any blocking correctness issues. I haven't independently reproduced the benchmark comparison.

The conflict overlaps with the test-helper consolidation in #293. main now imports shared helpers from tests_integration, while this PR adds clone-callback support in the same area of completion_test.rs.

Could you preserve the shared-helper refactor and adapt the new clone-callback regression test on top? In particular, the shared assert_completes_without_deadlock now takes only the test closure. We can then recheck the updated version.

@QwQBiG
QwQBiG self-requested a review September 10, 2026 12:02

@tisonkun tisonkun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally LGTM.

I'm considering two minor improvements. Would test out tomorrow and get this merged.

@tisonkun
tisonkun merged commit 03a5b58 into apache:main Sep 10, 2026
9 checks passed
@tisonkun

tisonkun commented Sep 10, 2026

Copy link
Copy Markdown
Member

Thanks for your contribution @ryux1 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Explore a simpler waiter registration path for Completion

3 participants