Skip to content

chore: regenerate benchmark artifacts only when cost tables drift - #3727

Draft
mmagician wants to merge 3 commits into
mmagician-claude/stabilise-bench-tx-keysfrom
mmagician-claude/regenerate-generated-on-drift
Draft

chore: regenerate benchmark artifacts only when cost tables drift#3727
mmagician wants to merge 3 commits into
mmagician-claude/stabilise-bench-tx-keysfrom
mmagician-claude/regenerate-generated-on-drift

Conversation

@mmagician

Copy link
Copy Markdown
Collaborator

Stacked on #3726. Review the top two commits.

Makes "regenerate only when the drift guard fails" the actual, stated policy. No new enforcement is added — the guard already exists (checked_in_note_costs_match_executed_scenarios). This stops the tooling and the docs from inviting regeneration the guard never asked for.

The sentence that caused the churn

bin/bench-transaction/README.md said a PR that "meaningfully changes cycle counts" must run the target and commit the updated tables, and sold the resulting diff as review signal. That is vague enough to invite regeneration on any cost-affecting change, when the guard only asks past 5%. #3699 regenerated both artifacts for a largest move of BURN at −3.1%, well inside tolerance: 906 lines of generated diff on top of ~250 lines of real change.

Replaced with the policy as it actually stands: feature PRs regenerate nothing, sub-threshold drift is absorbed, and when the guard fails both artifacts are regenerated and committed together.

make update-note-costsmake update-generated

The target regenerates two artifacts but was named for one, which is why contributors ran it for a cost-table refresh and got an 800-line bench-tx.json diff too. Behaviour is unchanged — one run, both artifacts.

Deliberately not split into table-only and snapshot-only targets. Under a both-or-neither policy a split has no purpose, and it would newly make it possible to commit one artifact without the other, a divergence that cannot happen today.

--release is new. The old target ran in debug, which for 43 scenarios each with a trace rebuild is a lot of wall clock for cycle counts that are identical either way.

The two generated table.rs headers are updated in place to match what the renamed generator emits; no constant changes. The one remaining update-note-costs reference in the repo is in the released v0.16.0 CHANGELOG section, left alone as shipped history.

Accepted gap

COMMITTED_SCENARIO_EXPECTATIONS in utils.rs checks padded trace brackets against the committed snapshot, which under this policy lags reality. A change pushing a scenario across a power-of-two padding boundary — a real proving-cost cliff, since proving cost is step-wise in padded trace length — can go unnoticed until a later table drift forces a refresh.

Cycle drift is not a proxy for this: a scenario sitting just under a bracket crosses it on a change far smaller than 5%. In the current snapshot the tightest margins are NETWORK_ACCOUNT_CONFIG at 8,082 of an 8,192 chiplets bracket (1.3% slack), PSWAP partial fill at 32,288 of 32,768 poseidon2 (1.5%), and CLAIM L2-with-fee at 64,045 of 65,536 core/range (2.3%). The window is not hypothetical.

Closing it needs fresh trace data on every PR, which is out of scope here.

Verification

  • make test-release: 1886 passed
  • make lint clean
  • make update-generated runs end to end; make update-note-costs no longer exists
  • No-argument cargo run --bin bench-transaction still writes the snapshot and does not touch the cost tables, so miden-vm's documented synthetic-bench workflow keeps working
  • Re-running the generator changes only range_rows on 7 scenarios, the run-to-run variance the README already documents

🤖 Generated with Claude Code

https://claude.ai/code/session_01LXcU9HrrmWTCTZTJZnuLSU

@mmagician
mmagician force-pushed the mmagician-claude/regenerate-generated-on-drift branch from 7e0d1fe to deb00c6 Compare August 25, 2026 13:48
@mmagician
mmagician force-pushed the mmagician-claude/stabilise-bench-tx-keys branch from d5e3327 to 632b757 Compare August 25, 2026 13:57
claude added 3 commits August 25, 2026 13:57
…lease

The target regenerates two artifacts, but its name described only one of
them, so contributors ran it expecting a cost-table refresh and got an
800-line bench-tx.json diff as well. Name it for what it does.

`--release` is new: 43 scenarios each with a trace rebuild is a lot of
wall clock in debug for cycle counts that are identical either way.

The generated table headers are updated in place to match what the
renamed generator now emits, so the constants are untouched.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LXcU9HrrmWTCTZTJZnuLSU
The README told contributors that a PR "meaningfully changing cycle
counts" must regenerate and commit the tables, and sold the resulting diff
as review signal. That is vague enough to invite regeneration on any
cost-affecting change, when the drift guard only asks for it past 5% - and
it is the direct cause of the generated churn on kernel PRs.

State the actual policy instead: feature PRs regenerate nothing,
sub-threshold drift is absorbed, and when the guard does fail both
artifacts are regenerated and committed together. `bench-tx.json` is
refreshed only as part of that, so it is not expected to be fresh in
between.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LXcU9HrrmWTCTZTJZnuLSU
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LXcU9HrrmWTCTZTJZnuLSU
@mmagician
mmagician force-pushed the mmagician-claude/regenerate-generated-on-drift branch from deb00c6 to 9a82da5 Compare August 25, 2026 14:01
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