Skip to content

refactor(mpsc): replace the legacy queue backend - #247

Open
mxsm wants to merge 6 commits into
apache:mainfrom
mxsm:mxsm-209
Open

refactor(mpsc): replace the legacy queue backend#247
mxsm wants to merge 6 commits into
apache:mainfrom
mxsm:mxsm-209

Conversation

@mxsm

@mxsm mxsm commented Aug 30, 2026

Copy link
Copy Markdown
Member

Summary

  • Replace the bounded and unbounded standard-library MPSC backends with owned queues while preserving endpoint APIs, auto traits, FIFO ordering, backpressure, cancellation safety, disconnection, and exactly-once destruction.
  • Refresh against current main (93b3413) and preserve its receiver-waker and semaphore changes.
  • Select bounded cursor alignment by architecture: 128 bytes on AArch64/ARM64EC, x86-64, and PowerPC64; 256 bytes on s390x; 64 bytes elsewhere.

Closes #209.

Design Notes

The unbounded queue protects sender-visible storage and receiver liveness with one mutex and transfers messages into a receiver-local batch. The bounded queue uses a preallocated, generation-stamped ring. A single Mutex<VecDeque> bounded implementation showed higher producer contention during review, so the ring is retained. Cursor padding uses conservative architecture estimates rather than assuming one cache-line size for every CPU.

Unsafe code is localized to bounded slots and their Sync implementation, with reservation, publication, acquisition, and reuse invariants documented. The endpoint trait matrix and wrapped receiver-disconnect drop-counting test remain covered, including the latter under Miri.

Benchmark Comparison

Measured both versions locally on September 4, 2026: Intel Core i7-11700K (8 cores / 16 logical processors), 64-bit Windows 11 Pro 10.0.26200, Rust 1.96.1 (31fca3adb), x86_64-pc-windows-msvc, default optimized bench profile. Base is current main at 93b3413dfcde09aaca937848ef7c1e815bbbb79e; head is 0f039429c1046cf5061f5b2659479dda07075084.

Both versions use the unchanged ecosystem MPSC harness: 16,384 messages per sample, bounded capacity 64, and all four producer counts. After cargo x bench passed on both versions, their generated ecosystem executables were run with --bench --color never --sample-count 100 'mpsc::.*concurrent.*Asyncband'. Five pairs were measured serially, alternating base/head and head/base order; no build, test, or Miri tasks ran alongside these measurements. Each table value is the median of the five per-run medians (100 samples per run, one batch per sample). Change is (head / base - 1) × 100; negative means less elapsed time.

Queue Producers Base median Head median Change
Bounded 1 2.220 ms 1.902 ms -14.3%
Bounded 2 1.701 ms 1.913 ms +12.5%
Bounded 4 1.921 ms 2.090 ms +8.8%
Bounded 8 2.568 ms 2.211 ms -13.9%
Unbounded 1 1.778 ms 2.164 ms +21.7%
Unbounded 2 1.404 ms 2.982 ms +112.4%
Unbounded 4 1.610 ms 3.047 ms +89.3%
Unbounded 8 1.823 ms 2.815 ms +54.4%

The unbounded backend regresses on this Windows host, and bounded results are mixed; these measurements do not establish an overall performance improvement. Eight-producer bounded results were especially variable in the initial 20-sample runs. With 100 samples, the five run medians span 2.229–3.166 ms for base and 2.165–2.381 ms for head. The previous mixed-machine comparison and unsupported changelog performance claim have been removed.

Validation

  • cargo x test: 480 tests and doctests passed.
  • cargo x check: feature matrix passed.
  • cargo x miri: 66 tests passed with -Zmiri-strict-provenance -Zmiri-symbolic-alignment-check.
  • cargo x bench: full suite passed on both base and head.
  • Clippy, rustfmt, typos, license checks, and rustdoc passed. cargo x lint stops at Taplo 0.10.0 on unchanged TOML files; the formatting failures also reproduce in a clean worktree of current main. The checks after Taplo were run separately using the commands defined by cargo x lint.

@tisonkun
tisonkun requested a review from orthur2 August 30, 2026 17:26
@orthur2

orthur2 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

OK. I'll review this PR tomorrow, or within the next few days.

@tisonkun tisonkun mentioned this pull request Aug 31, 2026
35 tasks

@orthur2 orthur2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for taking this on.

For reference, I ran these local benchmarks on an Apple M5 MacBook Pro (10-core CPU and arm64) running macOS 26.4.

I also wanted to check whether the bounded queue really needed the stamped ring. I replaced it locally with a naive single-lock Mutex<VecDeque> queue. That version passed the existing tests and removed all unsafe code from the MPSC module, but it was roughly 1.7–1.9x slower with four producers and 2.3–3.0x slower with eight. A more sophisticated safe implementation might do better, but the straightforward version would clearly regress the existing contention path. It would be useful to capture that tradeoff briefly in the Design Notes.

The description says that the full benchmark suite covers both channel flavors, but the only compare numbers shown are for bounded MPSC. Could we also include the unbounded figures and the machine and OS? On the same machine, the new unbounded backend was about 1.6x faster than the base with one producer and 7.3x faster with eight. Those numbers show the main performance benefit of this change much more clearly than the bounded-only table.

I would be happy to take another look once these comments are addressed.

Comment thread asyncband/src/mpsc/queue.rs Outdated
Comment thread asyncband/src/mpsc/queue.rs
Comment thread asyncband/src/mpsc/queue.rs
@mxsm

mxsm commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

@orthur2 All requested changes are addressed in 13a100e: the legacy Unpin/unwind-safe endpoint traits are restored with the 80-assertion regression matrix, the wrapped receiver-disconnect cleanup path now has an exactly-once drop test exercised by Miri, and the PR description includes the bounded safe-queue tradeoff, unbounded comparison, and benchmark environments. The three review threads are resolved; could you take another look when convenient?

@mxsm
mxsm requested a review from orthur2 September 2, 2026 05:12
@tisonkun tisonkun mentioned this pull request Sep 4, 2026
33 tasks

@orthur2 orthur2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, the three inline comments have all been addressed.

However, the 1 and 8 producer results I gave were just examples. Why use my numbers instead of running your own? We used different machines, so your bounded results and my unbounded results do not form a consistent benchmark set. And why include only those two? The harness reports 1, 2, 4, and 8 producers.

Please refresh the branch against current main before rerunning the benchmarks. It is now many commits behind and conflicts with main branch. And the existing benchmark table no longer represents the implementation currently on main.Please rerun both the bounded and unbounded base/head comparisons on your machine and include all four producer counts.

Comment thread asyncband/src/mpsc/queue.rs Outdated
@mxsm
mxsm requested a review from orthur2 September 5, 2026 00:18

@orthur2 orthur2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for following up. The padding change, and complete benchmark results address my comments.

I use codex to reran both the old and current base/head pairs on my M5. The current unbounded backend is still about 1.4x, 3.1x, 5.2x, and 6.6x faster with 1, 2, 4, and 8 producers. Updating from the old base to current main did not reverse the result on my machine, though that doesn't tell us whether the upstream changes affected your Windows results.
The results clearly differ between our machines. Thanks for reporting the regression and removing the general performance improvement claim.

I have no further code comments. @tisonkun, are you comfortable with the performance tradeoff for this backend replacement?

@tisonkun

tisonkun commented Sep 5, 2026

Copy link
Copy Markdown
Member

Thanks for your contribution @mxsm! And thank @orthur2 for the review.

I'll give a review on Monday.

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.

refactor(mpsc): replace the legacy queue backend

3 participants