perf(perf): key the base arm on its merge base and carry it between runs (CLOUD-1331) - #825
Conversation
CLOUD-1331 `perf pair` builds the base arm's whole release closure from nothing on every CI run — the base binary is a pure function of the merge base, so the `perf` job pays 13.5 min for a build no run can reuse
Why The **Measured 2026-09-02 from two **
For comparison the same runs' What the step does, read from the engine rather than the job. Where the cache reaches and where it does not. The base arm is a pure function of the merge base. Its inputs are the merge-base SHA, the pinned toolchain and Why the skip does not save this. CLOUD-875 widened Refinement — Ready (reuse the base arm across runs) Decided 2026-09-02, so the implementer does not choose: the base arm keeps its OWN target directory, keyed on the merge-base SHA, and is restored WHOLE from a cache keyed on that SHA plus the toolchain hash. Closure sharing between the two arms is NOT taken — the Refinement gate: Definition of Ready & Done. This body carries only specializations.
Acceptance
Found while grooming the CI-cost dispatch: measured against the runs above, |
|
Warning Review limit reachedNext included review available in 36 minutes. View limit detailsLimit details: You’ve used the included review currently available. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (3)
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Essentials by visiting https://app.coderabbit.ai/settings/billing. Comment |
`perf pair` built the merge base's release binary from nothing on every CI run: 13.5 min of the `perf` job's 13.7 (CLOUD-1331, measured over runs 33586699312 and 33584118886). The base binary is a pure function of the merge-base SHA, the toolchain and `[profile.release]`, and `main` advances only by fast-forward to already-judged SHAs — so consecutive pull requests share a merge base for hours and every one of them was compiling the identical binary. `rust-cache` was already carrying the bytes: its cache paths are the whole of `target/`, `target/perf/**` included, and the post-job cleaner walked `target/perf/pair/base-tree/...` on both measured runs. `perf::out_dir` `remove_dir_all`s `target/perf/pair/` at the start of every run and the base target directory lived inside it, so the restore was thrown away microseconds before the base build ran. The key alone would not have been enough. - `base_target_dir` is `target/perf/base-<merge-base-sha>`, a sibling of the per-run `pair/` rather than a child of it. The SHA in the path IS the refusal: a directory built from another base does not answer to this name, so a stale arm cannot be measured as this one. - `measure` skips `build(&base_tree, ...)` entirely when that directory already holds a binary, and bails could-not-look if a build leaves none. - `build` drops `--quiet`, which suppressed every `Compiling` line and made the row's own acceptance reading answer `0` on a run that compiled the closure twice. Cargo writes progress to stderr, so `perf-gate.sh`'s `^arm=` contract over stdout is untouched. - The `perf` job restores and saves that directory under an `actions/cache` entry keyed on the same SHA plus the toolchain and lockfile hash, with no `restore-keys` — a prefix hit would put another base's build under this base's name. The measurement is unchanged: both arms still built or restored, sampled back to back on one machine, the ratio still `perf-compare`'s. Refs: CLOUD-1331 Admits: d7c27bde12b7fe89dce932035c38e953b155f388b91bca1a449d43dee5739fe2 Admits-rule: protected-mutation Admits-verdict: path write refused Admits-subject: .github/workflows/ci.yml Admits-head: ee1c9f5 Admits-epoch: 51eda204a7ea8a29b661ddfcd5867afc30106378a358de6e6d7f7b3b617edd2c Admits-author: alec@wenzowski.com Admits-prev: - Admits-answer-lost: The base arm's keyed target directory would be built and then thrown away on every CI run. The engine half of this change (a SHA-keyed `target/perf/base-<sha>` outside the per-run wipe) is inert without a cache entry that carries that directory between runs, so the 13.5 min the `perf` job spends rebuilding the merge base — measured on runs 33586699312 and 33584118886 — is unrecovered, and the row's acceptance ("a second run on an unchanged merge base compiles zero crates for the base arm") is unreachable. Landing only the Rust half would ship a mechanism with nothing driving it. Admits-answer-precondition: The class names no surface that can add a step to a workflow job: `.github/workflows/**` is protected precisely because CI's definition of green must change under review, and there is no batten verb, task or generated artifact that writes a job's step list. CLOUD-1331 §1 names `.github/workflows/ci.yml`'s `perf` job as this row's authority boundary, and the change is two steps around `mise run perf-gate` — a merge-base resolution and an `actions/cache` entry keyed on it. It lands in a draft pull request whose diff a reviewer reads before it can merge, which is the redirect's own stated remedy ("change it in a pull request"). Admits-answer-rejected-route: `config read first` does not apply: nothing in `batten.toml` decides which steps a workflow job runs, so reading the committed config answers a different question and leaves the job unchanged. `patch run first` was in fact taken — the edit was applied as a reviewed one-shot splice with a single-anchor assertion (refusing unless the anchor matched exactly once) rather than a freehand rewrite, and the result is `git diff`-visible as +38 lines in one job. What it could not do is make the write unnecessary; the protected path still had to receive the bytes, which is why this admission is being requested rather than avoided.
`no_artifact_name_reaches_the_core` caught the previous commit naming `.github/workflows/ci.yml` and a cache action in `base_target_dir`'s rationale. Non-negotiable rule 1: the core knows that a consumer's CI carries the keyed directory between runs; which file wires that up is the consumer's, and saying so here would make the crate un-adoptable for a consumer whose CI is spelled some other way. The reasoning survives without the paths, and the one thing a reader has to get right is stated more directly than before: the base is `base_commit`'s merge base, never a forge's base-branch tip, because keying on the latter names a directory this function never writes. Refs: CLOUD-1331
caf109e to
79dd7b2
Compare
|
❌ The last analysis has failed. |
|
/fast-forward |
1 participant
Closes CLOUD-1331.
perf pairbuilt the merge base's release binary from nothing on every CI run. Measured from the job-logs API on runs33586699312(ee1c9f52) and33584118886,mise run perf-gatewas 13m41s and 13m30s of a 13.7 / 13.8 min job — theperfjob is one of the two wall-clock poles of a pull request's checks, and almost all of it is one step.What the measurement actually said, and why the prescribed reading could not answer
The row asked for a
Compilingline count per arm. That count is 0 on both runs and discriminates nothing, becauseperf::build()passedcargo build --quiet, which suppresses every such line. A reading that answers0on a run compiling the whole closure twice, and would answer0again after any fix, is not a reading.The decisive evidence is elsewhere and is stronger than the one the row expected:
Swatinem/rust-cachedoes carry the bytes. Its printed cache paths are the whole of/home/runner/work/batten/batten/target—target/perf/**inside it — and the run restored 189 MB fromv0-rust-perf-perf-Linux-x64-22abc94d-db6ecef6. Its post-job cleaner then walkedtarget/perf/pair/base-tree/…(fourENOENTlines naming it), so the pair tree was in the save set.perf.rsdeleted the base target directory itself, before every base build.out_dir()istarget/perf/pairandremove_dir_alls it unconditionally;measure()put the base target atout.join("base-target")— inside the directory just wiped. Whatever the cache restored there was gone microseconds beforebuild(&base_tree, …)ran.So the arm was not merely un-restored, it was un-restorable at that path, and a cache key alone would have changed nothing.
The change
Three files, matching the row's §1 boundary exactly. No
mise-tasks/program and no.batsis touched;[profile.release]is untouched.crates/batten/src/perf.rsperf_dir()names the checkout-owned directory, distinguished from the per-runout_dir()by lifetime: everything underout_dirbelongs to one run and is deleted at the start of the next; the base build is a fact about a commit and outlives every run that reads it.base_target_dir()istarget/perf/base-<merge-base-sha>— a sibling ofpair/rather than a child of it. The SHA in the path is the refusal: a directory built from another base does not answer to this name, so a stale arm cannot be measured as this one. The discriminator is structural rather than a check somebody has to remember to write.base_arm_is_built()asks for a file, not a path that exists. This gate is killed routinely —landraces it againstmain-watch, and the harness kills a foreground command at ~2 minutes — so a directory left by a build that never linked is the ordinary case, and reading it as "built" would hand hyperfine a path it cannot execute.measure()skipsbuild(&base_tree, …)entirely when that binary is present, and bails could-not-look if a build leaves none. The base tree is still materialised on the reuse path, becausewired_commandreads the base tree's own settings file to derive that arm's invocation.build()drops--quiet, which is the measurement half rather than a taste: it is what makes the row's own acceptance reading takeable at all. Cargo writes progress to stderr, soperf-gate.sh's^arm=contract over stdout is untouched..github/workflows/ci.yml(perfjob)git merge-base origin/main HEADinto$GITHUB_OUTPUT. This is the one thing the step cannot get approximately right:perf pairresolves the same merge base, so a key built fromgithub.event.pull_request.base.sha— the base branch tip — would name a directoryperf.rsnever writes, and the entry would miss forever while looking exactly like a hit. A silent no-op is the one failure a cache cannot report.actions/cacheentry ontarget/perf/base-<sha>, keyedperf-base-<os>-<hashFiles('mise.lock', 'Cargo.lock')>-<sha>, with norestore-keys: a prefix hit would restore some other base's directory under this base's name, which is the stale arm the keyed path exists to refuse. Exact or nothing — a miss costs exactly the build this job pays today.Swatinem/rust-cachekey: perfis unchanged, and fix(ci): put the dev profile in the rust-cache key, and stop asserting one rule with all 103 #819's work on the debug side is not folded in here.ci-local-parityrequires everymise run <task>a workflow names to be oneverifyruns, and a runner-side cache restore has no local counterpart to be.Closure sharing between the two arms is not taken, per the row's own decision: the
:606-608eviction and target-dir lock concerns are real underverify, and a base built once per merge base and restored by every later run makes sharing unnecessary. A new merge base builds the base arm cold exactly once; that is the accepted cost.The measurement itself is unchanged — both arms sampled back to back on one machine, the ratio still
perf-compare's, the same run count and the same 1.30 threshold.Tests, shown able to fail
crates/batten/tests/it/perf_pair.rsgains five cases: the different-SHA discriminator; reuse under the right SHA; a directory with no binary is not a built arm; the keyed directory is not inside the per-run wipe; and the anti-vacuity mirror — a cold checkout has no arm to reuse, so both arms build exactly as before.The discriminator is shown red (CLOUD-418) by the mutation that is the natural wrong implementation. Dropping the key from
base_target_dir(perf_dir.join("base-target")) gives:Exactly one case reddens and the other nine stay green. Mutation reverted.
Acceptance still outstanding
The CI half is a measurement recorded on the row after this lands:
Compilinglines per arm and theperf-gatestep wall on the nextperfrun whose merge base matches its predecessor's, against the 13.5 min baseline. With--quietgone that count now discriminates. If the reading does not fall, that is a finding on CLOUD-1331 rather than a reason to widen this PR.One admission
The commit carries an
Admits:block for.github/workflows/ci.yml(d7c27bde…), issued throughbatten override request/spend..github/workflows/**isprotected, and no batten verb, task or generated artifact writes a job's step list — the redirect's own remedy is "change it in a pull request", which is this. The articulation is in the commit message where a reviewer reads it.🤖 Generated with Claude Code
https://claude.ai/code/session_01MeLn5JsAqyBuPhdU8QMwCb