Skip to content

fix(bench): pin the pre-registered verdict pair so --widths cannot re-base it - #2245

Merged
justinchuby merged 2 commits into
mainfrom
squad/seb-worker-split-rule-pair
Aug 27, 2026
Merged

justinchuby merged 2 commits into
mainfrom
squad/seb-worker-split-rule-pair

Conversation

@justinchuby

Copy link
Copy Markdown
Owner

The defect

acc0_w16_worker_split.py's pre-registered rule says "Compare w=16 against w=8". verdict() and report() each computed narrow, wide = min(widths), max(widths).

I ran it with --widths 2,8,16 to get an extra descriptive column. That silently re-based every verdict and the entire Amdahl calibration onto w=2. Nothing in the output said so — the header line, the threshold banner and the verdict text were byte-identical in shape to a correct run.

What it changed

Re-scored from the identical archived datasets, 7 trusted launches per arm, no new host time:

published (w=2 baseline) pre-registered (w=8 baseline)
blocktime=0 verdict DISPATCH-BOUND DISPATCH-BOUND (unchanged)
blocktime=500 (shipped) verdict DISPATCH-BOUND + IMBALANCE-BOUND IMBALANCE-BOUND only
serial excess over Amdahl, bt=0 +0.086 −0.009
serial excess over Amdahl, bt=500 +0.082 −0.064
recoverable-in-principle, bt=500 0.146 of the window 0.064 of the window

The sign of the headline term flips. I had named that +0.086 "serial excess" as this lane's largest recoverable term and its highest-value remaining target. It does not exist. At w=8 → w=16 the dispatcher's serial share is below what Amdahl predicts. The only term the rule attributes at the shipped blocktime is the straggler wait, 0.064 of the window — which redirects the lane at #2017's lane-versus-chunk question rather than at the dispatcher.

DISPATCH-BOUND now misses at the shipped blocktime by 0.009 on a 0.10 threshold. That is a miss, not a near-hit to argue around.

WAKE-BOUND fires on neither baseline at either blocktime, so the one genuinely surprising result survives intact: my own futex-park hypothesis stays falsified at sessions=1, and the shipped 500 µs window's cost — full parking, 345.9 parks per worker — is 2.2% of the window in wake latency.

Why w=2 is wrong mechanically, not merely by the rule

The Amdahl calibration takes the narrow width's work_frac as "serial/parallel with no defect" and divides the parallel part by the width ratio. It is only as good as its baseline, and it is sharply sensitive to it.

At w=2 the pool spawns one worker — reserve_single_group_headroom keeps a CPU for the dispatcher and the dispatcher owns a shard inline. The mean worker is busy 0.997 of the window because there is barely a barrier to be serial at: one worker cannot wait for a straggler, and the dispatcher's shard runs concurrently with it rather than serially to it. That extrapolates to a predicted serial term of 0.021 at w=16. Nothing real clears that bar, so anything observed reads as "excess".

At w=8 the mean worker is busy 0.909 — a seven-participant barrier with a measurable dispatch gap. Predicts 0.167; observed 0.103.

Same dataset, opposite conclusion. This is not a threshold that wants tuning; w=8 was pre-registered before the first run for exactly this reason.

The fix

RULE_NARROW = 8 / RULE_WIDE = 16 are now constants, documented as clauses of the rule:

  • the pair is no longer derived from --widths;
  • extra widths are still measured and printed, under an explicit descriptive only, NOT used by the rule or the decomposition banner;
  • a run missing either member prints REPORT NOTHING (the pre-registered rule compares w=16 against w=8; this run measured 2,16) instead of substituting what it has;
  • every run prints # verdict pair: w8 vs w16 (pre-registered; not taken from --widths), so the provenance is in the output rather than in the reader's memory.

--binary is no longer required for --replay. It was demanded and never used — and awkward re-scoring is how a re-based verdict survives.

Tests, and they are proved against the defect

--self-test runs on a synthetic dataset built so the w=8 rise is +0.09 (below threshold) and the w=2 rise is +0.17 (above). DISPATCH must not fire under --widths 8,16, 2,8,16, 16,8 or 2,16,8,4; IMBALANCE must; the scored pair must be exactly {8, 16}. It also covers both REPORT NOTHING paths and the n_trusted floor.

Mutation-proved by restoring the original min/max:

AssertionError: (2, 8, 16): scored on [2, 16], not the pre-registered pair

It runs in CI. hostlock.yml already path-filters on crates/onnx-runtime-ep-cpu/benches/** and already owns driver conformance for that directory, so the new step lives there. It discovers harnesses advertising --self-test by behaviour rather than by a list — the same argument that file already makes about its own path filter, that the harness which most needs the check is the one that does not exist yet. The self-tests are arithmetic on synthetic records: no binary, no host load, no lock, safe on a shared runner.

Local: --self-test passes; both archived datasets replay to the corrected verdicts; the discovery loop runs clean; YAML parses.

Record

docs/benchmarks/2026-08-26-acc0-w16-baseline-correction.md carries the corrected decomposition for both arms, the mechanical argument against w=2, and the corrected priority list.

Method note

This is the third defect of one shape this week: a harness selecting an arm or a baseline by a parameter nobody re-checked — PIN-ON, a string the code never emits (#2177); a OnceLock that latches the first arm (#1736); and now a rule baseline taken from argv. In every case the output looked exactly like a valid measurement. The general defence is the one applied here: anything the pre-registered rule names must be pinned in code and printed in the output, and the harness must have a self-test that fails against the defect rather than exercising the happy path.

I will post the corrections to #2071 and #1801 whether or not this merges.

Refs #2071 #1801 #2017

…-base it

`acc0_w16_worker_split.py`'s rule says "Compare w=16 against w=8", but
`verdict()` and `report()` each took the pair from `min(--widths)` /
`max(--widths)`. Running it with `--widths 2,8,16` for an extra descriptive
column silently re-based every verdict and the whole Amdahl calibration onto
w=2, and the output was byte-identical in shape to a correct run.

Re-scored from the identical datasets against the pre-registered baseline, two
published conclusions change:

  * shipped blocktime: DISPATCH-BOUND + IMBALANCE-BOUND -> IMBALANCE-BOUND only
    (residual rise +0.091 against a +0.10 threshold)
  * "serial excess over Amdahl" +0.086 -> -0.009 (blocktime 0) and +0.082 ->
    -0.064 (blocktime 500). The sign flips: there is no serial excess, and the
    term I had named as this lane's largest recoverable target does not exist.

w=2 is the wrong baseline mechanically, not just by the rule: it spawns one
worker, so the dispatcher computes half the op inline and there is barely a
barrier to be serial at. work_frac 0.997 extrapolates to a serial prediction of
0.021 at w=16, which nothing real can clear.

WAKE-BOUND still fires on neither baseline at either blocktime.

RULE_NARROW/RULE_WIDE are now constants. Extra widths are still measured and
printed under an explicit "descriptive only" banner; a run missing either member
of the pair reports nothing instead of substituting what it has; and the pair is
printed on every run so the provenance is in the output, not the reader's
memory.

--self-test proves the pinning on a synthetic dataset built so the w=8 rise is
below threshold and the w=2 rise is above, under four --widths permutations,
plus the REPORT NOTHING and n_trusted paths. Mutation-proved: restoring
min/max fails with "scored on [2, 16], not the pre-registered pair".

It runs in CI. `hostlock.yml` already path-filters on the benches directory and
already owns driver conformance for it, so the new step discovers any harness
advertising --self-test by behaviour rather than by a list. The self-tests are
arithmetic on synthetic records: no binary, no host load, no lock.

--binary is no longer required for --replay; it was demanded and never used,
which made re-scoring an archived dataset harder than it should have been.

Refs #2071 #1801 #2017

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@codecov

codecov Bot commented Aug 27, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.71%. Comparing base (bb08c5a) to head (57c24ad).
⚠️ Report is 13 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2245      +/-   ##
==========================================
+ Coverage   80.64%   80.71%   +0.06%     
==========================================
  Files         432      433       +1     
  Lines      220114   221629    +1515     
  Branches   220114   221629    +1515     
==========================================
+ Hits       177512   178883    +1371     
- Misses      36717    36821     +104     
- Partials     5885     5925      +40     
Flag Coverage Δ
cli-ort-linux 72.51% <ø> (ø)
cli-ort-windows 72.10% <ø> (+0.09%) ⬆️
mlas 85.90% <ø> (ø)
offline 80.83% <ø> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 17 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Independent review found the self-test guarded `verdict()` only. The defect
lived in `report()` as well, and `report()` is the sole caller of
`barrier_decomposition` -- the Amdahl calibration whose sign flipped from +0.086
to -0.009. A future edit reverting only `report()` would print a correct
VERDICT line above a re-based serial excess, and the test would stay green.

The self-test now drives the whole report and checks the pinned pair reached the
calibration: the decomposition header reads w8/w16 and the Amdahl line divides
by ratio 2, not the 8 a w=2 baseline gives. Mutation-proved: reverting only
`report()`'s pair fails the run; before this commit that mutation passed,
because the test never called `report()`.

CI discovery now matches the argparse registration under either quote style
rather than the bare literal, so a file mentioning the flag in prose is not
selected and then failed for not accepting it, and a harness registering it with
single quotes is not silently skipped. Verified by extracting the step's script
out of the workflow YAML and running it verbatim.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@justinchuby

Copy link
Copy Markdown
Owner Author

Independent review (Opus), and what it changed

One real find, which I have fixed, plus one hardening. Seven other lines of enquiry were traced and came back clean; I am recording those too, because "I checked and it was fine" is evidence and an unrecorded check gets repeated.

The find — SHOULD-FIX, and it was the important half

The self-test guarded verdict() only. But the defect lived in report() as well, and report() is the sole caller of barrier_decomposition() — the Amdahl calibration whose sign flipped from +0.086 to −0.009. So a future edit that re-based only report() would print a correct VERDICT: line above a wrong serial excess, and the CI step this PR adds would stay green.

That is the same failure class the PR is about, one level up: I tested the half that produces the label and not the half that produces the number.

Fixed in 57c24ad90. The self-test now drives the whole report through contextlib.redirect_stdout and asserts the pinned pair reached the calibration — the decomposition header must read w8/w16, and the Amdahl line must divide the parallel time by ratio 2, not the 8 a w=2 baseline gives.

Mutation-proved. Reverting only report()'s pair:

  File "acc0_w16_worker_split.py", line 547, in self_test
    report(rows, widths)
  File "acc0_w16_worker_split.py", line 379, in report
    print(f"{k:>18} {vals[narrow]:>10.3f} {vals[wide]:>10.3f} "
KeyError: 2

Before this commit that mutation passed, because the test never called report().

There is also a structural property worth naming: verdict() returns its median map keyed only by the pinned pair, so report() now cannot decompose on any other width without failing loudly rather than printing a plausible wrong number.

The hardening — CI discovery

Discovery matched the bare literal "--self-test", which is quote-style-dependent in one direction and over-broad in the other: a harness registering the flag with single quotes would be silently skipped (the "assigned, never executed" shape this workflow's own header rails against), while a file merely mentioning the flag in prose would be selected and then fail the job for not accepting it. Now matched on the argparse registration, either quote style. Verified by extracting the step's script out of the workflow YAML with a parser and running it verbatim, rather than re-typing it.

No current false positive existed — the reviewer enumerated every .py in both directories and only this harness matches — so this is prophylactic.

Traced and clean

  • Pinning is complete. Every remaining min(/max( in the file is over per-worker lists, a modal-holder count, a clamp, or a sorted() inside an assertion message. No pair, median, decomposition or printed statistic derives from --widths ordering. one_launch's order = widths if launch % 2 == 0 else reversed(widths) is launch-parity anti-bias, not a comparison pair.
  • The fixture genuinely straddles the threshold, recomputed by hand from the constants: pinned rise 0.173 − 0.082 = 0.091 < 0.10 so DISPATCH must not fire; re-based rise 0.173 − 0.003 = 0.170 > 0.10 so it would. The test is not vacuous.
  • Every number in the doc and both commit messages reproduces exactly from a replay of the two archived datasets, including the "published" w=2 column re-derived by simulating the defect.
  • The doc's Amdahl argument is arithmetically sound against barrier_decomposition's own ratio = wide/narrow. One caveat the reviewer caught and I am recording rather than silently keeping: the doc writes "0.997 extrapolates to 0.021", but 0.997 alone recomputes to 0.024 — the 0.021 comes from the full-precision median 0.99730. Both figures are the tool's own output; the rounding in the prose is display-only.
  • set -euo pipefail interactions are safe: the missing nullglob is covered by [ -f "$f" ] || continue, and found=$((found + 1)) is an assignment, not the ((found++)) arithmetic-command gotcha.
  • Path filters do trigger this job on both pull_request and push to main — the PR touches crates/onnx-runtime-ep-cpu/benches/** and .github/workflows/hostlock.yml.
  • report()'s narrow, wide = pair cannot see a None: it is behind if not m: return, and m is non-empty only when rule_pair returned a pair.
  • Only stdlib imports; the sibling acc0_gap_matrix has no import-time side effects, so the CI self-test runs cleanly from the repo root with no lock and no host load.

Marking ready for review. This is not one of the required checks and must not become one; Host lock will run on this PR because of the path filter.

@justinchuby
justinchuby marked this pull request as ready for review August 27, 2026 01:05
justinchuby added a commit that referenced this pull request Aug 27, 2026
… the prior A/A-null result

Independent review found four precision defects in the replacement text, which
would have been self-defeating in a change whose whole point is that a comment
must say what was measured.

  * "CI [0.960, 1.013]" was the raw min-max of 7 paired ratios, not a
    confidence interval. A t-based 95% CI on the same data is [0.978, 1.008].
    Now stated as a range with its n.
  * "the median worker parks 0.533 times" conflated two aggregations: it is the
    median over launches of the mean over workers.
  * "on either baseline" referenced #2245's baseline bug, which is not context
    a reader of this file has. Dropped; "at either blocktime" carries it.
  * "~5% of pool width" was the low end. It is ~5.6% at t=8 and ~8% at t=16
    (1.3 of 16 cores). Now "~5-8% ... ~1.3 cores at width 16".

The review also surfaced a prior result I had not credited, and it corrects my
framing rather than merely my wording. `CPU_MATMUL_ASSIGNMENT.md` already
records removing the ramp as throughput-neutral at t=16 -- ratio 0.9960 against
a 5.24% A/A null. So the throughput half is not "unresolved" as I wrote: at zero
gap it is measured neutral, with the control my own batch lacked.

What is actually unmeasured is narrower and more pointed: every result on both
sides is a zero-gap decode loop, which is exactly where parking looks falsely
free, and the window's justification is inter-token gaps. The comment now says
that, and names #1395's gap-aware harness alongside #2071's arms.

fmt clean, clippy -D warnings clean, rustdoc warning count unchanged at 112.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@justinchuby
justinchuby merged commit 0feb180 into main Aug 27, 2026
32 of 33 checks passed
@justinchuby
justinchuby deleted the squad/seb-worker-split-rule-pair branch August 27, 2026 03:18
justinchuby added a commit that referenced this pull request Aug 27, 2026
…aim with the measurement (#2249)

## The sentence

`DEFAULT_BLOCKTIME`'s doc comment said:

> Parking inside the active path would pay a futex wake (~1-5 us) on
every barrier and **tank throughput**.

That is the sentence a future reader uses to decide not to re-measure
the spin window. It is not supported, and I am the one who should have
caught it earlier — #2071 has been open on exactly this question.

## What the measurement says

`benches/acc0_w16_worker_split.py`, 7 trusted launches per arm, width
16, host lock held:

| | `blocktime=0` | `blocktime=500` (shipped) |
|---|---:|---:|
| `parks_per_worker`, median | **345.9** | **0.533** |
| wake latency, share of window | **0.022** | 0.014 |
| useful work | 0.795 | 0.819 |
| straggler wait | 0.067 | 0.064 |

Three separable claims, and they do not all land the same way:

1. **The window eliminates parking during a generation. TRUE, and by a
wide margin** — 345.9 parks per worker per launch becomes 0.533. The
first-order claim is exactly right and the comment keeps it, now with
the number.
2. **Parking costs a lot in wake latency. FALSE.** Parking on
essentially every barrier costs **2.2% of the window**, at width 16,
with a profiler exposing it. The pre-registered `WAKE-BOUND` verdict
fires at **neither** blocktime on **either** baseline. This falsifies a
hypothesis of my own, not just the comment's.
3. **Removing the window would tank throughput. UNRESOLVED — which is
different from small.** The unprofiled paired A/B reads a clean null at
t=2 (0.996, CI [0.960, 1.013]) and **straddles 1.0** at t=8 and t=16,
and I ran **no A/A null** in that batch. I cannot claim a direction, in
either direction. The comment now says so instead of asserting a cliff.

## What the comment says now

The window is retained on the evidence that exists — it costs **~5% of
pool width in permanently-occupied cores** (#2071) and buys a wake term
that is small but real — and **not** on a throughput cliff nobody has
demonstrated. It names what a default change would require: #2071's
gap-distribution and concurrent-session arms, plus the missing A/A null.

**No behaviour change. No default change proposed.** `DEFAULT_BLOCKTIME`
stays 500 µs. I have not earned a change and say so in the comment.

## Also recorded: why the obvious test does not exist

The comment now cross-references the assertions that *do* cover the
behavioural half without timing —
`an_idle_gap_far_longer_than_the_blocktime_parks_the_workers` and the
`spin_hits + parks == dispatches * workers` identity on `SpmdCounters` —
and records why the complementary direction is deliberately **not** a
test: a descheduled worker's window expires off-CPU, so "back-to-back
dispatches never park" would fail on a loaded runner. That reasoning was
already in the test's own doc comment; it now also sits next to the
constant, so the 0.533 reads as an out-of-band substitute for a test
that cannot exist, not as a number somebody forgot to assert.

## Validation

Comment-only diff, one file.

- `cargo fmt --all --check`: clean
- `cargo clippy -p onnx-runtime-ep-cpu --all-targets -- -D warnings`:
clean
- `cargo doc --document-private-items`: warning count **112 before, 112
after** — measured by building both trees, not assumed, because the
change adds three intra-doc links
- `cargo test -p onnx-runtime-ep-cpu --lib decode_spmd`: **135 passed /
0 failed**, and the cross-referenced test runs and passes by name

## Provenance caveat

The numbers above come from datasets whose *verdicts* I initially
published against the wrong baseline (#2245 pins the pre-registered
comparison pair, which the harness had been taking from `--widths`).
Every figure quoted in this comment is baseline-independent —
`parks_per_worker`, `wake_frac` and the fact that `WAKE-BOUND` never
fires are all per-width medians, not deltas against a baseline — and all
of them were re-checked against the corrected replay. The
`docs/benchmarks/` record referenced from the comment lands with #2245.

Refs #2071 #2245

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
justinchuby added a commit that referenced this pull request Aug 27, 2026
…nd label the arm it ran (#2268)

Refs #2071. Instrument only — no production code, no benchmark claim in
this PR.

## Why

#1887 concluded the width-16 barrier wall could not be helped:

> `DEFAULT_STEAL_TILES_PER_WORKER = 1` makes `target == total_workers`,
so `work_stealing_segments_aligned` always falls back to static equal
segments — there are no spare tiles to steal.

That is no longer true. The native default is **4** since #2168.
Re-running the decomposition on today's `main` gives **straggler wait
0.064** at w=16 against #1887's **0.222**, with the w=8 control barely
moving (0.031 → 0.027) — but across 25 commits, so **nothing is
attributed yet**. Details in [this comment on
#2071](#2071 (comment)).

This turns the attribution into a **single-binary paired A/B** instead
of a cross-commit rebuild.

## Why the decomposition can answer what throughput could not

Worth stating precisely, because it is the reason this is cheap now and
wasn't in August. #1887 measured tiles 1→2 at +23% at t=16 with the
predicted mechanism and **correctly rejected it**, because the t=16
throughput A/A null in that same run was **±21.5%**. No realistic effect
clears that bar.

The barrier decomposition is not a throughput ratio. It is a
**within-launch structural measurement** of where the mean worker's
window goes, and it separated 0.222 from 0.064 on 7–10 trusted launches.
So the mechanism question is answerable even though the throughput
question was not.

## The change

`--steal-tiles N` sets `ONNX_GENAI_CPU_DECODE_STEAL_TILES_PER_WORKER`.
**Unset leaves the shipped default** rather than passing the default's
current value — the latter looks identical today and silently pins the
arm to a stale number the day the default moves.

The arm label is **recorded per launch and printed in the report
header**. An arm label that exists only in the operator's shell history
is exactly the defect #2245 fixed, one field along. Three states,
deliberately not collapsed:

| state | label |
|---|---|
| `--steal-tiles 1` | `1` |
| unset | `default (shipped, unset)` |
| key absent from an archived dataset | `unrecorded (dataset predates
the knob)` |

The third matters: my two archived datasets have no such key and ran at
**their** binary's default, which is not necessarily today's. Labelling
them `default` would assert a value nobody measured. Same doctrine as
the absent-key handling in `hostlock.sh` (#2261) and as
`takeover`/`gate` there.

## Validation

`--self-test` extended to cover all three label states **and** that the
label reaches the report rather than merely being computable.
Mutation-proved both ways:

- drop the absent-key branch → `KeyError: 'steal_tiles'`
- drop the header print → `AssertionError: expected '# steal_tiles=1' in
the report header`

The self-test runs in CI via the `Harness self-tests` step in
`hostlock.yml`, which already path-filters on
`crates/onnx-runtime-ep-cpu/benches/**`.

## Not in this PR

The measurement itself. The host is shared and currently held; I will
run the paired arms under the lock with an **A/A arm in the same batch**
and post the result to #2071.

---------

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant