Skip to content

Unify benchmark timing contracts and add calibrated CI gates - #924

Open
jhinpan wants to merge 4 commits into
ROCm:mainfrom
jhinpan:feat/timing-umbrella
Open

Unify benchmark timing contracts and add calibrated CI gates#924
jhinpan wants to merge 4 commits into
ROCm:mainfrom
jhinpan:feat/timing-umbrella

Conversation

@jhinpan

@jhinpan jhinpan commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

The problem

FlyDSL times kernels in at least six hand-written CUDA/HIP event loops. They are not copies of each other — they measure physically different things, but nothing in their names or their output says which:

loop shape what it actually measures
one event pair around all N calls pipelined mean latency; launch overhead amortized
one event pair per call, one sync at the end per-call latency distribution
one event pair per call, torch.cuda.synchronize() after each isolated latency, synchronization included
flush_buf.zero_() before each call cold-L2 latency instead of warm-cache latency

On the same kernel these differ by multiples. Each is a legitimate measurement; comparing across them is not. Units drifted along with them — some helpers return ms, some us, some hand-roll * 1e3.

The CI side had the matching gap. The benchmark CSV kept only tbps/tflops to three decimals and discarded the raw latency it had just measured, so compare_benchmark.py could report a ratio but could never fail a run. A performance regression was invisible to CI, and nothing recorded how any number had been produced.

Measurements, raw samples, and the design rationale behind the choices below are in the timing audit Gist.

What changes

1. One canonical event timer, with the schedule as an explicit argument

flydsl.do_bench becomes the single eager CUDA/HIP event implementation. The four loop shapes above are now three named schedules plus a cache-policy flag:

  • isolated — sync after every call (the existing autotuner contract)
  • pipelined — one event pair around all iterations, one averaged sample
  • per_iter — one event pair per call, a single sync at the end

Physically impossible combinations are rejected at the entry point rather than returning a plausible-looking number: pipelined produces exactly one sample, so it accepts only statistic="mean" and refuses prep_fn / cache flushing. isolated deliberately keeps the full-device torch.cuda.synchronize() instead of end.synchronize(), because autotuned callables may enqueue work on streams other than the current one.

Results come back as BenchResult, which stores microseconds and carries the schedule, statistic, cache policy, warmup, and iteration count that produced them.

2. Equivalent loops become thin wrappers

bench_gpu_us_torch, bench_kernel_us, and the MoE-sorting / TDM-bandwidth / RoPE-cache / a8w4 timers delegate to do_bench while preserving their existing behavior, including bench_kernel_us's branch between pipelined-mean and per-iter-median-with-IQR.

Two adjacent fixes fall out of the audit:

  • bench_wallclock documented host dispatch time but included the final GPU drain in the measured window. The sync now happens after the timer stops, locked by a regression test.
  • MoE-sorting graph benchmarks now verify every meaningful output after replay (poison the buffers, replay, compare), so a graph that silently stops doing work can no longer post a good number. bench_kernel_us there is renamed bench_profiled_device_us to match what it measures.

3. Benchmark records carry their own measurement contract

run_benchmark.sh --output_csv keeps its existing throughput columns and appends avg_us, sample statistics, statistic / instrument / schedule / cache_policy / warmup / iters, and arch. Benchmarks emit a self-describing Benchmark contract: ... line that the parser prefers over its per-op fallback table.

The human-readable five-column table is byte-for-byte unchanged, because tag baselines and the dashboard parse it. The only edit there removes field truncation (%-22.22s to %-22s) so long keys survive intact.

The two inline heredoc parsers in run_benchmark.sh (~140 lines of regex) move into scripts/benchmark_log_parser.py, which makes them testable.

4. A calibrated regression gate

scripts/benchmark_compare.py lifts the relative-and-absolute rule that allreduce has used for a while into a shared helper, and allreduce now calls it. A row regresses only when it is both more than X% slower and more than N microseconds slower, so a 5 us kernel drifting 20% does not page anyone.

This is the part that changes CI behavior for everyone: the current vs main benchmark comparison now runs with --fail-on-regression and can turn a PR job red.

Scope of the gate is deliberately narrow, defined by the versioned allowlist in .github/benchmark_thresholds.json:

  • gfx942 and gfx950, softmax / layernorm / rmsnorm at 32768x8192 bf16 — three rows per arch
  • 20% relative plus a 10 us absolute floor, chosen above the documented ~14% gfx950 clock-variation band
  • gfx1201 is declared with an empty rule list: reported, never gating
  • everything not in the allowlist stays report-only

The gate compares raw avg_us, not TB/s. Throughput is derived — change the bytes formula and the number moves without the kernel moving.

Fails closed (a broken measurement must not become a way past the gate):

  • baseline has raw latency but current does not
  • the measurement contract tuple (statistic, instrument, schedule, cache_policy, warmup, iters) differs between the two sides
  • an allowlisted row disappears from the current run

Fails open (no false alarms from missing history):

  • baseline predates the enriched CSV, so it has no raw latency
  • architectures do not match

There is also a runner-trust check: when --arch is supplied, every row in both CSVs must report that architecture, otherwise the comparator exits 2 rather than gating gfx950 results against a gfx942 baseline.

Two workflow details make the comparison well-formed. The main-baseline worktree is given the current branch's run_benchmark.sh and benchmark_log_parser.py, so both sides emit the same schema. The pip-installed tag baseline cannot be patched that way, so benchmark_output_to_csv.py now emits the same 17-column header with placeholders — those rows land on [GATE SKIP: baseline has no raw us] instead of breaking the parse.

Finally, dashboard ingest accepts conclusion == "failure" in addition to "success". Without that, the first time the gate actually fires the dashboard would go blank exactly when the regression data is most useful.

What this PR does not change

  • Other instruments stay separate. run_perftest profiler attribution, host wall clock, graph replay, and distributed timing measure different quantities and are not folded in. Over-merging is what created the original problem; the docs now say so explicitly.
  • Legacy do_bench(fn, warmup, rep, quantiles) keeps its scalar/list milliseconds, isolated schedule, and median statistic.
  • The five-column stdout table keeps its exact format.
  • No kernel changes. Nothing in this PR is expected to move a performance number; the CI run should show flat deltas with populated contract metadata.

Reviewer guide

Suggested order, roughly one concept at a time:

  1. python/flydsl/autotune.py — the timer, the schedules, BenchResult, and the rejected-combination guards
  2. tests/unit/test_benchmark_timer.py — the semantics the timer is now pinned to
  3. tests/kernels/benchmark_common.py — what a migrated wrapper looks like
  4. scripts/benchmark_log_parser.py and scripts/benchmark_output_to_csv.py — how the contract reaches the CSV
  5. scripts/benchmark_compare.py, scripts/compare_benchmark.py, .github/benchmark_thresholds.json — the policy and its calibration
  6. .github/workflows/flydsl.yaml — where the gate is wired and why both baselines get the same parser
  7. remaining tests/ files — mechanical migrations

Test plan

  • 94 unit tests across the timer (test_benchmark_timer.py), comparator (test_benchmark_compare.py), parser (test_benchmark_log_parser.py), dashboard ingest, and existing autotune suites
  • host-dispatch regression test asserting the final drain sits outside the measured window
  • real GPU smoke tests: legacy call shape, all three schedules, explicit streams, L2 flush, every migrated wrapper, and full-output graph verification
  • real gfx950 softmax benchmark produced the enriched CSV and passed the calibrated hard gate end to end
  • bash -n scripts/run_benchmark.sh, workflow YAML parse, threshold JSON parse, git diff --check
  • Ruff on all changed files

Follow-ups

  • Thresholds must be recalibrated from CI history before any row is added to the allowlist; the rationale for the current numbers is recorded in the calibration field of the JSON.
  • Remaining reported / unknown contract values in the CSV mark benchmarks that have not yet been migrated to a declared schedule.

Notes

A repository-wide Ruff run still reports one pre-existing import-order issue in tests/unit/test_cluster_mcast_gemm_gfx1250.py; no file changed by this PR has lint diagnostics.

jhinpan added 2 commits July 29, 2026 23:28
Move the final synchronization outside the measured window so the benchmark matches its documented host-dispatch semantics, and lock the ordering with a regression test.
Consolidate equivalent event timers behind the shipped do_bench implementation while preserving distinct profiler, host, graph, and distributed semantics. Carry raw latency and measurement contracts into CI so calibrated regressions fail closed without breaking existing dashboard output.
Copilot AI review requested due to automatic review settings July 30, 2026 00:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

jhinpan and others added 2 commits July 30, 2026 00:42
Apply the repository Black configuration so the focused style check accepts every Python file touched by the umbrella PR.
@jhinpan

jhinpan commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator Author

cc @coderfeli This PR is trying to unify benchmarking wit halso adding calibrated CI gates. Could u help review if u get some time~ But not that priority. Thx!

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.

2 participants