Skip to content

test(EPv2): make the routing independent of the sweep list, and report the spread - #659

Open
jhchouuu wants to merge 3 commits into
ROCm:mainfrom
jhchouuu:bench-audit-fixes
Open

jhchouuu wants to merge 3 commits into
ROCm:mainfrom
jhchouuu:bench-audit-fixes

Conversation

@jhchouuu

Copy link
Copy Markdown
Collaborator

Two defects found by auditing the benchmark against its own output, both in how it reports rather than in what it measures.

The routing still moved with the sweep list

_data.py split the payload and routing generators precisely so that changing SWEEP could not resample the routing. That fixed the payload half. But wts is (M, TOPK) with M = max(SWEEP), and it was drawn from the routing generator before idx — so how much randomness it consumed, and therefore where idx started, still depended on the sweep list.

TOKENS=512 and TOKENS=64,...,16384 disagreed on all 512 rows of the 512 tier. The visible symptom was recv~1688 from one and recv~1693 from the other for what is nominally the same point; the per-rank load differs by 1.7% on the slowest rank, which is the rank that sets the time.

idx consumes exactly one randperm per row, so drawn first its first ct rows are identical for any M. Verified on 4x MI4xx: the 512 tier now reports recv~1702 from both sweeps.

The mean was the only statistic, and it is the least stable one

Measured on 4x MI4xx, ITERS=200:

mean min p50 p95
ct=256, six repeats 55.8 → 63.2 (13%) 53.1–54.4 (1.2%) 54.9–60.3 61.6–71.0
ct=4096, four repeats 308–313 244.6–247.6 278–285 502–602

The mean sits 10–26% above its own min and has a long right tail — at 4096 tokens p95 is 1.8x p50. Reporting it alone means a reader cannot tell a clean point from a polluted one. That is not hypothetical: a bf16 sweep taken for the micro-bench image's README read 190 us at a tier that sits at 66, and nothing in the output distinguished it from a good point. Its min would have.

The per-iteration deltas already existed and were being discarded. Now min trails the human line and all three quantiles go into the JSON row:

ct=256   [hip/graph] dispatch  58.6 us ( 208.1 GB/s)  combine  43.1 us ( 283.5 GB/s)  pair  101.7 us  recv~851  min 52.9/36.8

The mean stays the headline — it is the right number for what a layer costs with the tail included. min is there so the mean can be audited.

Every statistic is reduced across ranks the same way, so the reported min is the mean of each rank's own minimum, not the global minimum (which would present one lucky rank as the collective).

Scope

Neither change alters what is measured — only the routing sample drawn and what is printed alongside the mean. Previously published latencies and ratios are unaffected.

Also checked, and NOT changed

Measured rather than assumed, so they are recorded here:

  • Cross-rank mean vs max. Per-rank spread is 1.0–3.3% across every tier from 64 to 16384, so averaging costs at most ~3% against taking the slowest rank. Not worth the churn.
  • torch.cuda.Event lazy initialisation. The docs are clear that the underlying event is created on first record(), and all 800 records happen inside the timed loop — but an A/B against a pre-warmed variant showed no effect (52.4/55.4 vs 54.9/54.9 at ct=64, inside run-to-run noise). The host runs far enough ahead that event creation never starves the GPU.
  • Whether the tail biases the wire comparison. It does not: at 4096 tokens bf16 and fp4 both sit ~+22–23% above their own min, so mean-based and min-based ratios agree (2.71x vs 2.70x).
  • The ev[1]/ev[2] gap. Both events are enqueued back-to-back before run_c(), so combine's host submission latency lands inside ev[2]→ev[3]; d + c does equal ev[0]→ev[3].

…t the spread

Two findings from auditing the bench against its own output.

The routing still moved with SWEEP. Splitting the payload and routing generators
fixed the payload half of that, but wts is (M, TOPK) with M = max(SWEEP) and it
was drawn FIRST, so how much randomness it consumed -- and therefore where idx
started -- depended on the sweep list. TOKENS=512 and TOKENS=64,...,16384
disagreed on all 512 rows of the 512 tier, worth 1.7% of the slowest rank's
load; the tell was recv~1688 against recv~1693 for what should have been the
same point. idx consumes exactly one randperm per row, so drawing it first makes
its first ct rows the same for any M. Verified: the 512 tier now reports
recv~1702 from both sweeps.

The mean was the only statistic reported, and it is the least stable one here.
Over six repeats of one tier the mean moved 13% while the min moved 1.2%, and
the mean sits 10-26% above its own min because the distribution has a long right
tail (at 4096 tokens p95 is 1.8x p50). Reporting it alone means a reader cannot
tell a clean point from a polluted one -- a sweep taken for the image README read
190 us at a tier that sits at 66, and nothing in the output said so. The
per-iteration deltas already existed and were being thrown away, so min/p50/p95
now come with the mean: min on the human line, all three in the JSON row. Every
statistic is reduced across ranks the same way, so min is the mean of each rank's
own minimum rather than one lucky rank's.

The mean stays the headline. It is the right number for what a layer costs, tail
included; min is there so the mean can be audited.

Neither change alters what is measured, only the routing sample and what is
reported alongside it. Verified on 4x MI4xx EP4.
… looks

min was on bench_ep.py's own line, but the driver reprints its own table from a
regex and only carried the mean across -- so the one-shot output, which is what
anyone actually reads, still showed a single unauditable number. The min was
only in the log file the run points at.

Also adds max. It is the noisiest of the five and must not be compared between
runs, but it is the only one that shows an isolated stall: two of six repeats of
one tier hit 203 and 241 us against a 59 us median, which moved the mean by
0.7 us and was otherwise invisible.
The driver reprints its own table from this line, so a statistic that is not on
it cannot reach the table the reader actually sees.
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