ci: cut the PR gate from 14m to ~5-6m - #4
Merged
Conversation
The PR gate took 14m11s and it was not the tests: all 70 test binaries finish in five seconds. Every other second was codegen, and three things were wasting it. The single `check` job ran fmt -> gallery sync -> web -> clippy -> tests -> doc serially on one runner. Worse, `cargo clippy` artifacts are never reused by `cargo test` -- verified in a scratch crate, where `cargo build --all-targets` then `cargo test --no-run` re-ran zero rustc invocations but `cargo clippy --all-targets` then the same command re-ran twelve. In CI that meant 261 of 264 dependency crates compiled twice in the same job, ~257s of pure waste. Split into parallel `web`/`lint`/`test`/`doc` jobs so the wall clock is the slowest dependency chain rather than the sum. Splitting alone lands at ~7.5m, because the remaining 402s is the serial `core -> theme -> components -> herogpui -> gallery` chain and cannot be fanned out by package. The rest comes from building the gates at -O0: Cargo.toml's -O1 workspace / -O3 dependencies is a local-dev choice for a responsive gallery, and a correctness gate does not need it. The override is written to $CARGO_HOME, not to Cargo.toml, so local builds are untouched, and a new `optimized` job re-runs the same tests at the real profile on every push to master so that coverage is traded off the PR path, not dropped. `Source install` logged "No cache found" and rebuilt 486 crates in 694s for two independent reasons: rust-cache's default key embeds job_id, so jobs could not see each other's cache, and `cargo install` builds in a throwaway directory. Fixed with a shared key and --target-dir. The key must not vary by event -- a cache from the default branch is restorable by any PR targeting it, which is what keeps PR runs warm.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
assert_settled failed 5 tests on one -O0 run and 1 on the next, same commit and same profile, so the measure-then-cap double layout in popover.rs can wobble by a pixel. Two samples cannot tell a rate from luck, so run the binary 40 times at -O0 and 40 times at the Cargo.toml profile and count. Delete this with the PR.
The -O0 gate profile made assert_settled in dropdown_viewport_deep fail deterministically (40/40 at -O0, 0/40 at the real profile), so the job that executes layout cannot be one of the jobs the profile override buys time for. The push-only `optimized` job is now an exact duplicate of `test` and is gone, which also means the optimized path stays gated on every PR rather than only on master. The probe is rewritten to isolate the half of the profile that matters: only workspace crates are on the per-PR critical path (dependencies are cached), so it asks whether dependencies at -O3 with the workspace at -O0 still settles.
assert_settled compared whole Bounds with exact equality. A height-capped panel is laid out twice per frame by the positioner -- once at MaxContent to choose a side, once at the cap -- and debug_bounds reports whichever pass wrote last, so its height is ambiguous by the pixel the cap rounds to. Every observed failure was 125px vs 126px with an unchanged origin; the rest of this file already treats 1.5px as layout noise via near(). Compare each frame to the first reading so drift still fails, keep origin exact because movement is what the test rules out, and give size the existing tolerance. This is what let the test job go back to the -O0 profile: measured 237s there against 842s at Cargo.toml's.
Unoptimizing dependencies too made layout assertions in dropdown_viewport_deep fail 40/40: a height-capped panel is laid out twice per frame and debug_bounds reports whichever pass wrote last, so its height lands a pixel either side depending on codegen. Origin never moved. Rather than loosen three exact-equality assertions to absorb a build flag, keep dependencies at Cargo.toml's -O3 -- they are cached, so they cost a PR nothing, and they stay identical to a developer's build -- and drop only the workspace crates, which is the half on a PR's critical path. Measured 0/12 for that configuration in the same regime. Reverts the assert_settled tolerance change: it was not sufficient (still 40/40) and is no longer needed. With dependencies profile-identical, one cache key serves every Rust job, so the per-profile keys are gone. Co-Authored-By: qoder-agent
cargo -v quotes every argument separately, so the grep for `--crate-name herogpui_core` matched nothing, and this shell runs with -e -- the failed grep ended the job before the 40-run verdict could report. Flatten the quotes and guard each grep. Co-Authored-By: qoder-agent
With shared-key: ci every Rust job logged a hit on the same entry, and then `cargo doc` rebuilt 0 crates, `clippy` 3, `cargo test --workspace` 479 and `cargo install` 467 -- one entry cannot hold both the rlib set a test build links against and the one rustdoc consumes, and the writers answered "Cache up-to-date" so it never converged on the shape the slowest job needed. Per-job keys restore what each job actually builds against, and stay warm across branches because the embedded id is the YAML job key, identical on master and on a PR. Co-Authored-By: qoder-agent
It answered the question it was added for: dependencies at Cargo.toml's -O3 with the workspace at -O0 settled 0/40 in the single-threaded regime where unoptimized dependencies failed 40/40, with both full workspace passes clean. Co-Authored-By: qoder-agent
The profile comment quoted timings from an intermediate revision and a
probe verdict ("passed 0/12") that the probe never printed -- its notices
were unexpanded, so the real evidence is the failure count per phase: 40
out of 40 with dependencies at -O0, 0 out of 92 with them at Cargo.toml's
-O3. Saying "shared cache" after moving to per-job keys also left the
reader looking for a key that no longer exists.
The install step's drop is attributed to --target-dir alone, which is
overclaiming: the flag makes the artifacts cacheable, warm cache is what
makes it fast.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Check & Teston #2 took 14m 11s. The gate is entirely compile-bound: itstest binaries execute in about five seconds and the rest of the time is rustc
codegen. This splits the battery across parallel jobs, drops the optimization
level of the workspace crates on PRs, gives each job its own cache, and makes
the install step cacheable. Warm PR now: 1m 49s, with the same commands and
the same flags.
Diff scope:
.github/only. No Rust source, noCargo.toml, no tests.Measured before (run
33995638880)checkjobSource installjobThe four changes
checkintoweb,parity,lint,test,doc. The wall clockbecomes the slowest chain instead of the sum.
webneeds no Linux builddeps, so a formatting or gallery-sync failure reports in under a minute
rather than after a full compile.
-O0for the workspace crates on PRs..github/actions/rust-env(new,composite) appends
[profile.dev] opt-level = 0to$CARGO_HOME/config.tomlwhen
profile: fast. Dependencies keepCargo.toml's-O3. Reason below.Swatinem/rust-cache's default key embedsgithub.job_id, which is the YAML job key and is identical on master andon a PR, so master's runs keep serving PRs.
cargo install --path gallery --target-dir target. Without a target dirthe step builds somewhere throwaway, so there was nothing for the cache to
hold.
.github/actions/rust-envexists because the apt package list was alreadyduplicated verbatim twice in
ci.yml, and because the profile override has tobe byte-identical across jobs or their caches diverge. It is written to
$CARGO_HOMErather than passed as--configbecause.shots/lint.ps1ownsits own cargo invocation.
release.ymlis untouched.Measured after (run
34009942300, warm)Wall clock 109s.
check)check)check)check)Why dependencies stay at
-O3The first cut dropped everything to
-O0andassert_settledindropdown_viewport_deep.rsstarted failing. Rather than loosen an assertion Icould not justify, I measured it: a temporary probe workflow ran that binary
forty times per profile and at both extremes. Recorded output, by phase:
-O0+ workspace-O0-O3+ workspace-O0Cargo.tomlprofile (-O1/-O3)So the sensitivity is in GPUI and taffy's layout math, not in this repo's
crates: a height-capped popover is measured twice per frame and its reported
height lands one pixel either side depending on dependency codegen. Scoping the
override to the workspace keeps every assertion exact and keeps CI's
dependencies identical to a developer's, which is the point of a gate. The
probe workflow is deleted and the test file is back to its baseline, so zero
test changes ship in this PR:
That 1px ambiguity is a real latent issue in the positioner and deserves its own
investigation. This PR does not fix it and does not hide it behind a tolerance;
it records where it bites.
Why not one shared cache
shared-key: ciwas tried and reverted. Every job logged a hit on the sameentry, and then
cargo docrebuilt 0 crates,clippy3,cargo test --workspace479 andcargo install467. An entry holds one artifact shape:the rlib set a test build links against is not the one rustdoc or clippy
consume, and the writers kept answering "Cache up-to-date", so it never
converged on the shape the slowest job needed.
Testswent 646s with the sharedkey against 107s per-job.
Gate coverage is unchanged
Comparing the logs of
33995638880and34009942300line for line:test result:lines, 1757 passed, 13 ignored;all 5 crates inherit [workspace.lints],clippy clean (warnings denied)andcargo deny clean;pnpm run extract:checkandRUSTDOCFLAGS=-D warningsstill gate;.shots/package_audit.pyreports 0 errors, so the CI text it asserts on(
cargo install --path gallery, nocargo publish) still holds.Same commands, same
-D warnings, fewer minutes.Costs I want on the record
Cargo.lockor toolchain change, oranywhere a job's cache key is new,
Testsmeasured 668-715s because the~480 dependencies at
-O3have to be built. Steady state after that is thetable above. Concretely: the first
push: masterafter merge re-seedstest,lintanddocunder their new keys, so that one run is cold.installproved the mechanism by hitting master's entry on its first run.optimizedhas never executed. It is gated onpush: master, and everyrun on this branch has been a
pull_requestevent. Prior evidence it isgreen: master's push
33996338866rancargo test --workspace --lockedatexactly that profile and passed, and this PR changes no Rust and no tests.
Worst case, it goes red after merge, which is the residual risk of moving the
optimized build off the PR path.
made obsolete. Master's two dead
v0-rust-check-*entries (3.0 GB) should goonce this merges, or GitHub's LRU eviction will turn a warm job cold and it
costs ten minutes to find out.
squash before merge if the eight commits bother anyone.
Test plan
python .shots/package_audit.py-> 0 errorsweb, parity, lint, test, doc, install, optimized34009942300)Source installlogs a cache hit instead ofNo cache foundpush: masterafter merge:optimizedbuilds at-O1/-O3with no profile override in its log, and re-seedstest/lint/doc