Conversation
…ail claim
Peeled from `mikegrier/deferred-namespace-ops`, where it was written alongside
work that is not ready. It is landed on its own because it is an INSTRUMENT: it
is useful before the plan that consumes it exists, and landing it first is what
lets that decision be made against a measurement rather than an argument.
The probe answers two questions a queue-shape decision is waiting on: whether
the bounded array queue's tail claim contends badly enough to justify the linked
and sharded MPSC shapes, and what `reserving_mpsc`'s extra read of the
consumer's position actually costs.
Two regimes, and the pair is the point. **Isolated** gives producers capacity
large enough that nothing is refused and runs no consumer, so whatever curve
appears against N is the claim and nothing else. **Drained** runs a consumer
popping continuously, which is the only regime that can price the read of
`head` -- that read is cheap until a consumer is WRITING the line, so measuring
it in isolation would report it as free. Each row carries the refusal count from
the queue's own `Observable` counters, so a consumer-bound plateau is visible as
a fact rather than mistaken for contention.
**It is deliberately absent from the CI probe job**, unlike every other probe,
and the reason is a measurement rather than a preference: that job runs
`cargo run` without `--release`, and in a debug build `mpsc` and
`reserving_mpsc` come out at 249.7 and 254.0 ns/push at sixteen producers --
indistinguishable. In release, same machine, same minute: 193.5 and 52.2. A
debug run does not merely lose precision, it reports the two shapes as
equivalent, which is a confident wrong answer. It also wants more cores than a
hosted runner has, and costs about a minute against a job whose other probes
are seconds.
Verified by running it rather than by building it: exit 0 in 64.8s on
x86_64 16p/8c, 181 lines, banner and both regimes present. At sixteen producers,
isolated: `baseline_fetch_add` 14.2 ns/push, `reserving_mpsc` 35.0,
`permit_mpsc` 22.2, `slotwise_mpsc` 198.5.
The `experimental-permit-claim` feature is enabled on the dependency because
this probe is what decides its fate -- it has to be measured against the
shipping shapes on the same host, in the same run, by the same harness. `dwcas`
is what lets it instantiate the 128-bit claim layout.
Two design-note corrections were needed on the way in, because the notes had
been written against a state that has since moved:
- The claim-word section said the three apportionments were "built as
duplicates in claim_layout.rs so the shipping crate was not disturbed".
That scaffolding is gone -- the layouts SHIP now, as `ClaimLayout` with
`Balanced`, `Enduring`, `Perpetual` and `Wide`, which is what this probe
imports. A reader who went looking for `claim_layout.rs` would not have
found it. The duplicate-then-decide cycle closed and the note never said so.
- Both sections linked to checklists that do not exist in this repository
(`CHECKLIST-io-domains.md`, `CHECKLIST-claim-word-layout.md`); they arrive
with the rest of the queue work. The notes now name the QUESTIONS rather
than linking to items that would dangle, and say plainly that the plans
carrying them land later.
Every link in the added notes was checked to resolve before writing it.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved benchmark timing, consumer-readiness, interpretation, and documentation issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a standalone Windows queue-contention probe for comparing MPSC queue shapes, reservation costs, and claim layouts.
Changes:
- Adds isolated and drained benchmark regimes with refusal counts.
- Registers the probe binary and enables required queue features.
- Documents methodology, rationale, and recorded measurements.
File summaries
| File | Reviewed change |
|---|---|
crates/windows-platform-probes/src/queue_contention.rs |
Benchmark implementation and timing regimes |
crates/windows-platform-probes/src/lib.rs |
Probe module export and documentation index |
crates/windows-platform-probes/src/bin/queue_contention.rs |
Benchmark report rendering and comparisons |
crates/windows-platform-probes/DESIGN-NOTES.md |
Measurement rationale and results |
crates/windows-platform-probes/Cargo.toml |
Binary registration and dependency features |
Cargo.lock |
Dependency graph update |
Review details
Suppressed comments (6)
crates/windows-platform-probes/DESIGN-NOTES.md:525
- The heading says the wide word costs 2-3x, but the shipping-type measurements below report 3.83x and 3.99x at 16 and 32 producers, and the later section explicitly says those values supersede the earlier stand-in results. Update the heading so it does not understate the result that this note now treats as authoritative.
## The claim word's width costs 2-3x in isolation and much less in use
crates/windows-platform-probes/DESIGN-NOTES.md:530
- This introduction still says there are three apportionments and omits the probe's
8/56(Perpetual) layout. The current probe measures four layouts, and the superseding table below includes 8/56, so the note's setup is inconsistent with the instrument it documents.
Three apportionments of `reserving_mpsc`'s claim word: 32/32 and 16/48 over
`AtomicU64`, and 64/64 over `AtomicU128`.
crates/windows-platform-probes/src/bin/queue_contention.rs:183
cmpxchg16bis x86-specific, but this binary can be built for the repository's other Windows targets andportable-atomicuses target-specific primitives for the wide operation. On ARM64 this line mislabels the instruction being measured. Use architecture-neutral wording or render the instruction conditionally.
" apportioned differently; 64/64 is a u128 exchange (cmpxchg16b)."
crates/windows-platform-probes/src/queue_contention.rs:31
- Removing the consumer and backpressure does not make this "the compare-and-swap and nothing else": each timed push still performs the queue's slot-metadata/publication work, and
reserving_mpscstill readsheadeven when no consumer is running. The resulting curve is total producer-only push-path scaling, so interpreting it as pure tail-CAS contention can misattribute the cost behind the queue-shape decision. Please narrow this description or add a matched control that holds the other push-path work constant.
//! - **Isolated** -- capacity large enough that nothing is ever refused, and no
//! consumer running. This is the *cleanest* measurement of tail-claim
//! contention: nothing else touches the queue, so whatever curve appears
//! against N is the compare-and-swap and nothing else.
crates/windows-platform-probes/src/queue_contention.rs:410
- All participants are released at the same barrier, but that does not make the consumer ready to pop. The scheduler can run producers first, fill the 1024-slot queue, and record refusal/retry cycles before the consumer writes
head; that startup delay is included instarted, so the drained rows are not guaranteed to represent a continuously draining consumer, especially at low producer counts. Add a consumer-readiness handshake before starting the producers and clock, and apply it to each drained helper.
let consumer = thread::spawn(move || {
consumer_gate.wait();
// Spin rather than park: the doorbell's cost is `doorbell_cost`'s
// question, and parking here would measure that instead of the claim.
while !consumer_done.load(Ordering::Relaxed) {
crates/windows-platform-probes/src/queue_contention.rs:281
- The barrier only releases the workers; it does not synchronize their first operation with the timestamp taken afterward. Once
gate.wait()releases everyone, a worker can run (or finish) before the main thread is scheduled to executeInstant::now(), so the elapsed interval is too short and the reported throughput can be inflated, especially for the one-producer rows. This release-then-timestamp pattern is repeated throughout the timed functions; use a ready barrier plus a separate release barrier (or another timestamp handoff) so the timestamp is established before workers enter the loop.
gate.wait();
Instant::now()
- Files reviewed: 5/6 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…features took away Four findings from a code review of this PR, one of them a real regression the PR introduced. **Enabling `dwcas` and `experimental-permit-claim` on a workspace member unifies them across the whole workspace.** `windows-platform-probes` did not depend on `windows-waitable-queues` at all before this branch, so every `--workspace` step built that crate at its default features. With the probe's dependency added, `cargo tree --workspace -e features -i windows-waitable-queues` now reports both features on, sourced solely from here -- and since CI builds that crate only through `--workspace` steps, with no `-p windows-waitable-queues` job anywhere, NOTHING was left compiling it without `dwcas`. That is the configuration that can break unnoticed, because `dwcas` is additive: `Wide` and its `ClaimLayout` impl exist only under it. The queue crate's own manifest calls `dwcas` "non-default so nothing depends on it by accident" and "the only thing in this crate that costs a third-party dependency" -- claims that are only true while something still builds without it. Fixed by adding a `windows-waitable-queues (default features)` job, modelled on the existing `placement-probe-no-serde` job, which exists for exactly this shape of problem. Verified locally before committing: build, clippy `-D warnings`, and 304 + 12 + 1 tests all clean at default features. Not fixed by making the probe's dependency optional: the probe must stay buildable by a plain `cargo build`, and a feature that has to be remembered before it compiles is a worse trade than a job that cannot be forgotten. **The design note's stated reason for keeping the probe out of CI was false**, and it was the load-bearing half. It said the job "runs `cargo run` without `--release`" -- but `probe-doorbell-cost` and `probe-request-cost` already run there WITH `--release`, under a comment establishing the very rule this probe would fall under. It also said "unlike every other probe", and `probe-cancel-io` is likewise absent. And it said the run "costs about two minutes" against the 64.8s this PR's own commit message reports. The decision is unchanged and still right; the argument for it was wrong. Rewritten around what is true -- core count and wall time -- with the release requirement kept as a constraint on HOW it runs rather than as a reason to exclude it. Re-measured: 60.2s. **Two prose defects, both mine and both the same class as ones this PR already fixed elsewhere.** The module doc still named `CHECKLIST-io-domains.md`, a file in no branch -- I swept the design notes for dangling links and did not sweep the source beside them. And three sites named `mpsc`, a module the queue crate does not have; it is `slotwise_mpsc`, which is what the probe measures and labels. That is residue of exactly the rename this probe's own comments record as having caused one silent failure already. Re-ran the probe after changing its output text rather than assuming: exit 0, 60.2s, 181 lines, and the reworded passage renders as intended. Swept every path-shaped `.md`/`.rs` reference in the PR's files for resolvability: zero unresolvable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate findings remain around target support, timing synchronization, and measurement safeguards.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
Previously missed (1) — in code that hasn't changed since the last review.
crates/windows-platform-probes/DESIGN-NOTES.md:537
- The heading and opening count are stale relative to the replacement results later in this same note: the shipping-type section reports 3.83x/3.99x at 16/32 producers and the probe now measures four layouts, including 8/56. Leaving
2-3xandThree apportionmentshere makes the canonical heading present the superseded stand-in result as current; update this opening to the final result or label the old block as historical.
crates/windows-platform-probes/src/bin/queue_contention.rs:16
- The PR's measurement notes state that debug timings make the relevant queue shapes look equivalent, but this entry point accepts a normal debug
cargo runand emits an authoritative-looking report. Add a release-build guard before measuring so a manual invocation cannot produce numbers that are used for the queue decision despite being known-invalid.
fn main() {
crates/windows-platform-probes/src/lib.rs:151
- The crate-level “What each probe establishes” table enumerates the probe functions, but the new
queue_contention::measurebinary-only probe is not added to it. The public crate documentation therefore omits this new probe and its claim, leaving the inventory stale. Add a row for it alongside the other binary-only probes.
pub mod queue_contention;
crates/windows-platform-probes/src/queue_contention.rs:245
- This warm-up calls
timer, but every timer call allocates and drops its own queue. The untimed pass therefore does not pre-touch the backing storage used by any timed repetition; first-use page faults or initialization can still occur afterInstant::now(), contrary to the comment and making the reported time depend on allocator/page reuse. Reuse the allocation being timed or explicitly pre-touch that allocation before starting the clock.
// One untimed pass first: the first touch of a fresh allocation faults
// pages in, and that cost belongs to the allocator rather than the queue.
let _ = timer(producers);
let mut results: Vec<Repetition> = (0..REPETITIONS).map(|_| timer(producers)).collect();
crates/windows-platform-probes/src/queue_contention.rs:406
- This barrier only proves that the consumer reached
wait(), not that it has entered the drain loop. After release, the scheduler can run producers long enough to fill the 1024-slot queue before the consumer executespop; that startup delay and its refusals are then included in the supposedly continuously-drained timing. Add a consumer-ready handshake before releasing and timing the producers, and apply the same synchronization to the other drained variants.
let gate = start_barrier(producers + 1);
- Files reviewed: 6/7 changed files
- Comments generated: 2
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
It has unresolved portability, timing-correctness, documentation, and rationale issues.
Review details
Suppressed comments (7)
.github/workflows/ci.yml:510
- The PR description still says the probe is excluded because the CI probe job runs
cargo runwithout--release, but this workflow already runs the timing probes with--releaseat lines 317-324. The new design note also identifies that premise as false; please update the description to the actual reasons for exclusion (host core count and runtime) so the stated rationale matches the workflow.
# **This job exists because adding `probe-queue-contention` took the default
# configuration away from every other job.** That probe needs `dwcas` and
# `experimental-permit-claim`, and enabling them on a workspace member
# unifies them across the whole workspace -- so the `--workspace` steps that
# deliberately omit `--all-features` stopped being a default-features build
crates/windows-platform-probes/Cargo.toml:158
- This normal dependency enables
dwcasfor every Windows build ofwindows-platform-probes, soreserving_mpsc::Wideandportable_atomic::AtomicU128are compiled even when the probe is built for a target without native double-width CAS. The queue crate's owndwcasdocumentation records thatAtomicU128is unavailable on i686 withdefault-features = false, so a normalcargo build -p windows-platform-probesloses the Windows-wide buildability this crate otherwise has. Keep the wide experiment x64-only (for example, target-gate the probe/dependency or split the binary) while leaving the ordinary probe crate buildable on other Windows targets.
windows-waitable-queues = { path = "../windows-waitable-queues", features = [
"experimental-permit-claim",
# So the probe can instantiate the 128-bit layout. The 64-bit ones need no
# feature; this is the only one that costs the queue crate a dependency.
"dwcas",
crates/windows-platform-probes/DESIGN-NOTES.md:650
CW-1.6is still an unresolvable work-item reference: it has no definition elsewhere in this checkout, while the preceding section explicitly says the queue checklists are not present yet. This leaves the provenance of the deletion unavailable to readers; either link the eventual checklist when it lands or state the current fact without the missing item ID.
`CW-1.6` deleted the duplicated protocol in this crate once
`windows-waitable-queues` took the layout as a parameter, so the probe now
instantiates the real type at each layout. The numbers below supersede the ones
above, which were taken from the stand-in.
crates/windows-platform-probes/DESIGN-NOTES.md:533
- The heading still presents the old stand-in result as the current decision, even though the later section says the shipping-type measurements supersede it and reports a 3.83-3.99x wide-word cost at 16-32 producers. Mark this section explicitly as historical (or update/remove the stale title), otherwise readers can stop here and carry forward the obsolete 2-3x conclusion.
## The claim word's width costs 2-3x in isolation and much less in use
crates/windows-platform-probes/src/bin/queue_contention.rs:186
- The report hard-codes
cmpxchg16b, butWideis backed byportable-atomicand this binary is not restricted to x86_64. Onaarch64-pc-windows-msvcthe same 128-bit operation uses the ARM atomic sequence (ldxp/stxp), so running the probe there would attach the wrong instruction to the measurement. Make the description target-dependent or describe it generically instead of presenting an x86-specific name as universal.
" apportioned differently; 64/64 is a u128 exchange (cmpxchg16b)."
crates/windows-platform-probes/src/lib.rs:151
- The crate-level
# What each probe establishestable is the inventory for this crate and currently ends atrequest_cost::measure; adding the publicqueue_contentionmodule without a row leaves the new instrument undiscoverable in the API documentation and omits its binary-only/release-only constraints. Add aqueue_contention::measureentry describing the two measured regimes.
pub mod queue_contention;
crates/windows-platform-probes/src/queue_contention.rs:308
Barrier::wait()only releases the parties; it does not make the followingInstant::now()atomic with that release. Every caller returns the workers from this barrier and only then takes the timestamp, so a worker can execute an arbitrary prefix of its loop before timing starts, biasing the producer-count curves (especially when the main thread is descheduled). Use a second start barrier/flag so the timestamp is established before workers can enter the timed loop.
/// The count includes this thread: the workers arrive and block, this thread
/// arrives last, and the clock starts as the barrier releases them together.
fn start_barrier(participants: usize) -> Arc<Barrier> {
Arc::new(Barrier::new(participants + 1))
- Files reviewed: 6/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
… a spin on a dead queue
Three findings from a second review of this PR. The first changes the numbers.
**The timing window was wrong at both ends, and the error was not small.** Every
timer released a barrier, called `Instant::now()` on THIS thread, and read
`elapsed()` after `thread::scope` returned:
- `Barrier::wait` releases every party together and this thread is just
another party, so a worker could return from `wait` and run an arbitrary
prefix of its pushes before this thread was scheduled again to read the
clock. That understates the interval, which OVERSTATES throughput -- and it
bites hardest at high producer counts, where this thread is competing with
N busy workers for a core. Which is exactly where the curve is the finding.
- `thread::scope` joins before it returns, so thread exit and join sat inside
the measured interval. That overstates it, and dominates at one producer
where a whole run is only hundreds of microseconds.
Each worker now timestamps itself either side of its own pushes, and the span is
the earliest start to the latest finish. A previous review had explicitly
cleared this, concluding the clock "genuinely starts at release"; it does not.
**Measured, with a control, because a benchmark correction that is not measured
is just a rewrite.** On x86_64 16p/8c, `reserving_mpsc` at sixteen producers:
35.0 ns/push before, 49.8-52.8 after. Two runs of the same build put the
run-to-run spread at 2-6%, so this is signal: the probe was reporting roughly
45% more throughput than the producers actually achieved, at the producer counts
the decision turns on. The one-producer rows moved the other way and slightly
(2.4 -> 2.3, 6.5 -> 6.0), which is the join overhead leaving the window.
The four figures recorded in DESIGN-NOTES predate this and are now marked as
such rather than quietly left standing. The qualitative finding they support --
a debug build swamps the effect -- is unaffected.
**A dead queue was retried forever.** The drained producers looped on every
`PushError`, but only `Full` is retryable; the queue crate's own documentation
says "retrying the first is sensible and retrying the second is a spin". If the
consumer panicked and dropped the receiver, every producer would spin
indefinitely, and because the consumer is joined only after the producer scope
completes, the panic could never surface -- the probe would hang rather than
fail. All four loops now assert `is_retryable()` first. Joining the producers
inside the scope, which the timing fix required anyway, means a producer panic
now surfaces too.
**The isolated regime does not isolate the compare-and-swap**, and both the
module doc and the design note said it did: "whatever curve appears against N is
the compare-and-swap and nothing else". What is timed is each shape's whole push
path -- tail claim, slot-sequence load, item write, publication store, doorbell
fence -- and `permit_mpsc` takes two shared read-modify-writes where the others
take one. A difference here is a difference in push cost. Narrowed to that.
Gate: fmt, clippy --all-targets, 236 lib + 12 integration, and the probe run end
to end (exit 0, 63.6s and 64.8s across two runs).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Moderate issues remain around target support, drained-regime synchronization, report validation, and platform-correct output.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (9)
crates/windows-platform-probes/Cargo.toml:158
- Enabling
dwcasunconditionally makes this package fail to build fori686-pc-windows-msvc:queue_contention.rsimports and instantiatesWide, but the queue crate'sAtomicU128implementation is unavailable on i686. The queue crate explicitly preserves i686 support, and this probe is not target-gated, so a workspace check for that supported target now breaks. Gate the wide rows and dependency feature on targets with 128-bit atomics, or explicitly exclude this probe from unsupported targets.
windows-waitable-queues = { path = "../windows-waitable-queues", features = [
"experimental-permit-claim",
# So the probe can instantiate the 128-bit layout. The 64-bit ones need no
# feature; this is the only one that costs the queue crate a dependency.
"dwcas",
crates/windows-platform-probes/DESIGN-NOTES.md:545
- This heading is stale relative to the re-measured shipping-type results later in the same section: lines 669-670 report 3.83x and 3.99x at 16 and 32 producers, and explicitly supersede the earlier 2.37x/2.99x figures. Leaving "2-3x" in the heading makes the current design note contradict its own authoritative table.
## The claim word's width costs 2-3x in isolation and much less in use
crates/windows-platform-probes/DESIGN-NOTES.md:545
- The later "Re-measured on the shipping type" section explicitly says that its numbers supersede this section, but this heading has no adjacent superseded-status marker. Add the required marker immediately below the heading so readers do not mistake the earlier 2-3x table and conclusions for the current measurement.
## The claim word's width costs 2-3x in isolation and much less in use
crates/windows-platform-probes/src/bin/queue_contention.rs:186
- The report hard-codes
cmpxchg16b, but this binary is not x86_64-only; the sameWidelayout is available on ARM64, where the exchange usesldxp/stxp. Running the probe there therefore emits an incorrect instruction description. Use a target-neutral description or select the instruction name with target-specific code.
" apportioned differently; 64/64 is a u128 exchange (cmpxchg16b)."
crates/windows-platform-probes/src/bin/queue_contention.rs:114
- These lookups intentionally turn a missing row into
Noneand then into--, so a wiring regression can produce a plausible report with an absent column. The new probe has no deterministic test for the expected shape/producer matrix or ratio rows, even though its module documentation calls out this exact rename failure mode. Add a syntheticObservation/renderer test in a sibling test module that asserts every expected row is present.
let plain = observation.find(&observation.drained, shapes::SLOTWISE_MPSC, producers);
let reserving = observation.find(&observation.drained, shapes::RESERVING_MPSC, producers);
let permit = observation.find(&observation.drained, shapes::PERMIT_MPSC, producers);
let ratio = format_ratio(reserving, plain);
crates/windows-platform-probes/src/lib.rs:151
- The crate-level
What each probe establishesinventory has no entry for the newly registeredqueue_contentionmodule. Because this probe is binary-only and deliberately omitted from CI, leaving it out makes the public crate documentation an incomplete inventory and hides the claim this instrument supports. Add aqueue_contentionrow with its binary-only tier and measurement claim.
pub mod queue_contention;
crates/windows-platform-probes/src/queue_contention.rs:558
- This second drained setup has the same readiness gap: the barrier releases the consumer and producers together, but does not ensure the consumer has executed a pop before producer timers begin. Consequently this row can measure startup scheduling as well as the intended head contention. Use an explicit consumer-ready handshake before starting the producers.
let gate = start_barrier(producers + 1);
let consumer_gate = Arc::clone(&gate);
let consumer = thread::spawn(move || {
consumer_gate.wait();
while !consumer_done.load(Ordering::Relaxed) {
crates/windows-platform-probes/src/queue_contention.rs:623
- The barrier does not establish that the permit consumer is actually draining when the producers start; it only establishes that all parties reached the barrier. A producer can run and fill/refuse against the queue before the consumer's first
pop, so the measured cost includes an uncontrolled startup phase. Add a consumer-ready handshake before the timed producer work.
let gate = start_barrier(producers + 1);
let consumer_gate = Arc::clone(&gate);
let consumer = thread::spawn(move || {
consumer_gate.wait();
while !consumer_done.load(Ordering::Relaxed) {
crates/windows-platform-probes/src/queue_contention.rs:731
- This layout-specific drained row also releases the consumer and producers simultaneously, without proving that the consumer has started its drain loop. The resulting initial backlog/refusals depend on scheduling and can bias the layout comparison. Gate producer timing on an explicit consumer-ready signal instead of using this barrier as a readiness guarantee.
let gate = start_barrier(producers + 1);
let consumer_gate = Arc::clone(&gate);
let consumer = thread::spawn(move || {
consumer_gate.wait();
while !consumer_done.load(Ordering::Relaxed) {
- Files reviewed: 6/7 changed files
- Comments generated: 2
- Review effort level: Lite
The queue-contention section said the u64 re-apportionments "track the default within noise", so twenty years of counter headroom was "free". The table directly beneath it showed 1.21x and 1.13x against a noise floor the same file put at 2-6% -- the prose contradicted its own evidence in adjacent lines. Re-measured seven times on x86_64 16p/8c after the timing correction in d49a71f, which the original figures predate. Two findings: - The 2-6% floor was obtained by comparing two runs, which cannot measure a spread. Seven runs put the same-configuration spread at 7-61%. - The probe already emits its own control: `reserving_mpsc` and `reserving(32/32)` are the same code at the same layout, measured twice per run, so their ratio is an empirical "no difference" -- 0.68-1.27x. That is a derived control rather than an asserted floor. Against that control the 128-bit word separates decisively (3.45x/3.81x at 16/32 producers) and the u64 re-apportionments do not. The claim is withdrawn in BOTH directions rather than inverted: 1.23-1.30x against a control reaching 1.12x, on one host, is a flag to measure locally, not a cost. Swept the class rather than the reported line: the same claim appeared three times (the stand-in section, the headroom section's "at no measured cost", and the re-measured section). All three corrected. Records D-observations-not-verdicts: figures are published with their capture parameters, and fine-grained layout choices belong to the client. Windows exposes no NUMA distance table, so the processor-to-node assignment in the banner is the analog used in its place; the measurement host's `numa[16]` is a single domain, so these figures say nothing about cross-domain behaviour. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…dance The withdrawn claim had propagated out of the probe's design note into this crate's consumer-facing documentation, where it told callers that `Perpetual` buys twenty years of headroom "at no measured cost". Three restatements plus the decision they derive from: - README.md, the "What to do about it" bullet - src/lib.rs, the same text as public rustdoc on docs.rs - README.md, the "Start here" chooser - DESIGN-NOTES.md D-41, "measured indistinguishable outside noise" The recurrence arithmetic is unaffected -- time-to-wrap follows from the field width and a rate, not from a throughput measurement -- so the recommendation stands. What is removed is the unsupported claim about what it costs to run. Re-measured, the deeper layouts are indistinguishable from `Balanced` up to eight producers and near 1.26x at sixteen and thirty-two, against a same-code control reaching 1.12x, on a single host. This is CONTRACT INTEGRITY rule 3: the reported site was a sample, not the population, and a correction to a shipped rule obliges a re-check of what was written against the old one. Verified: 13 doctests pass, including the compiled README. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate findings affect measurement reporting, probe discoverability, and helper-test coverage.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (6)
crates/windows-platform-probes/DESIGN-NOTES.md:782
- The table immediately above is unlabeled, but it contains the 64/64 ratios 1.37x, 3.45x, and 3.81x at 1, 16, and 32 producers, while this sentence says the drained 128-bit word stays inside the control band. As written, the note either contradicts its table or omits the drained table needed to support the claim; label the table's regime and correct the conclusion to match the actual measurements.
In the drained regime nothing separates at all -- every u64 layout *and* the
128-bit word sit inside the control band at every producer count (the widest
median is 1.13x at one producer, against a control that reaches 1.27x).
crates/windows-platform-probes/DESIGN-NOTES.md:746
- These capture details are not emitted by the new binary: it runs five repetitions and reports only the median, does not retain or print the observed range, and prints no release/debug marker. Because this probe is intentionally manual rather than CI-gated, stdout is the measurement record; add those fields to the observation/report or revise this promise before relying on the ratios.
Seven runs, median of the per-run ratios with the observed range beside it,
release build. **The banner's `numa[16]` is load-bearing here: it means a single
NUMA node holding all sixteen processors**, so every figure below was taken
crates/windows-platform-probes/src/bin/queue_contention.rs:50
- This label duplicates
DRAINED_CAPACITYfrom the measurement module. If the benchmark capacity changes, execution will use the new value while the captured report still sayscapacity 1024, making the observation self-contradictory; share/export the constant (or expose it through the observation) and interpolate it here.
"\n-- drained: a consumer popping continuously, capacity 1024 --"
crates/windows-platform-probes/src/bin/queue_contention.rs:57
- This heading overstates what the measurement isolates.
scalingis computed fromRun.pushes_per_second, and each timed run includes slot-sequence loads, the item write, publication, and the doorbell fence, as the module documentation notes; it is not a tail-CAS-only measurement. Rename the heading to describe producer push-path scaling/contended push cost so readers do not attribute the whole curve to the tail claim.
let _ = writeln!(out, " 1. tail-claim contention (isolated regime)\n");
crates/windows-platform-probes/src/lib.rs:151
- Please add
queue_contention::measureto theWhat each probe establishestable above. That table is the crate's canonical inventory of probe tiers and claims (src/lib.rs:110-136), but the new module is currently only exported below, so this binary-only probe is undiscoverable in the documented inventory.
pub mod queue_contention;
crates/windows-platform-probes/src/queue_contention.rs:167
- The new pure observation helpers have no tests: a missing shape/producer lookup silently becomes
--, and an incorrectscalingcalculation would directly change the conclusions printed by this probe. Add sibling unit tests forfindandscaling(including missing and one-producer cases) without invoking the long host measurement; the repository already keeps focused sibling tests for comparable probe logic.
pub fn scaling(&self, regime: &[Run], shape: &str, producers: usize) -> Option<f64> {
let one = self.find(regime, shape, 1)?;
let many = self.find(regime, shape, producers)?;
Some(many.pushes_per_second / one.pushes_per_second)
- Files reviewed: 10/11 changed files
- Comments generated: 2
- Review effort level: Lite
…not a wider ruler The queue-contention repair derived a noise control from the probe's own output -- `reserving_mpsc` and `reserving(32/32)` are the same code at the same layout, measured twice per run -- and immediately used it to withdraw an unsupported claim. But it then used it only as a yardstick, which quietly promoted a symptom into a tool. Two measurements of identical code in the same run differing by 27%, with same-configuration spread reaching 61%, is first a finding about the METHOD. Recording it as merely a coarser ruler and carrying on is how a methodological problem becomes permanent. Records D-variance-is-a-finding. The candidate causes are not separable from the dispersion itself, so the note refuses to guess among them: wrong instrument for the variable, a residual probe defect (one was already found and fixed in d49a71f, invisible in the numbers and worth ~45%), too few runs or too short a span, or a nanosecond-scale measurement on a shared loaded desktop. The calibration is recorded rather than assumed, because it cuts both ways: a spread this wide would be disqualifying in a benchmark or marketing document, which exist to carry a comparative claim. These figures exist to support planning for deployment environments resembling the measured one -- an ordinary machine under ordinary load -- so the treatment is neither suppression nor promotion. Publish the data, publish the dispersion, and publish that the dispersion is unexplained. Queues M4.2 as a diagnosis item rather than leaving the work in a design note, per "design notes are not a work queue". It names a negative result as a valid outcome that must still be recorded, and forbids resolving it by suppressing the ranges. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved moderate and nit findings remain in probe reporting, inventory, and documentation.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (8)
Previously missed (3) — in code that hasn't changed since the last review.
crates/windows-platform-probes/src/bin/queue_contention.rs:32
- This report includes a host banner, but
fingerprint::banner_line()formats only host identity and does not say whether the binary is debug or release. The probe's documentation shows that debug and release can make the queue shapes look equivalent, so a captured report cannot establish whether its figures were produced under the required profile. Add an explicit build-profile line before the measurements.
crates/windows-waitable-queues/README.md:185 - The earlier README paragraph at lines 142-147 still says deeper positions cost nothing measurable, while this later changed paragraph correctly says the cost is unknown. A reader of this document gets both conclusions; update the earlier statement and the matching API/manifest docs too.
crates/windows-waitable-queues/src/lib.rs:155 - This new bullet says the throughput cost is not established, but the preceding public module docs at lines 110-116 still say that a deeper position costs nothing measurable; the same stale claim remains in
reserving_mpsc.rs:186-188andCargo.toml:60-62. That leaves the published API guidance contradictory, so update all of those restatements in the same change.
crates/windows-platform-probes/DESIGN-NOTES.md:620
- The PR description still says the existing probe job runs
cargo runwithout--release, but the workflow already runs the nanosecond probes in release at.github/workflows/ci.yml:317-324. This paragraph correctly attributes exclusion to core count and runtime; align the PR description with that reason so release is not reported as the job-wide blocker.
**That is a constraint on HOW it runs, not an argument for keeping it out**, and an earlier draft of this
paragraph confused the two -- it said the CI job "runs `cargo run` without `--release`", which is not true
of the job it describes: `probe-doorbell-cost` and `probe-request-cost` already run there with `--release`,
under a comment establishing exactly the rule this probe would fall under. It also said "unlike every other
probe", and `probe-cancel-io` is likewise absent. Corrected by a review. The release precedent exists; what
keeps this one out is that it costs an order of magnitude more than the two probes that use it, on hardware
that cannot answer the question anyway.
crates/windows-platform-probes/DESIGN-NOTES.md:688
- This subsection still presents
cmpxchg16bas 2-3x in isolation and 5-12% in the drained regime, but the shipping-type remeasurement below reports 3.45-3.81x isolated at 16/32 producers and says all layouts fall within the control band when drained. The later section says it supersedes these numbers; mark this heading/conclusion as historical or move it to the rationale so the current design note does not expose two incompatible conclusions.
**Widening the word is not free, and how much it costs depends entirely on the
regime.** Isolated, where the claim is the only thing happening, `cmpxchg16b`
costs 2-3x and the penalty *grows* with contention. Drained, with a consumer
running, it is 5-12%. This is the one conclusion in this section that the
seven-run re-measurement strengthened rather than withdrew.
crates/windows-platform-probes/src/bin/queue_contention.rs:50
DRAINED_CAPACITYis defined as 1024 in the library, but this report repeats 1024 as a string literal. If the benchmark capacity changes, the output will describe the wrong regime, which is especially harmful because refusal counts are interpreted relative to this capacity. Render the heading from the shared constant instead.
"\n-- drained: a consumer popping continuously, capacity 1024 --"
crates/windows-platform-probes/src/lib.rs:151
- The module-level "What each probe establishes" table at
src/lib.rs:110-136is the crate's exhaustive probe inventory, but this addition only declaresqueue_contentionand adds no row forprobe-queue-contention. Add its binary-only tier and supported claim there so the new instrument is discoverable and the inventory remains complete.
pub mod queue_contention;
crates/windows-platform-probes/src/queue_contention.rs:11
- These module docs say the probe exists to force two decisions, but the implementation and renderer also measure and report a third decision about claim-word layout (the output's "Question 3"). Update the module-level contract to mention that comparison, or explain why it is subordinate to one of the two decisions, so users can reconcile the documented scope with the report.
//! # The two decisions this exists to force
//!
//! **1. Are the linked and sharded MPSC shapes needed at all?** They are parked
- Files reviewed: 11/12 changed files
- Comments generated: 1
- Review effort level: Lite
…ere the floor is Extends D-variance-is-a-finding with the practical half, and re-scopes M4.2 from "diagnose the variance" to "give the probes the controls that make diagnosing it possible" -- the judgement stays with the person, the probe stops being the obstacle. What the note now says: - Gather more of the same before gathering anything different. Lengthening the span and raising the repetition count is the only step that changes nothing about what is measured, so it is the only one whose result is interpretable before the others have been tried. Pinning, quiescing, or altering the probe all move the measurement as well as the noise. - A warmup separates transient cost from ongoing noise. Recorded with the objection it invites: if noise were inherent, warming could not remove it. It does not -- warming removes front-loaded transients (page faults, cold caches, frequency ramp) while tenant contention runs for the whole measurement. So warming uncovers the inherent floor rather than hiding it, and the expected signature is a spread that narrows and then plateaus. - There is always a floor, and recognising it is the skill. Past it more runs buy nothing. - The floor is NOT necessarily a fraction of the measured value. It can be set by the sampling regime -- clock granularity, independent sample count, how the span is built. This probe takes two timestamps per worker per repetition, so what limits resolution at small values is the pass count, not a percentage of the nanoseconds printed. The two readings prescribe opposite actions, which is why "small numbers are just noisy" is a guess and not a diagnosis. M4.2's software scope, verified against the source: PUSHES_PER_PRODUCER (50_000) and REPETITIONS (5) are private consts and measure() takes no arguments, so the prescribed first move currently requires a source edit and a rebuild. Defaults must not change, and whatever settings a run used have to be reported beside the host banner -- they are capture parameters now, per D-observations-not-verdicts. Note the warmup is partly present already: one untimed pass exists, but only to fault in the allocation's pages. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…meters This PR added D-observations-not-verdicts, which requires every published figure to carry the parameters of its capture, and then published figures missing two of them. The block recorded the host banner, the build profile, and "seven runs" -- but not what a run consists of. Each run is a whole probe invocation, within which every configuration is measured REPETITIONS (5) times and the median reported, each measurement being PUSHES_PER_PRODUCER (50,000) pushes per producer thread, preceded by one untimed pass that faults in the fresh allocation's pages. So each published figure rests on 35 timed passes per configuration, and "seven runs" alone does not let anyone reproduce it. Also names where the two constants live and points at M4.2, which makes them adjustable -- the note now prescribes lengthening the span and raising the repetition count as the first response to a wide control, and the probe cannot currently do either without a source edit. Recording the gap is what this PR owes; closing it is follow-up. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved documentation issues overstate probe conclusions, publish pre-correction measurements, and omit required layout guidance.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (5)
crates/windows-platform-probes/src/bin/queue_contention.rs:11
- The executable's module docs still say it decides which shapes are needed and whether they should merge, while the report itself correctly says the drained ratio cannot isolate or bound the
head-load cost. Together withD-no-client-prescriptions, this makes the binary promise a design conclusion that its measurements do not support; describe it as reporting observations instead.
//! This decides two things that are otherwise decided by taste: whether the
//! linked and sharded MPSC shapes are ever needed, and whether `slotwise_mpsc`
//! and `reserving_mpsc` should merge. See `queue_contention`'s module docs.
crates/windows-platform-probes/src/queue_contention.rs:30
- This section still presents the probe as deciding that a cheap
headread means the shapes merge and an expensive one vindicates the split, but the regime note below says the drained ratio cannot isolate or even bound that read because the shapes also differ in unordered protocol, metadata, and retry costs. That overstates what this instrument can establish and conflicts withD-no-client-prescriptions; describe the result as an end-to-end comparison and leave the merge decision to the consumer/design work.
//! **2. Should `slotwise_mpsc` and `reserving_mpsc` merge?** They ship as peers because
//! honouring a reservation costs the producer a read of the consumer's
//! position -- one line every thread touches -- and *how much* that costs was a
//! judgement rather than a measurement. If it is cheap, the two shapes merge and
//! the non-reserving one goes; if it is expensive, the split is vindicated.
crates/windows-platform-probes/src/queue_contention.rs:73
- This summary calls the drained rows a measurement of the
headcost, but the preceding regime contract explicitly says the ratio neither isolates nor bounds that load. Calling it a cost measurement here reintroduces the claim that the report just withdrew; describe it as an end-to-end comparison whileheadis being written.
//! rather than mistaken for contention. Read the isolated regime for the
//! contention question, and the drained one for the cost of `head`.
crates/windows-waitable-queues/README.md:368
- These new lines frame the following three-run table as current measurements, but its x64 16-producer values (193.5/52.2) are the exact figures the probe note identifies as predating the corrected timing window. Leaving them unqualified makes the published README present a known optimistic result as current; either retake this table with the corrected timer or label it as historical/pre-correction data.
The measurements below are what this workspace observed on the hosts named; they
are not a ranking, and which shape suits a given deployment is the deployment's
question.
crates/windows-waitable-queues/src/lib.rs:288
- These new lines frame the following three-run table as current measurements, but its x64 16-producer values (193.5/52.2) are the exact figures the probe note identifies as predating the corrected timing window. Leaving them unqualified makes the public rustdoc publish a known optimistic result as current; either retake this table with the corrected timer or label it as historical/pre-correction data.
//! The measurements below are what this workspace observed on the hosts named;
//! they are not a ranking.
- Files reviewed: 14/15 changed files
- Comments generated: 3
- Review effort level: Lite
| The default remains `Balanced` so that no existing caller's behaviour changed | ||
| when the choice was introduced. It is not the recommended layout. | ||
| when the choice was introduced. It carries the recurrence described above. |
| //! The default remains `Balanced` so that no existing caller's behaviour | ||
| //! changed when the choice was introduced. It is not the recommended layout. | ||
| //! changed when the choice was introduced. It carries the recurrence above. |
| /// because it is what the shape shipped with: the reservation ceiling it buys | ||
| /// is far beyond any use this crate has seen, and it is paid for with the whole | ||
| /// of the `SH-14.1` exposure. [`Enduring`] and [`Perpetual`] trade ceiling for | ||
| /// recurrence in the other direction; [`Enduring`]'s ceiling is 65,535. |
…ecommending against it **The README and public rustdoc were publishing known-optimistic numbers.** The three-run comparison table's x64 sixteen-producer row is 193.5 / 52.2 -- the exact pair the probe's note flags as predating the correction to the timing window. The sentence added in 0ad7465 then framed the table as "what this workspace observed", with no caveat, which made it worse. Both copies are now labelled: the direction survived re-measurement on the x64 host, the absolute numbers are optimistic, and the high-producer rows most so. Not retaken, because retaking needs the EPYC 7763 and Snapdragon X2 Elite named beneath the table and the correction was measured on neither. **Three sites still had the probe deciding design questions.** The binary's module doc said it "decides two things that are otherwise decided by taste"; the library module said a cheap `head` read means the shapes merge and an expensive one vindicates the split; the regime summary said to read the drained rows "for the cost of `head`". The same file says two paragraphs later that the drained ratio can neither isolate nor bound that read. All three now describe what the regimes report and say plainly that the merge question is an input to a decision rather than the decision. **On the review's request to restore "Perpetual is the practical recommendation" and "Balanced is not recommended": declined, but the concern behind it was real.** A recommendation is exactly what D-no-client-prescriptions removes. The legitimate half is that "carries the recurrence described above" was too weak to keep the hazardous default from looking endorsed by inertia -- a skimming reader does not connect "recurrence" to losing data. The answer is a stronger fact, not a restored preference: the default's documentation now says that past 2^32 pushes from two or more producers the queue can **silently lose an item**, in all four places it is described. That is more informative than "not recommended" and carries no judgement about anyone's deployment. D-41's clause is updated to match what the documentation now says, and records why the form changed. Verified: fmt and clippy clean; 13 doctests including the compiled README. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The unresolved feature-gated rustdoc link and specified report/documentation corrections must be addressed.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (4)
crates/windows-platform-probes/src/bin/queue_contention.rs:355
- This output still says the two same-code rows “should agree within noise”, but the probe's recorded control spans 0.68-1.27x and the design note explicitly treats that width as an unresolved finding about the instrument. That wording normalizes the variance the report is supposed to expose; describe the rows as the same-code control and report any disagreement as observed dispersion instead of presuming it is noise.
" configuration run twice, so they should agree within noise. They"
crates/windows-platform-probes/src/bin/queue_contention.rs:67
- The new capture contract requires each published figure to record its build profile, but this report only emits a warning when
debug_assertionsis enabled; a required release run has no explicit profile field. A captured report therefore cannot prove it was produced with--release, even though the debug path is documented as a confidently wrong measurement. Emit an explicit profile value alongside the sampling parameters.
if cfg!(debug_assertions) {
crates/windows-waitable-queues/DESIGN-NOTES.md:78
- D-41 now withdraws the measured no-cost claim, but this still says that re-apportioning the narrow word “removes the exposure for free.” That leaves a second statement that readers can reasonably take as a throughput claim, contradicting the new conclusion that the cost is unestablished; please limit this wording to the absence of a third-party dependency and state that throughput remains unestablished.
| <a id="d-41"></a>D-41 | **The claim word's apportionment is a caller's choice, and the recurrence behind SH-14.1 is a number the caller sets rather than one this crate imposes.** Supersedes [D-36](#d-36), whose premise was that the only fix was the [D-35](#d-35) claim-protocol replacement, gated on an open question -- so disclosing beat delaying. That premise was false, and measurement is what showed it: the 32/32 split followed from requiring the reservation half to hold the *entire capacity*, because every slot could be reserved at once. Capping outstanding reservations instead leaves the capacity bounded only by the ring, and the position is free to take 48 or 56 bits. `Balanced` (32/32), `Enduring` (16/48) and `Perpetual` (8/56) issue the **same** `lock cmpxchg` on the same `u64`, differing only in shift constants -- so the recurrence moves from about 37 seconds to about 20 years without a dependency. **An earlier version of this row added "and measured indistinguishable outside noise", and that clause is withdrawn**: it rested on a single probe run read against a noise floor of 2-6% that seven runs put at 7-61%. Re-measured, the deeper layouts are indistinguishable from `Balanced` at low producer counts, and at high counts differ by less than the run-to-run variation of the same code measured twice -- which is one host declining to call it, not a cost. The figures are in the probe's note rather than restated here. The layout is the caller's choice and the throughput question belongs on the caller's hardware; see the queue-contention section of [DESIGN-NOTES.md](../windows-platform-probes/DESIGN-NOTES.md#d-queue-layout-observations). The reservation half was the wrong half to spend bits on: it held 2^32 where the real bound is however many producers are mid-send. `Wide` (64/64 over a `u128`) is offered behind the non-default `dwcas` feature, because it is the one thing here that costs a third-party crate -- the standard library has no 128-bit atomic -- and choosing it measured slower on the whole push path as producer count rises in the isolated regime -- near parity at one or two, several times by thirty-two -- while falling inside the same-code control when drained. The probe times the complete push, so that is the layout's effect on that path rather than a measurement of the exchange alone; it buys a guarantee rather than a lifetime argument. **The default stays `Balanced`** so introducing the choice changed no existing caller's behaviour, and its documentation states the defect outright -- that past 2^32 pushes from two or more producers the queue can silently lose an item -- rather than leaving the status quo to look safe by inertia. It states the defect rather than recommending against the layout, per [D-no-client-prescriptions](../windows-platform-probes/DESIGN-NOTES.md#d-no-client-prescriptions): a caller told what the layout does can decide; a caller told what to prefer has been handed our judgement about their deployment. |
crates/windows-waitable-queues/README.md:222
- This still gives the reader an imperative recommendation (
Take it when you want...), contradicting the newD-no-client-prescriptionsrule incrates/windows-platform-probes/DESIGN-NOTES.md:61-83, which explicitly says measurement documentation must stop at observations rather than tell callers what to choose. Replace this with a neutral statement thatWideremoves the recurrence while adding the measured push-path cost and dependency; leave the deployment tradeoff to the caller.
two, several times by thirty-two, in the isolated regime. Take
it when you want the recurrence gone as a guarantee rather than deferred by an
argument about deployment lifetimes.
- Files reviewed: 14/15 changed files
- Comments generated: 1
- Review effort level: Lite
…d three more claims Five findings, all procedural. **A broken intra-doc link, introduced in 9c5f3e0.** `[`Wide`]` was added to `Balanced`'s rustdoc, but `Wide` is `#[cfg(feature = "dwcas")]` and `Balanced` is always built, so a default-feature rustdoc build emits `unresolved link to Wide`. Confirmed by running `cargo doc -p windows-waitable-queues --lib --no-deps` with default features, which reproduced it at the exact line, and clean afterwards. Nothing caught it: the new default-feature CI job does not run rustdoc, and the workspace docs job uses `--all-features`. Now a non-linking code reference that says why it does not link. **The report could not show it was a release build.** The capture contract requires the build profile, and the report only warned when `debug_assertions` was set -- so a captured release report carried no positive evidence of its profile, only the absence of a warning. It now prints `profile: release` (or `debug -- NOT A MEASUREMENT`) beside the sampling parameters. **The report told readers to discount its own control.** It said the two same-code rows "should agree within noise". They span 0.68-1.27x across seven runs, and D-variance-is-a-finding treats that width as an unresolved finding about the instrument -- so calling it noise normalises exactly what the line exists to expose. I cleared this in the previous round on the grounds that it described the control rather than advising a reader; that was the wrong reading. It now says the gap is dispersion, and that its width is an open question. **Two more restatements of withdrawn claims.** `D-37` still said re-apportioning the narrow word "removes the exposure for free"; the README still said "Take it when you want the recurrence gone", an imperative that survived the D-no-client-prescriptions sweep two commits ago. Also, found by reading the rendered report rather than by review: the report's own title still asked "does the array queue's tail claim contend?" -- the attribution corrected in the module doc last round and missed here. It now names push-path scaling, matching the table beneath it. Verified: default-feature and all-features rustdoc clean; fmt and clippy clean; 252 lib + 12 integration tests; release run against a binary confirmed newer than source. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved target-feature portability and measurement-order issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (11)
crates/windows-platform-probes/Cargo.toml:203
- This target condition still matches x86-64 builds where
cmpxchg16bis explicitly disabled. In that configuration it enablesdwcas, while the queue'sdwcasimplementation namesportable_atomic::AtomicU128, which this repository documents as unavailable without that instruction; the probe therefore fails to build instead of omitting theWiderows. Gate the x86-64 branch ontarget_feature = "cmpxchg16b"here and in the matching sourcecfgs.
[target.'cfg(any(target_arch = "x86_64", target_arch = "aarch64"))'.dependencies]
windows-waitable-queues = { path = "../windows-waitable-queues", features = [
"dwcas",
] }
crates/windows-platform-probes/src/queue_contention.rs:239
- The control and candidate rows are measured in a fixed, widely separated order:
reserving_mpscfirst, then the drained shapes, then the 32/32 and 64/64 layout rows. Any frequency, thermal, or scheduler drift across those intervening runs is therefore folded into the supposed same-code control and can make later layouts look slower; the reported 61% same-configuration spread makes this material. Interleave each candidate with a nearby control or otherwise balance the order before using these ratios as evidence.
isolated.push(median_run(shapes::RESERVING_MPSC, producers, |count| {
time_isolated_reserving(count)
}));
isolated.push(median_run(shapes::PERMIT_MPSC, producers, |count| {
time_isolated_permit(count)
}));
crates/windows-waitable-queues/Cargo.toml:64
- The antecedent of "choosing it" is
Perpetual, but the measurement described here is the slowerWide(64/64) layout. The probe and the updated README only establish the higher cost for the 128-bit word; leaving this as written attributes that cost toPerpetualand can recreate the withdrawn performance claim.
# throughput is not established, and choosing it measured slower on the
# whole push path as producer count rises -- near parity at one or two, several
# times by thirty-two, in the isolated regime. The probe times the complete push,
crates/windows-waitable-queues/DESIGN-NOTES.md:1270
- This variance caveat does not identify the D-35 table and its derived claims as predating the probe's corrected timing window. The values here (for example, 225.1/56.1/20.5 ns at 16 producers) are from the earlier coordinator-clock measurement, while the updated queue guidance explicitly says pre-correction figures are optimistic; please add an explicit pre-correction marker before this table so readers do not treat these absolute values and ratios as current.
**Read "within noise" here against the wider floor measured later**: seven runs
of this probe put the same-configuration spread at 7-61%, and its same-code
control at 0.68-1.27x, so three agreeing runs establish less than the phrase
suggests. See
[DESIGN-NOTES.md](../windows-platform-probes/DESIGN-NOTES.md#d-variance-is-a-finding).
crates/windows-waitable-queues/DESIGN-NOTES.md:857
- The updated variance note still leaves the D-26 measurement table unmarked as predating the corrected timing window. Those absolute rows are the same pre-correction figures that the README and crate rustdoc now label optimistic; add the same explicit caveat here, otherwise this design-note section presents stale throughput numbers as a current measurement.
**That "within noise" rests on a floor this workspace has since measured to be far
wider.** The figure was read against a 2-6% run-to-run spread; seven runs of the
same probe later put the same-configuration spread at 7-61% depending on producer
count, and the probe's own same-code control spans 0.68-1.27x. The figures below
are not retracted -- the direction of `D-26` survived a re-measurement on the
shipping type -- but "agreed within noise" is a weaker statement than it reads as,
and any difference here smaller than that control should not be treated as
established. See
[DESIGN-NOTES.md](../windows-platform-probes/DESIGN-NOTES.md#d-variance-is-a-finding).
crates/windows-waitable-queues/DESIGN-NOTES.md:78
- This decision states that all three layouts issue
lock cmpxchg, but the decision applies to the crate's ARM64 support as well as x86-64. That instruction name is x86-specific; retain the structural conclusion with “same atomic compare-exchange on the sameu64” or qualify the instruction to x86 so the decision does not misdescribe ARM64.
| <a id="d-41"></a>D-41 | **The claim word's apportionment is a caller's choice, and the recurrence behind SH-14.1 is a number the caller sets rather than one this crate imposes.** Supersedes [D-36](#d-36), whose premise was that the only fix was the [D-35](#d-35) claim-protocol replacement, gated on an open question -- so disclosing beat delaying. That premise was false, and measurement is what showed it: the 32/32 split followed from requiring the reservation half to hold the *entire capacity*, because every slot could be reserved at once. Capping outstanding reservations instead leaves the capacity bounded only by the ring, and the position is free to take 48 or 56 bits. `Balanced` (32/32), `Enduring` (16/48) and `Perpetual` (8/56) issue the **same** `lock cmpxchg` on the same `u64`, differing only in shift constants -- so the recurrence moves from about 37 seconds to about 20 years without a dependency. **An earlier version of this row added "and measured indistinguishable outside noise", and that clause is withdrawn**: it rested on a single probe run read against a noise floor of 2-6% that seven runs put at 7-61%. Re-measured, the deeper layouts are indistinguishable from `Balanced` at low producer counts, and at high counts differ by less than the run-to-run variation of the same code measured twice -- which is one host declining to call it, not a cost. The figures are in the probe's note rather than restated here. The layout is the caller's choice and the throughput question belongs on the caller's hardware; see the queue-contention section of [DESIGN-NOTES.md](../windows-platform-probes/DESIGN-NOTES.md#d-queue-layout-observations). The reservation half was the wrong half to spend bits on: it held 2^32 where the real bound is however many producers are mid-send. `Wide` (64/64 over a `u128`) is offered behind the non-default `dwcas` feature, because it is the one thing here that costs a third-party crate -- the standard library has no 128-bit atomic -- and choosing it measured slower on the whole push path as producer count rises in the isolated regime -- near parity at one or two, several times by thirty-two -- while falling inside the same-code control when drained. The probe times the complete push, so that is the layout's effect on that path rather than a measurement of the exchange alone; it buys a guarantee rather than a lifetime argument. **The default stays `Balanced`** so introducing the choice changed no existing caller's behaviour, and its documentation states the defect outright -- that past 2^32 pushes from two or more producers the queue can silently lose an item -- rather than leaving the status quo to look safe by inertia. It states the defect rather than recommending against the layout, per [D-no-client-prescriptions](../windows-platform-probes/DESIGN-NOTES.md#d-no-client-prescriptions): a caller told what the layout does can decide; a caller told what to prefer has been handed our judgement about their deployment. |
crates/windows-waitable-queues/README.md:365
- The README's lower-case
reserveclaim is also false as written: the table above listsspscas reserving, and the experimentalpermit_mpsc::Producerhas an inherentreservemethod. Please scope this bullet to the shipping MPSC pair or name the exceptions, so the selection guidance does not contradict the API it just documents.
- **`reserve` exists only on `reserving_mpsc`**; `slotwise_mpsc` structurally
cannot offer it. That no longer forces a trade against the recurrence, since
naming a layout addresses it.
crates/windows-waitable-queues/README.md:190
- The README is written for all supported targets, including ARM64, where
lock cmpxchgis not the instruction used for this atomic operation. The intended mechanical argument is target-neutral; please say “same atomic compare-exchange on the sameu64” or qualify this as x86-specific, and update the matching public rustdoc and D-41 text.
- **Naming a layout moves it.** `Perpetual` puts the recurrence about twenty
years out. **What it costs in throughput is not established** -- it issues the
same `lock cmpxchg` on the same `u64` as the
default, and measured indistinguishable from it at low producer counts; at
crates/windows-waitable-queues/src/lib.rs:286
- This says the
Reservingtrait exists only onreserving_mpsc, butspsc::Produceralso implements it (src/spsc.rs:1035) and its producer exposesreserve. Restrict this statement to the shipping MPSC choices; otherwise the public crate rustdoc incorrectly says a supported capability is absent.
//! - **[`Reserving`] exists only on [`reserving_mpsc`]**; [`slotwise_mpsc`]
//! structurally cannot offer it. Naming a layout addresses the recurrence, so
//! that no longer trades against this capability.
crates/windows-waitable-queues/src/lib.rs:162
- The crate documentation covers both x86-64 and ARM64, but
lock cmpxchgis x86-specific instruction syntax. Replace it with the target-neutral “same atomic compare-exchange on the sameu64” (or explicitly qualify the x86 measurement), otherwise this public explanation is inaccurate on ARM64; keep the corresponding module, README, and D-41 wording synchronized.
//! years out. **What it costs in throughput is not
//! established** -- it issues the same `lock cmpxchg` on the same `u64` as
//! the default, and measured indistinguishable from it at low producer
crates/windows-waitable-queues/src/reserving_mpsc.rs:190
- This is public, target-independent documentation, but
lock cmpxchgis x86 instruction syntax and is not what the ARM64 implementation executes. The structural point is valid as “the same atomic compare-exchange on the sameu64”; using an x86-specific instruction name here makes the claim false for one of the crate's supported architectures. Please use target-neutral wording (and propagate it to the duplicated crate/README/design-note text).
/// **Choosing a deeper position is the same instruction on the same word.** All
/// three issue the same `lock cmpxchg` on the same `u64` and differ only in
/// shift and mask constants, so there is no structural reason for one to be
- Files reviewed: 14/15 changed files
- Comments generated: 1
- Review effort level: Lite
…hitecture
**The i686 fix had a hole, and the same measurement that proved i686 proves this.**
Gating `dwcas` on `target_arch = "x86_64"` still enables it on an x86-64 build with
`cmpxchg16b` switched off, where `portable_atomic::AtomicU128` does not exist:
RUSTFLAGS="-C target-feature=-cmpxchg16b" cargo check -p windows-platform-probes
--target x86_64-pc-windows-msvc
error[E0433]: cannot find `AtomicU128` in `portable_atomic`
`D-37` predicted exactly this and its reasoning checks out on 1.98:
`rustc --print cfg --target x86_64-pc-windows-msvc -C target-feature=-cmpxchg16b`
still prints `target_has_atomic="128"` while dropping
`target_feature="cmpxchg16b"` -- so the feature is the discriminator and
`target_has_atomic` would have been the wrong gate, as D-37 says.
The manifest now carries two target sections, `cfg(all(target_arch = "x86_64",
target_feature = "cmpxchg16b"))` and `cfg(target_arch = "aarch64")`, with the
source `cfg`s matching. aarch64 needs no feature test -- `ldxp`/`stxp` is ARMv8-A
baseline. Verified in all four configurations: the no-`cmpxchg16b` build now
succeeds, i686 still succeeds, the host build still succeeds, and a release run
still emits all 12 `reserving(64/64)` rows.
**A methodological finding worth more than the rest of this round.** The
same-code control is the `reserving_mpsc` row against the `reserving(32/32)` row,
and `measure()` runs them FOUR measurements apart -- the permit shape and all
three drained shapes fall between, each five repetitions of 50,000 pushes per
producer. Frequency, thermal and scheduler drift across that interval lands inside
the control, and inside every candidate judged against it. That is the first
*specific* mechanism anyone has proposed for the 7-61% same-configuration spread;
the other candidates in D-variance-is-a-finding are general. Added there, and
queued as M4.4 with the same blocker M4.3 carries -- interleaving changes the
measurement and obsoletes the published figures -- plus a note that M4.4 should
precede M4.2's diagnosis, since an unpaired control cannot answer whether
lengthening the run narrows the spread.
Documentation corrections, all procedural:
- **`lock cmpxchg` is x86 syntax** in documentation that covers aarch64, where the
instruction is not that. The structural argument is target-neutral, so all four
sites now say "the same atomic compare-exchange on the same `u64`".
- **"`Reserving` exists only on `reserving_mpsc`" is false.** `spsc::Producer`
implements it (`spsc.rs:1035`) and `permit_mpsc::Producer` has an inherent
`reserve`. Scoped to the two MPSC shapes, with the others named.
- **The `dwcas` manifest comment attributed `Wide`'s cost to `Perpetual`** -- the
antecedent of "choosing it" was `Perpetual` while the figures were `Wide`'s,
which recreated the withdrawn claim by misattribution. Split into two
statements.
- **D-26's and D-35's tables were marked for the variance floor but not for the
timing correction**, though the README and rustdoc got that caveat last commit.
Same class, same treatment.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The concurrent benchmark and feature/CI changes span multiple crates and warrant final human validation.
Review details
Suppressed comments (9)
Previously missed (2) — in code that hasn't changed since the last review.
crates/windows-waitable-queues/README.md:190
- The README repeats the withdrawn-measurement wording with a grammatical omission: "and measured indistinguishable" should say that it was measured as indistinguishable. Otherwise this user-facing explanation reads as an incomplete sentence.
crates/windows-waitable-queues/src/lib.rs:162 - This new sentence is missing the word identifying the claim word and is grammatically malformed: it currently says "the same ... on the same the default." Please include
u64and make the comparison read naturally, otherwise the crate-level layout guidance is confusing.
crates/windows-waitable-queues/Cargo.toml:60
- The new manifest comment says
Widemeasured slower, which is not grammatical and obscures that the complete push path—not the exchange alone—was the measured subject. Please use passive wording.
# This feature adds only the `Wide` layout. `Wide` measured slower on the whole
crates/windows-waitable-queues/DESIGN-NOTES.md:78
- The updated D-41 row repeats
choosing it measured slower, which is grammatically incorrect and inconsistent with the rest of the measurement caveat. Change it to passive wording so the row says what the probe measured.
| <a id="d-41"></a>D-41 | **The claim word's apportionment is a caller's choice, and the recurrence behind SH-14.1 is a number the caller sets rather than one this crate imposes.** Supersedes [D-36](#d-36), whose premise was that the only fix was the [D-35](#d-35) claim-protocol replacement, gated on an open question -- so disclosing beat delaying. That premise was false, and measurement is what showed it: the 32/32 split followed from requiring the reservation half to hold the *entire capacity*, because every slot could be reserved at once. Capping outstanding reservations instead leaves the capacity bounded only by the ring, and the position is free to take 48 or 56 bits. `Balanced` (32/32), `Enduring` (16/48) and `Perpetual` (8/56) issue the **same** atomic compare-exchange on the same `u64`, differing only in shift constants -- so the recurrence moves from about 37 seconds to about 20 years without a dependency. **An earlier version of this row added "and measured indistinguishable outside noise", and that clause is withdrawn**: it rested on a single probe run read against a noise floor of 2-6% that seven runs put at 7-61%. Re-measured, the deeper layouts are indistinguishable from `Balanced` at low producer counts, and at high counts differ by less than the run-to-run variation of the same code measured twice -- which is one host declining to call it, not a cost. The figures are in the probe's note rather than restated here. The layout is the caller's choice and the throughput question belongs on the caller's hardware; see the queue-contention section of [DESIGN-NOTES.md](../windows-platform-probes/DESIGN-NOTES.md#d-queue-layout-observations). The reservation half was the wrong half to spend bits on: it held 2^32 where the real bound is however many producers are mid-send. `Wide` (64/64 over a `u128`) is offered behind the non-default `dwcas` feature, because it is the one thing here that costs a third-party crate -- the standard library has no 128-bit atomic -- and choosing it measured slower on the whole push path as producer count rises in the isolated regime -- near parity at one or two, several times by thirty-two -- while falling inside the same-code control when drained. The probe times the complete push, so that is the layout's effect on that path rather than a measurement of the exchange alone; it buys a guarantee rather than a lifetime argument. **The default stays `Balanced`** so introducing the choice changed no existing caller's behaviour, and its documentation states the defect outright -- that past 2^32 pushes from two or more producers the queue can silently lose an item -- rather than leaving the status quo to look safe by inertia. It states the defect rather than recommending against the layout, per [D-no-client-prescriptions](../windows-platform-probes/DESIGN-NOTES.md#d-no-client-prescriptions): a caller told what the layout does can decide; a caller told what to prefer has been handed our judgement about their deployment. |
crates/windows-waitable-queues/README.md:366
- This says that choosing a layout removes the trade between
Reservingand the recurrence, but the layouts deliberately trade reservation capacity for position width:Perpetualaccepts only 255 outstanding reservations andEnduring65,535. A caller needing more reservations still must choose between that ceiling and the shorter recurrence, so this guidance should qualify the claim.
- **Of the two MPSC shapes, only `reserving_mpsc` offers `reserve`**;
`slotwise_mpsc` structurally cannot. (`spsc` has it too, and the experimental
`permit_mpsc` exposes its own.) That no longer forces a trade against the
recurrence, since naming a layout addresses it.
crates/windows-waitable-queues/README.md:150
- The new sentence uses
choosing it measured slower, which makes a choice the agent that performed the measurement. Please use passive wording so it is clear that the whole push path was measured as slower.
and choosing it measured slower on the whole push path as producer count rises
-- near parity at one or two, several times by thirty-two, in the isolated
crates/windows-waitable-queues/src/lib.rs:288
- This says that choosing a layout removes the trade between
Reservingand the recurrence, but the layouts deliberately trade reservation capacity for position width:Perpetualaccepts only 255 outstanding reservations andEnduring65,535. A caller needing more reservations still must choose between that ceiling and the shorter recurrence, so the crate-level guidance should qualify the claim.
//! `reserve`.) Naming a layout addresses the recurrence, so that no longer
//! trades against this capability.
crates/windows-waitable-queues/src/lib.rs:118
- This new sentence makes the choice itself the subject of
measuredand is missing theu64comparison, so the public guidance is grammatically broken. Use passive wording and include the word being compared.
//! matter: it needs a 128-bit exchange, and choosing it measured slower on the
//! whole push path as producer count rises -- near parity at one or two,
crates/windows-waitable-queues/src/reserving_mpsc.rs:538
- This public rustdoc repeats the same grammatical error:
The 128-bit exchange measured slowermakes the exchange the measuring agent, while the probe measured the whole push path. Please say that the exchange's layout was measured as slower.
/// The 128-bit exchange measured slower on the whole push path than a `u64`
- Files reviewed: 14/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
… a construction Mostly grammar, with one substantive correction and one sentence I broke. **A sentence I broke in ea090c5.** Making the instruction wording target-neutral dropped two words from the crate rustdoc, leaving "the same atomic compare-exchange on the same the default" -- the `u64` and the "as" went with the replaced text. The README's copy survived intact, which is why it read correctly and this one did not. **A claim that was too strong.** Both selection blurbs said that naming a layout means wanting `reserve` "no longer forces a trade against the recurrence". The trade is not gone, it changes axis: a deeper position is paid for with a lower ceiling on outstanding reservations -- 65,535 under `Enduring` and 255 under `Perpetual`, against 2^32 under the default, as `reserving_mpsc.rs:478-517` states. A caller who needs more outstanding reservations than that still chooses between the ceiling and the shorter recurrence. Both sites now say so. **Six instances of one construction.** "choosing it measured slower", "`Wide` measured slower", "the 128-bit exchange measured slower" all make the subject the thing performing the measurement, and the last also names the exchange as the measured subject when the probe timed the whole push path. All now passive: the push path *was measured as* slower under the layout. Two related omissions fixed in the same pass -- "and measured indistinguishable" now reads "was measured as indistinguishable". The instance of "and measured indistinguishable outside noise" inside D-41 is left alone: it is a quotation of the withdrawn clause, and altering it would misrepresent what was withdrawn. Verified: fmt and clippy clean; 13 doctests including the compiled README. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
One moderate correctness issue and several documentation and checklist inconsistencies remain unresolved.
Review details
Suppressed comments (7)
crates/windows-platform-probes/CHECKLIST.md:173
- This block is listed before M4.4, but M4.4 explicitly says it must run before M4.2's diagnosis when both are taken because the currently unpaired control would confound that diagnosis. A contributor following the checklist order will therefore do the work in the wrong sequence; move M4.4 ahead of M4.2 or make the dependency/order unambiguous.
**Do this before M4.2's diagnosis work if both are taken**, since a control that is not paired
cannot answer whether lengthening the run narrows the spread -- the answer would be confounded by
the same drift.
crates/windows-platform-probes/DESIGN-NOTES.md:765
- The probe and its
dwcasdependency are gated onall(target_arch = "x86_64", target_feature = "cmpxchg16b"), not on x86-64 alone. This wording therefore overstates the set of targets that produce the 128-bit rows and contradicts the preceding gate rationale: an x86-64 build withcmpxchg16bdisabled deliberately omitsWide. Please name the target feature here so the design note matches the implemented support matrix.
`AtomicU64`, and 64/64 over `AtomicU128`. The last is measured only where a
128-bit exchange is native -- x86-64 and aarch64 -- so on a target without one
the report carries the other three and leaves its column empty.
crates/windows-platform-probes/DESIGN-NOTES.md:795
- This sentence gives the combined same-code control range as
0.69-1.27x, but the table immediately below reports0.68-1.27xonce both isolated and drained regimes are included, and the surrounding updated documentation uses the latter range. Please correct the lower bound or explicitly qualify this sentence as isolated-only so the design record does not contradict its own data.
figures, and the same-code control measured later ranges 0.69-1.27x, which is
wider than most of the differences being called "noise" -- note that this very
crates/windows-platform-probes/src/bin/queue_contention.rs:265
- On targets without a native 128-bit exchange (for example i686, or x86-64 without
cmpxchg16b), theWiderows are cfg-elided, so this report contains only three apportionments. The unconditional "Four apportionments" text is therefore false on a supported configuration; make the wording conditional or avoid the count.
" Four apportionments of reserving_mpsc's claim word, measured on"
crates/windows-waitable-queues/README.md:369
ClaimLayout::MAX_RESERVEDis capped atu32::MAX, andreservereturnsNoneonce that value is reached, so the default does not support 2^32 simultaneous reservations. This new selection guidance calls the ceiling2^32, unlike the exact 65,535 and 255 values beside it; please spell it asu32::MAX(2^32 - 1) to avoid overstating the public limit.
deeper position is paid for with a lower ceiling on outstanding reservations,
65,535 under `Enduring` and 255 under `Perpetual` against 2^32 under the
default.
crates/windows-waitable-queues/src/lib.rs:290
ClaimLayout::MAX_RESERVEDis capped atu32::MAX, andreservereturnsNoneonce that value is reached, so the default does not support 2^32 simultaneous reservations. This new selection guidance calls the ceiling2^32, unlike the exact 65,535 and 255 values beside it; please spell it asu32::MAX(2^32 - 1) to avoid overstating the public limit.
//! recurrence, but the trade is not gone -- it changes axis: a deeper position
//! is paid for with a lower ceiling on outstanding reservations, 65,535 under
//! `Enduring` and 255 under `Perpetual` against 2^32 under the default.
crates/windows-waitable-queues/src/reserving_mpsc.rs:485
ClaimLayout::MAX_RESERVEDis capped atu32::MAX, andreservereturnsNoneonce that value is reached, so the default does not support 2^32 simultaneous reservations. This updated public type documentation calls the ceiling2^32; please use the exactu32::MAX(2^32 - 1) limit so callers are not promised one extra reservation.
/// It is the default because it is what the shape shipped with, not because the
/// division is a good one: the 2^32 reservation ceiling it buys is far beyond
/// any use this crate has seen, and the exposure is what pays for it.
- Files reviewed: 14/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
…fore M4.2 Seven findings, each a statement that stopped being true when something else changed. **The report hardcoded "Four apportionments".** On a target without a native 128-bit exchange the `Wide` rows are cfg-elided, so the count is three and the sentence was false. Replaced with a count derived from what was actually measured, which cannot drift. Verified in both directions by building and running each configuration: the host build reports 4 with 12 `reserving(64/64)` rows, and a `-C target-feature=-cmpxchg16b` build reports 3 with none. The `64/64` column header is left in place on that build -- its cells render `--`, the documented marker for a measurement not taken, which discloses that the layout exists rather than hiding it. **The reservation ceiling was rounded, and inconsistently.** `MAX_RESERVED` caps at `u32::MAX`, so `Balanced` holds 2^32 - 1 outstanding reservations, not 2^32 -- while the two figures written beside it, 65,535 and 255, are exact. Corrected in the two selection blurbs I added last commit and in `Balanced`'s own rustdoc, where the same rounding was pre-existing. **Two statements that my own gate tightening invalidated.** The design note still said the wide rows are measured "where a 128-bit exchange is native -- x86-64 and aarch64", but ea090c5 narrowed the gate to the target FEATURE: an x86-64 build with `cmpxchg16b` disabled omits them deliberately. And the stand-in section gave the same-code control as 0.69-1.27x, mixing the isolated minimum with the drained maximum; the combined range is 0.68-1.27x, which is what every other site says. **M4.4 said it must precede M4.2 and was filed after it.** Moved ahead, with the reason restated from its new position and a reciprocal note on M4.2 explaining why it appears second despite the lower number. IDs are unchanged -- this milestone already documents why they stay stable. Verified: fmt and clippy clean; 13 doctests including the compiled README; probe run in both target-feature configurations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Public layout documentation and measurement rationale still contain inconsistencies that should be corrected.
Review details
Suppressed comments (6)
crates/windows-platform-probes/src/bin/queue_contention.rs:337
- The report labels
Wideas removing recurrence outright, but the measured layout still has a 64-bit position and wraps after 2^64 pushes. Keep the interpretation aligned with the queue contract by describing this as moving recurrence to 2^64 (practically unreachable), not eliminating it.
" push path -- what removing the recurrence outright costs, against"
crates/windows-platform-probes/src/queue_contention.rs:627
- This rationale misstates what high-water tracking adds.
reserving_mpsc::publishalways performs the acquire load of the consumer position needed before writing the slot and reuses that value for the depth calculation, even when tracking is disabled; enabling tracking adds the branch/arithmetic/metric update, not another read of the shared line. Please correct this explanation so the comparison does not attribute an existing synchronization load to the optional metric.
// enabled high-water tracking here and nowhere else, to "also price the
// switch M31.4 made opt-in". But the number it feeds is presented as the
// cost of *reservation*, and tracking adds an unrelated operation to this
// shape's push path alone -- a load of the consumer's position, which is
// exactly the shared line the other shape's push is built to avoid
// touching. The ratio therefore measured reservation plus a handicap, with
// no way for a reader to separate them.
crates/windows-waitable-queues/README.md:222
Widedoes not eliminate recurrence mathematically: its 64-bit position wraps after 2^64 pushes. The crate's rustdoc documents that bound, so this README wording is an inconsistent public guarantee. Replace "removed outright" with wording that says the recurrence is beyond any practical deployment.
that the `u64` layouts do not is the recurrence removed outright rather than
deferred.
crates/windows-waitable-queues/src/lib.rs:123
Wideuses a 64-bit position and therefore still recurs after 2^64 pushes; the public layout docs state that explicitly. This new sentence says the recurrence is removed outright, which gives callers a stronger guarantee than the implementation provides. Say that it moves the recurrence beyond any practical deployment instead.
//! not is the recurrence removed outright rather than deferred.
crates/windows-waitable-queues/src/reserving_mpsc.rs:553
Widestill has a 64-bit position (POSITION_BITS = 64), so its claim position recurs after 2^64 pushes; the preceding documentation explicitly states that. Calling the recurrence "removed outright" overstates the guarantee and contradicts that contract. Describe it as moved beyond any practical deployment instead.
/// layout provides that the others do not is the recurrence removed outright
/// rather than deferred.
crates/windows-waitable-queues/src/reserving_mpsc.rs:482
ClaimLayout::MAX_RESERVEDis explicitly capped atu32::MAX, but the public layout tables insrc/lib.rsandREADME.mdstill list2^32outstanding reservations (including theWiderow). That gives callers two different ceilings; update those tables, or label the power-of-two entries as field widths rather than exact counts.
/// Holds [`u32::MAX`] outstanding reservations and recurs after 2^32 pushes --
/// about
/// **37 seconds** of sustained maximum-rate pushing. Past that point, with two
/// or more producers, the queue can **silently lose an item**: that is the whole
/// of the `SH-14.1` exposure, and this layout carries it.
- Files reviewed: 14/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
Six findings, two of them factual errors about code. **`Wide` was described as removing the recurrence outright, in five places.** Its `POSITION_BITS` is 64, so the position recurs after 2^64 pushes -- which the layout tables have always said, and which the prose then contradicted. That is a stronger guarantee than the implementation provides, and it was published in the README, the crate rustdoc, `ClaimLayout`'s docs, the `reserving_mpsc` module header, and the probe's own report. All now say the recurrence moves to 2^64, which no deployment reaches, rather than that it is eliminated. Four of those were mine, written while removing client prescriptions. The fifth was pre-existing and is the likely source: `Wide`'s own rustdoc gave the bound correctly and then glossed it as "not 'not for twenty years', but not at all", which is where the "outright" reading came from. **A factual error about the queue's implementation.** The comment explaining why this probe measures both shapes with default metrics said high-water tracking "adds a load of the consumer's position". It does not: `reserving_mpsc::publish` loads `head` **unconditionally**, because the slot write needs that acquire edge whether or not anything is measured -- the comment at that load says exactly this, and the code confirms it (the load precedes the `tracks_high_water()` branch and its result is reused by it). What the switch adds is the depth arithmetic and the metric update. The handicap is real and the correction stands; it is smaller than claimed and in a different place. **The reservation ceilings were inconsistent between prose and tables.** Last commit corrected the prose to `u32::MAX` but left three layout tables saying 2^32, so callers had two ceilings for the same layout -- and the tables were internally inconsistent too, giving exact counts for `Enduring` (65,535) and `Perpetual` (255) beside rounded ones for `Balanced` and `Wide`. All four entries are now the exact 4,294,967,295. Verified: fmt and clippy clean; 13 doctests including the compiled README. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Five unresolved documentation corrections remain in the final review comments.
Review details
Suppressed comments (5)
crates/windows-waitable-queues/DESIGN-NOTES.md:78
- The canonical D-41 row says the three layouts differ only in shift constants, but the implementation also specializes
POSITION_MASK:advanceanddistanceboth use that mask (crates/windows-waitable-queues/src/reserving_mpsc.rs:571-592), and the publicClaimLayoutdocumentation explicitly says “shift and mask constants” (src/reserving_mpsc.rs:188-191). Please change this phrase to “shift and mask constants” so the decision does not misdescribe the code that is being compared.
| <a id="d-41"></a>D-41 | **The claim word's apportionment is a caller's choice, and the recurrence behind SH-14.1 is a number the caller sets rather than one this crate imposes.** Supersedes [D-36](#d-36), whose premise was that the only fix was the [D-35](#d-35) claim-protocol replacement, gated on an open question -- so disclosing beat delaying. That premise was false, and measurement is what showed it: the 32/32 split followed from requiring the reservation half to hold the *entire capacity*, because every slot could be reserved at once. Capping outstanding reservations instead leaves the capacity bounded only by the ring, and the position is free to take 48 or 56 bits. `Balanced` (32/32), `Enduring` (16/48) and `Perpetual` (8/56) issue the **same** atomic compare-exchange on the same `u64`, differing only in shift constants -- so the recurrence moves from about 37 seconds to about 20 years without a dependency. **An earlier version of this row added "and measured indistinguishable outside noise", and that clause is withdrawn**: it rested on a single probe run read against a noise floor of 2-6% that seven runs put at 7-61%. Re-measured, the deeper layouts are indistinguishable from `Balanced` at low producer counts, and at high counts differ by less than the run-to-run variation of the same code measured twice -- which is one host declining to call it, not a cost. The figures are in the probe's note rather than restated here. The layout is the caller's choice and the throughput question belongs on the caller's hardware; see the queue-contention section of [DESIGN-NOTES.md](../windows-platform-probes/DESIGN-NOTES.md#d-queue-layout-observations). The reservation half was the wrong half to spend bits on: it held 2^32 where the real bound is however many producers are mid-send. `Wide` (64/64 over a `u128`) is offered behind the non-default `dwcas` feature, because it is the one thing here that costs a third-party crate -- the standard library has no 128-bit atomic -- and the whole push path was measured as slower under it as producer count rises in the isolated regime -- near parity at one or two, several times by thirty-two -- while falling inside the same-code control when drained. The probe times the complete push, so that is the layout's effect on that path rather than a measurement of the exchange alone; it buys a guarantee rather than a lifetime argument. **The default stays `Balanced`** so introducing the choice changed no existing caller's behaviour, and its documentation states the defect outright -- that past 2^32 pushes from two or more producers the queue can silently lose an item -- rather than leaving the status quo to look safe by inertia. It states the defect rather than recommending against the layout, per [D-no-client-prescriptions](../windows-platform-probes/DESIGN-NOTES.md#d-no-client-prescriptions): a caller told what the layout does can decide; a caller told what to prefer has been handed our judgement about their deployment. |
crates/windows-waitable-queues/README.md:152
- This README performance claim is not qualified by the machine that produced it, even though the result is explicitly a host-specific whole-push-path observation in the design note and rustdoc. Please say that these figures are from the isolated run on one x86-64 host; otherwise readers may treat the
Widescaling as a portable guarantee.
the same code measured twice. `Wide` is a separate matter: it needs a 128-bit exchange,
and the whole push path was measured as slower under it as producer count rises
-- near parity at one or two, several times by thirty-two, in the isolated
regime -- and it is the only thing in
this crate that costs a third-party dependency.
crates/windows-waitable-queues/README.md:220
- The Cargo-features section repeats the same unqualified performance claim: this is a one-host isolated measurement, not a target-independent property. Qualify the sentence here too so the README does not give a different scope to the result later on the same page.
that costs in throughput is not established, while under `Wide` the whole push
path was measured as slower as producer count rises -- near parity at one or
two, several times by thirty-two, in the isolated regime. What `Wide` provides
crates/windows-waitable-queues/src/lib.rs:121
- This public crate-level paragraph presents the
Wideslowdown as an observed layout property without saying that the observation is from one x86-64 host and the isolated regime. The design note andreserving_mpscrustdoc qualify it, but callers reading this overview could treat the scaling as portable; please carry the host qualification into this summary (and keep the sentence grammatically consistent).
//! matter: it needs a 128-bit exchange, and the whole push path was measured as
//! slower under it as producer count rises -- near parity at one or two,
//! several times by thirty-two, in the isolated regime -- and it is the only
//! thing in
//! this crate
crates/windows-waitable-queues/src/reserving_mpsc.rs:34
- The new text correctly describes
Wideas adwcaslayout, but this public module still contains the old packing section below: it says the 128-bit word is not used here, names a nonexistentreserving_mpsc_wide, and tells callers to request 2^62 slots through that shape.Wideis now an in-module layout with au32reservation ceiling, so update or remove that stale section; otherwise the rustdoc gives contradictory API guidance.
//! them, and the `dwcas` feature adds a 128-bit word whose 64-bit position moves
//! the recurrence to 2^64 pushes, which no deployment reaches.
- Files reviewed: 14/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
…ribution The README and crate rustdoc carried a two-host comparison whose x64 sixteen- producer row was 193.5 / 52.2 -- the exact figures the probe's note flags as predating the correction to the timing window. Labelling them was a stopgap; this replaces them. **Re-measured**, on the machine this branch has been developed and verified on, with the capture parameters published beside the numbers rather than inferred: host banner verbatim, build profile, sampling parameters, run count, the instrument and its commit, and the date. A figure without those is not data anyone else can use, which is what D-observations-not-verdicts says and what the old table did not do. The table also gains the two columns it should always have had: `permit_mpsc`, which is the shape under evaluation, and `baseline_fetch_add`, which is N threads on one `AtomicU64` -- without it a reader cannot tell how much of any curve is the queue and how much is what this processor does to a contended line. **The ARM64 data point is gone rather than stale**, and that is stated plainly. Its figures predate the same correction, and neither the EPYC 7763 nor the Snapdragon X2 Elite is available here to retake them. Restoring a second architecture is what M2.15 in the probe crate's checklist is for. The one finding from that comparison that was structural rather than numeric is kept: the split was designed on the assumption that `slotwise_mpsc` would be cheaper, and measurement disagreed on both machines. Also in this pass, from the review: - **A stale section named a shape that never shipped.** `reserving_mpsc`'s module header still said a wide claim "ships instead as its own shape (`reserving_mpsc_wide`, not yet built)" and told callers to request 2^62 slots through it. D-41 made the wide word a layout in this module instead. Rewritten to say what shipped, keeping the reason the feature is non-default. - **"differing only in shift constants" was incomplete** -- `POSITION_MASK` is specialized per layout too, used by `advance` and `distance` and const-asserted to differ. `ClaimLayout`'s own rustdoc already said "shift and mask constants"; D-41 and the module header now agree with it. - **Two surviving client prescriptions**: "Measure your own workload before treating any of this as settled" in the README and its twin in the rustdoc. Replaced with what moves the numbers, and a statement that the probes exist and run the measurement -- which is a fact about the repository rather than an instruction. This also fixed a dangling reference, since "5.6x on one of these hosts" no longer had hosts to refer to. Verified: fmt and clippy clean; default-feature rustdoc clean (the module header's `Wide` mention deliberately does not link, since the type is `dwcas`-gated); 13 doctests including the compiled README. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The multi-crate measurement and documentation changes retain corrections requiring final human review.
Review details
Suppressed comments (6)
Previously missed (3) — in code that hasn't changed since the last review.
crates/windows-waitable-queues/README.md:128
- This row now reports the packed count-field ceiling as if it were the number of outstanding reservations a
Balancedqueue can actually hold. The layout's capacity ceiling is2^31, so a caller cannot create 4,294,967,295 simultaneous reservations; rename the column to make the field-versus-capacity distinction explicit.
crates/windows-waitable-queues/src/lib.rs:96 - This row now reports the packed count-field ceiling as if it were the number of outstanding reservations a
Balancedqueue can actually hold. The layout's capacity ceiling is2^31, so a caller cannot create 4,294,967,295 simultaneous reservations; rename the column to make the field-versus-capacity distinction explicit.
crates/windows-waitable-queues/src/reserving_mpsc.rs:179 - The
4,294,967,295value is theMAX_RESERVEDpacked-field ceiling, not the number of reservations aBalancedqueue can hold: this layout'sBOUNDS_MAXis2^31, and reservation admission is capacity-bounded. Calling this column "Outstanding reservations" therefore overstates the achievable queue state; label it as the field ceiling (or show the capacity-dependent bound).
crates/windows-platform-probes/DESIGN-NOTES.md:905
- The new shipping-type section says these figures supersede the earlier layout calculations, but the earlier table at lines 860-869 still lists
2^32maximum reservations for 32/32 and2^64for 64/64.ClaimLayout::MAX_RESERVEDis capped atu32::MAXbecause the count is returned asu32, so this design note still documents impossible reservation counts; update or remove that table as part of this documentation sweep.
`CW-1.6` deleted the duplicated protocol in this crate once
`windows-waitable-queues` took the layout as a parameter, so the probe now
instantiates the real type at each layout. The numbers below supersede the ones
above, which were taken from the stand-in.
crates/windows-platform-probes/src/bin/queue_contention.rs:120
- The report labels this scaling column
slotwise x1thr, but that abbreviation is not defined and reads as a different measurement from the other shape columns. Since the surrounding text says every value is throughput at N divided by throughput at one, use a clear label such asslotwise scaleso captured reports remain interpretable.
"producers", "slotwise x1thr", "reserving", "permit", "atomic floor"
crates/windows-waitable-queues/DESIGN-NOTES.md:78
- This amended decision still says the reservation half "held 2^32", while the implementation and the newly corrected public tables define the largest count as
u32::MAX(2^32 - 1). A 32-bit field has 2^32 encodings, but its maximum count is one lower; update the decision and its related rollover table so the contract distinguishes field width from the maximum count.
| <a id="d-41"></a>D-41 | **The claim word's apportionment is a caller's choice, and the recurrence behind SH-14.1 is a number the caller sets rather than one this crate imposes.** Supersedes [D-36](#d-36), whose premise was that the only fix was the [D-35](#d-35) claim-protocol replacement, gated on an open question -- so disclosing beat delaying. That premise was false, and measurement is what showed it: the 32/32 split followed from requiring the reservation half to hold the *entire capacity*, because every slot could be reserved at once. Capping outstanding reservations instead leaves the capacity bounded only by the ring, and the position is free to take 48 or 56 bits. `Balanced` (32/32), `Enduring` (16/48) and `Perpetual` (8/56) issue the **same** atomic compare-exchange on the same `u64`, differing only in shift and mask constants -- so the recurrence moves from about 37 seconds to about 20 years without a dependency. **An earlier version of this row added "and measured indistinguishable outside noise", and that clause is withdrawn**: it rested on a single probe run read against a noise floor of 2-6% that seven runs put at 7-61%. Re-measured, the deeper layouts are indistinguishable from `Balanced` at low producer counts, and at high counts differ by less than the run-to-run variation of the same code measured twice -- which is one host declining to call it, not a cost. The figures are in the probe's note rather than restated here. The layout is the caller's choice and the throughput question belongs on the caller's hardware; see the queue-contention section of [DESIGN-NOTES.md](../windows-platform-probes/DESIGN-NOTES.md#d-queue-layout-observations). The reservation half was the wrong half to spend bits on: it held 2^32 where the real bound is however many producers are mid-send. `Wide` (64/64 over a `u128`) is offered behind the non-default `dwcas` feature, because it is the one thing here that costs a third-party crate -- the standard library has no 128-bit atomic -- and the whole push path was measured as slower under it as producer count rises in the isolated regime -- near parity at one or two, several times by thirty-two -- while falling inside the same-code control when drained. The probe times the complete push, so that is the layout's effect on that path rather than a measurement of the exchange alone; it buys a guarantee rather than a lifetime argument. **The default stays `Balanced`** so introducing the choice changed no existing caller's behaviour, and its documentation states the defect outright -- that past 2^32 pushes from two or more producers the queue can silently lose an item -- rather than leaving the status quo to look safe by inertia. It states the defect rather than recommending against the layout, per [D-no-client-prescriptions](../windows-platform-probes/DESIGN-NOTES.md#d-no-client-prescriptions): a caller told what the layout does can decide; a caller told what to prefer has been handed our judgement about their deployment. |
- Files reviewed: 14/15 changed files
- Comments generated: 0 new
- Review effort level: Lite
Peeled from
mikegrier/deferred-namespace-ops, where it was written alongside work that is not ready.It lands on its own because it is an instrument: useful before the plan that consumes it exists, and landing it first is what lets that decision be made against a measurement rather than an argument.
What it measures
Two questions a queue-shape decision is waiting on:
reserving_mpsc's extra read of the consumer's position actually costs.Two regimes, and the pair is the point. Isolated gives producers capacity large enough that nothing is refused and runs no consumer, so whatever curve appears against N is the claim and nothing else. Drained runs a consumer popping continuously -- the only regime that can price the read of
head, because that read is cheap until a consumer is writing the line. Measuring it in isolation would report it as free.Each row carries the refusal count from the queue's own
Observablecounters, so a consumer-bound plateau at high producer counts is visible as a fact rather than mistaken for contention.Verified by running it, not by building it
Seven runs on
x86_64 16p/8c smt+ L2[2,2,2,2,2,2,2,2] ec[0:16] numa[16], release, exit 0, ~65s each. Isolated, at sixteen producers, median of seven:baseline_fetch_addpermit_mpscreserving_mpscslotwise_mpscGate: fmt, clippy
--all-targets, 304 lib tests, 13 doctests including the compiled README.A timing defect found in review, and what it moved
The first version of the probe released a
Barrier, calledInstant::now()on the coordinator, and readelapsed()afterthread::scopereturned. Both ends were wrong:thread::scopejoins before returning, so exit and join cost sat inside the window.Fixed with per-worker timestamps and a span of
min(start)..max(end). Measured effect:reserving_mpscat sixteen producers moved 35.0 -> 52.3 ns/push. An earlier review round had explicitly cleared this code as correct.Any figure in the design notes taken before that correction is marked as predating it.
The claim this probe withdrew
The notes had said the
u64re-apportionments "track the default within noise", soPerpetual's twenty years of counter headroom was free. Re-measuring seven times says otherwise, and in a way worth stating precisely:reserving_mpscandreserving(32/32)are the same code at the same layout, measured twice per run, so their ratio is an empirical "no difference" -- and it spans 0.68-1.27x.Against that control the 128-bit word separates decisively in isolation (3.45x / 3.81x at 16 / 32 producers); the
u64re-apportionments do not. The claim is withdrawn in both directions rather than inverted: 1.23-1.30x against a control reaching 1.12x, on one host, is a flag to measure locally, not a cost.That withdrawal had to sweep out of this crate, because the claim had propagated into
windows-waitable-queues-- its README, its public rustdoc, andClaimLayout's own doc comment, which is what a caller reads while choosing a layout. The mechanical argument is kept (samelock cmpxchg, sameu64, shift constants only, so no structural reason for one to be slower); only the claim that this was measured to cost nothing is gone.Perpetualis still the right recommendation. It is no longer described as free.Why it is NOT in the CI probe job
Deliberate, and the reason is a measurement rather than a preference. In a debug build
slotwise_mpscandreserving_mpsccome out indistinguishable (249.7 vs 254.0 ns/push at sixteen producers); in release, 193.5 vs 52.2. A debug run does not merely lose precision -- it reports the two shapes as equivalent, which is a confident wrong answer.(Those four figures predate the timing correction above and have not been retaken. The qualitative finding is unaffected -- a debug build still swamps the effect.)
It also wants more cores than a hosted runner has, and costs about a minute against a job whose other probes take seconds. So it is run by hand, on a known machine, with the numbers recorded against that machine.
A CI hole this PR opened, and closed
Adding
windows-waitable-queueswithdwcas+experimental-permit-claimunified those features workspace-wide. CI built that crate only via--workspace, so nothing compiled the no-dwcaspath any more -- anddwcasis additive, gatingWideandClaimLayoutitems that would have stopped being checked.Restored with a dedicated default-features job, modelled on the existing
placement-probe-no-serdejob.Feature flags
experimental-permit-claimis enabled on the dependency because this probe is what decides its fate -- it has to be measured against the shipping shapes on the same host, in the same run, by the same harness.dwcasis what lets it instantiate the 128-bit claim layout.Review history
Five rounds. The first four each found something in the code: the feature-unification hole, the timing window, four unguarded retry loops, and three prose sites naming
mpscfor a module actually calledslotwise_mpsc. The fifth ran the probe end-to-end and found nothing in the code -- only the documentation contradiction that produced the withdrawal sweep above.