refactor(mpsc): replace the legacy queue backend - #247
Conversation
|
OK. I'll review this PR tomorrow, or within the next few days. |
orthur2
left a comment
There was a problem hiding this comment.
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.
|
@orthur2 All requested changes are addressed in 13a100e: the legacy |
orthur2
left a comment
There was a problem hiding this comment.
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.
orthur2
left a comment
There was a problem hiding this comment.
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?
Summary
main(93b3413) and preserve its receiver-waker and semaphore changes.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
Syncimplementation, 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 currentmainat93b3413dfcde09aaca937848ef7c1e815bbbb79e; head is0f039429c1046cf5061f5b2659479dda07075084.Both versions use the unchanged ecosystem MPSC harness: 16,384 messages per sample, bounded capacity 64, and all four producer counts. After
cargo x benchpassed 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.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.cargo x lintstops at Taplo 0.10.0 on unchanged TOML files; the formatting failures also reproduce in a clean worktree of currentmain. The checks after Taplo were run separately using the commands defined bycargo x lint.