fix(bench): restore /bench-growth rendering, the fixture digest check, and the measured costs - #887
fix(bench): restore /bench-growth rendering, the fixture digest check, and the measured costs#887MauroToscano wants to merge 11 commits into
Conversation
… run The Compare step stopped emitting the eight `*growth*` step outputs that the Comment step reads, so `/bench-growth` proved the growth sweep and rendered nothing: PR_GROWTH_HEAPS arrived empty and the whole Memory Growth section is keyed off it. Restore them. shellcheck had been calling the corresponding BASELINE_GROWTH_* shell variables unused, which is the same break seen from the other end; those three warnings are now gone. render_bench_comment.js could not catch this — its scenarios hand env to the comment script directly, which is why the growth table rendered there while CI could never produce it. Give it a wiring check that walks the workflow's steps and fails when a consumed `steps.X.outputs.Y` has no producer. Scenario C also fed a pre-signed REAL_TIME_DIFF and rendered `++41.400s`; the Compare step prints diffs unsigned and the renderer adds the sign. The three benchmark scripts wrapped `make ethrex-real-block-fixture` in a file-existence test, which skipped the digest check in exactly the case it exists for — a fixture already on disk from an earlier block or an interrupted fetch. Call it unconditionally; a matching digest costs ~35 ms. The /bench-gpu ETA scaled at 2 min per pair, but a pair is two proves. The measured 4-pair run was 20 min 11 s: 3 min 56 s of overhead plus 4.06 min per pair, so the slope is 4.
The CPU real-block prove cost was derived, not measured: the bench runner's 5.31-5.62 s/Mcycle synthetic rate at epoch 2^21 was multiplied by the block's 50.78M cycles to give ~4.5-4.8 min, and that number then spread across four files. The runner has since been measured on the block itself at epoch 2^22 — 158.8 s median, 2.8% spread over 3 runs, 13 epochs — which is 3.13 s/Mcycle and 2.65x the calibration RTX 5090's 59.87 s, not 4.5-4.8x. Every site now quotes that: /bench is ~8 min of proving in a ~15 min occupancy, perf_diff.sh ~13 min, a 20-pair real ABBA ~1.8 h (was given as both ~4 h and ~3 hours). Memory was being copied across machines the same way. The 32.21 GiB at 2^22 is the calibration box's peak RSS; the bench runner measured ~52 GB of heap for the same block and epoch. Label each figure with the box it came from, and say which build a host-RAM number is for — the CUDA path peaks at ~36 GB, the CPU build at ~32 GiB, and the GPU workflow cited the latter as ~28 GiB. Other measured corrections: - the continuation bundle is 1.15 GB on CPU / 1.12 GB on GPU, not ~1.9 GB (a 2^21-era figure), so a block must be ~1.7x heavier to reach the 2 GiB pointer_width_64 threshold, not ~1.05x - ECSM per Mcycle is 9.16 synthetic against 2.28 real; the old 8.83 vs ~1.3-1.6 pair was computed on superseded pre-LTO cycle counts - "keccaks per ecrecover" counted keccaks per ECSM call, and an ecrecover is four ECSM ecalls, so the row is relabelled and the per-ecrecover figures (20.6 and 361) spelled out - "gas does not order cost" contradicted its own table, which has gas ordering all three candidates correctly. What the data shows is that gas does not size cost: 29.7 to 38.2 cycles per gas on one common ELF - the CPU-server epoch was marked pending calibration two sections above the completed calibration table
…reen The real-block fixture is downloaded from a release asset and checked against a sha256 in the Makefile, but several places still described it as converted locally from an ethrex-replay cache. executor/tests/README.md also named a file no target produces (ethrex_hoodi_1265656.bin — that is the converter's own pinned TEST block) and pointed at the wrong place for the checksum. Corrections to comments that named the wrong thing: - caches/ is populated by ethrex-real-block-cache and the converter's test-cache rule, not by ethrex-real-block-fixture, which writes to executor/tests/ - the k256 feature pin is what keeps `cargo run` and `cargo test` on one resolution; the callers are regen-real-block-fixture and test-ethrex-real-block-converter, not the fixture fetch - no workflow invokes `make test-ethrex`: the PR gate spells out the -offline variant inline, and the converter workflow runs the usability screen directly - the repoint block is six variable lines, not five - print-real-block-fixture's caller list was missing scripts/bench_abba.sh - the benchmark cache is read only by regen-real-block-fixture; the converter's tests read a different, upstream-pinned one benchmark-pr.yml still described the synthetic screen as running alongside the real block and as something to degrade to. It proves one workload; the growth sweep is separate and run_growth-gated, and on a plain /bench with no fixture the footer already says nothing was measured. The comment sections are numbered 1 and 2, and the cold-baseline real-block loop samples $RUNS, not 1. tooling/ethrex-block-converter/** is dropped from the push-to-main trigger paths. It was there so a repoint could not leave main's baseline stale, but CI never runs the converter — it fetches a .bin pinned by URL and sha256 in the Makefile, which is still a trigger path. A converter change cannot alter the bytes proven, so triggering on it only spent ~15 min of the shared bench runner republishing an identical baseline.
|
/review-ai |
Codex Code Review
|
|
Review Verified the three functional fixes against the checked-out base plus the diff. All three are correct, and the arithmetic in the numbers section holds up. Functional fixes — all sound Growth outputs. Confirmed the break: The wiring check. I walked it against the post-PR workflow. The digest guard. GPU ETA. Numbers — spot-checked, all consistent 158.8/50.78 = 3.13 s/Mcycle; 158.8/59.87 = 2.65x; 3 runs = ~8 min; 5 = ~13 min; 20 pairs x 2 proves = 1.76 h. ECSM/Mcycle 80/8.73 = 9.16 and 116/50.78 = 2.28. The keccak correction checks out in code — Also confirmed the doc claims that assert something about the repo: the repoint block really is six lines now, no workflow invokes Dropping Findings Three, none blocking — one Medium and two Low, inline. No safety, memory, or correctness issues; nothing here touches the VM, the executor, or any GPU code path. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # pr_growth_heaps, so these pass through even when only one side ran. | ||
| { | ||
| echo "pr_growth_heaps=$PR_GROWTH_HEAPS" | ||
| echo "pr_growth_times=$PR_GROWTH_TIMES" |
There was a problem hiding this comment.
Low — two of the eight restored outputs feed dead reads.
pr_growth_times and baseline_growth_times land in the Comment step's env (lines 720/724) and are read into prGrowthTimes (756) and baseGrowthTimes (760) — and those two consts are never referenced again anywhere in the renderer. So this restores wiring for a value nothing renders.
Either drop the pair (2 outputs here, 2 env: lines, 2 consts) or put the times to work — a prove-time column next to the heap column in the growth table is the obvious use, and the data is already being collected.
Worth noting the asymmetry while you're here: the new wiring check catches consumed-without-producer but not produced-without-consumer, which is the shape of this one and of pr-growth's growth_steps (produced at line 357, never consumed; the renderer hardcodes labels = ['4','8','12','16','20'] at 852 with a "keep in sync" comment instead).
| itself measured **~52 GB** of peak heap for the same block and epoch. See | ||
| [Choosing the epoch size](#choosing-the-epoch-size) for the full curve and the other | ||
| tiers. The bundle on disk is ~1.15 GB (1.12 GB on the GPU path); a block would have to | ||
| be ~1.7x heavier to push it past 2 GiB, which needs rkyv `pointer_width_64` to |
There was a problem hiding this comment.
Low — the 1.7x uses 2 GB where the sentence says 2 GiB.
rkyv's 32-bit offset limit is 2 GiB = 2.147 GB, so from a 1.15 GB bundle the headroom is ~1.87x (1.92x from the GPU path's 1.12 GB). ~1.7x is what you get from 2.0/1.15.
Given this PR is specifically about numbers that were carried across contexts without being re-derived, worth making this one exact — ~1.9x against 2 GiB, or say 2 GB and keep 1.7x.
bench_verify.sh, bench_abba.sh and perf_diff.sh defaulted to the synthetic N-transfer fixture, which /bench no longer proves. The default is now `real`, matching /bench and /bench-gpu, so a screen and the tool used to chase its result measure the same block; WORKLOAD=synthetic remains as an explicit opt-in for reproducing a number recorded against that fixture. bench-verify.yml pins WORKLOAD=synthetic rather than inheriting the new default. It reports a monolithic arm alongside the continuation one, and the real path runs only the latter, so inheriting would silently drop an arm and break comparability with every verify number recorded so far. bench_abba.sh's sizing table is restated for the real block, from the paired-t resolvable delta rather than the old synthetic power figures. The pair-delta sd on the bench runner is not measured yet, so the table brackets it between 1.0% and 2.0% and says where to read the real value off the output.
/bench proved a real block while its tiebreaker proved 100 synthetic transfers, so escalating from an unresolved /bench reading measured a different workload than the one that raised the question. /bench-abba now proves the same block at the same epoch (2^22, pinned here as /bench pins it — the script's own default of 2^20 is the laptop-safe one, not the runner's tier). The default drops from 20 pairs to 12: at 2 x 158.8 s per pair that is ~72 min rather than the ~4.5-5 hr a cont100 run cost, and it resolves 1.3% at the pessimistic end of the pair-delta sd bracket. The resolution table in the header gives the other rungs, brackets the unmeasured sd, and says where to read the real value once a run has produced one. The syntax is now a pair count and nothing else. The cont[TX]/mono[TX] tokens selected between synthetic fixtures that this job no longer proves, and the rkyv pointer_width_64 fail-fast went with them: it fired on >=40tx continuation proofs exceeding the old 2 GiB cap, and the real block's bundle is 1.15 GB. The timeout drops from 720 to 360 minutes, still a hang guardrail rather than an expected duration: ~3.7 hr for the 40-pair clamp plus a cold two-sided build.
/bench's unresolved-band verdict pointed at /bench-abba without saying what it would measure or what it would cost — which mattered, because until now it measured a different block. Both flows prove the same block at the same epoch, so the comment now says so and quotes the tiebreaker's default cost and resolution: 12 pairs, ~72 min, ~1%. The converter README gains the resolution table behind that number, with the sd bracket it rests on and a note that the first run measures the real value. Its bench section also lists /bench-abba alongside /bench and /bench-gpu, drops the WORKLOAD=real prefixes now that real is the scripts' default, and records that /bench-verify is the one flow pinning synthetic.
AI ReviewPR #887 · 13 changed files FindingsNo non-rejected structured findings were reported. Reviewer Lanes
Verification Lanes
Native Codex and Claude reviews run separately and post their own comments. They are not included in this structured provenance report. Discarded candidates (3) — rejected by the verifier
Raw lane outputs, candidates, final issues, and model metrics are uploaded as workflow artifacts. |
A consumed steps.X.outputs.Y passed silently when X was not among the producers the check had enumerated — a typoed step id, or a run: step that never writes $GITHUB_OUTPUT, satisfied 'every consumed output has a producer' vacuously. The check now classifies every step id: unknown ids fail, run: steps with no outputs fail on any read, and uses:/dynamic steps are skipped as statically unverifiable (native action outputs cannot be enumerated from the workflow text). Verified by typoing a live reference: 'no step with id zzpr-ref' fires; the untouched suite still passes.
… comment The fixture-guard comment claimed the comment renders the growth sweep on push and workflow_dispatch; the Comment step is gated to comment triggers on those events, so the sweep's output goes only into the baseline artifact — which is what the render harness's own scenario annotation already said.
perf_diff.sh's missing-fixture error said to run bench_abba.sh — whose default workload is now real, so following it literally fetches the real fixture and leaves the synthetic path still missing its input. The message now pins WORKLOAD=synthetic.
The ~1.7x came from dividing decimal 2.0 GB by the 1.15 GB bundle while the sentence names the binary 2 GiB rkyv offset limit.
The wiring check existed but nothing executed it — the guard against the orphaned-step-output class that silently broke /bench-growth only ran when a developer invoked it by hand. It now runs in the required Lint job: pure node, no dependencies, ~10 s, and deliberately unfiltered since any of several files can break the wiring.
1 participant
Review follow-ups to #884, which merged before the review landed. Three functional fixes, then the numbers and text they exposed.
Functional
/bench-growthrenders nothing. The Compare step stopped emitting the eight*growth*step outputs the Comment step reads, soPR_GROWTH_HEAPSarrives empty and the whole Memory Growth section — table, slope, R², scaling verdict — is dead. The sweep still proves 5 programs and reports none of it. This is live onmain. Restored; theBASELINE_GROWTH_*shell variables that feed them were already there, which is why shellcheck had been flagging three of them as unused.render_bench_comment.jscould not catch it — its scenarios hand env to the comment script directly, so the growth table rendered there while CI could never produce it. It now checks wiring first: it walks the workflow's steps and exits non-zero when a consumedsteps.X.outputs.Yhas no producer. Removing any one of the restored lines reproduces the failure.The fixture digest guard was bypassed.
bench_verify.sh,bench_abba.shandperf_diff.sheach wrappedmake ethrex-real-block-fixturein[ ! -f "$INPUT_REL" ], skipping the check in exactly the case it exists for: a fixture already on disk from an earlier block, a re-upload under the same block number, or an interrupted fetch. That is the file-rule semantics the Makefile made the target phony to avoid. Called unconditionally now — a matching digest is a ~35 ms no-op./bench-gpuposted an ETA at half the real slope.12 + PAIRS*2, but a pair is two proves. The measured 4-pair run was 20 min 11 s: 3 min 56 s of overhead plus 4.06 min per pair. At the default 14 pairs it promised ~40 min for a ~61 min run.Numbers
The CPU real-block prove cost was derived, not measured: the runner's synthetic 5.31–5.62 s/Mcycle rate at epoch 2^21, multiplied by the block's 50.78M cycles, giving ~4.5–4.8 min — which then spread across four files. Measured on the block itself at 2^22 it is 158.8 s (median of 3, 2.8% spread, 13 epochs): 3.13 s/Mcycle, and 2.65x the calibration RTX 5090's 59.87 s rather than 4.5–4.8x. So
/benchis ~8 min of proving,perf_diff.sh~13 min, a 20-pair real ABBA ~1.8 h — that last one had been stated as both ~4 h and ~3 hours in the same document.Memory was being copied across machines the same way. The 32.21 GiB at 2^22 is the calibration box's peak RSS; the bench runner measured ~52 GB of heap for the same block and epoch. Each figure now says which box it came from, and host-RAM numbers say which build they are for — the CUDA path peaks at ~36 GB, the CPU build at ~32 GiB, and the GPU workflow was citing the latter as ~28 GiB.
Also corrected: the continuation bundle is 1.15 GB / 1.12 GB, not ~1.9 GB (a 2^21-era figure that made the 2 GiB
pointer_width_64threshold look ~1.05x away instead of ~1.7x); ECSM per Mcycle is 9.16 synthetic against 2.28 real, the old pair having been computed on superseded pre-LTO counts; and "keccaks per ecrecover" was counting keccaks per ECSM call, which is 4x off, sincelincomb2_with_oracleissues fourecsm_mulecalls per signature."Gas does not order cost" contradicted its own table, where gas orders all three candidates correctly. What the data shows is that gas does not size cost — 29.7 to 38.2 cycles per gas on one common ELF.
Text
The fixture is fetched from a release asset and checked against a sha256 in the Makefile, but several places still described it as converted locally from an ethrex-replay cache.
executor/tests/README.mdalso named a file no target produces (ethrex_hoodi_1265656.binis the converter's own pinned test block) and pointed at the wrong place for the checksum. Same class:caches/is not written by the fixture target, no workflow invokesmake test-ethrex, the repoint block is six lines rather than five, andprint-real-block-fixture's caller list was missingbench_abba.sh.benchmark-pr.ymlstill described the synthetic screen as riding along and as something to degrade to; it proves one workload, and the comment's sections are 1 and 2.One trigger change:
tooling/ethrex-block-converter/**is dropped from the push-to-main paths. It was listed so a repoint could not leave main's baseline stale, but CI never runs the converter — it fetches a.binpinned by URL and sha256 in theMakefile, which is still a trigger path. A converter change cannot alter the bytes proven, so triggering on it only spent ~15 min of the shared bench runner republishing an identical baseline.Validation
actionlinton both workflows (only removals vs. base — the three dead-variable warnings),shellcheck+bash -non all three scripts, the fullrender_bench_comment.jsscenario suite plus the new wiring check, andcargo test --releaseinethrex-block-converter(3 passed).Note by Mauro:
Triple reviewed by kimi, claude with review skill and fable, and the matrix of AIs, Opus, and Codex with different prompts