Set up performance measurement for pks check

[perryqh]

Groundwork for a series of performance changes to pks check. This branch only makes the tool measurable and fixes the release profile.

Note

Stacked on #52. The base is bump-rust-toolchain-1.97.1, so the diff above shows only the measurement work. GitHub will retarget this to main automatically once #52 merges.

Why

There was no way to answer "did that make it faster, and which phase moved?" without a lot of manual setup. dev/run_benchmarks.sh compares against packwerk but reports only a total, and assumed a specific directory layout.

What's here

1. [profile.release] was never configured. It sat at cargo defaults — lto = false, codegen-units = 16 — so cargo build --release, which is what dev/run_benchmarks.sh measures, was less optimized than the shipped dist build. Now thin LTO and one codegen unit.

Measured on a 51,513-file application:

profile	mean	build time
default (before)	5.289 s ± 0.084	27 s
lto = "thin"	5.127 s ± 0.010	27 s
lto = "fat"	5.433 s ± 0.072	42 s

Fat LTO is slower to run and 55% slower to build, so thin it is — which also matches the existing dist profile. The numbers are recorded as a comment in Cargo.toml so this doesn't get re-litigated.

The variance drop matters as much as the mean: ±0.084 s → ±0.010 s. Several changes I want to measure next are worth 3–8%, which is not distinguishable from noise at the old variance.

Important

This is not entirely inert, contrary to what this description originally said. Thanks @dduugg for catching it. [profile.dist] uses inherits = "release" and overrides only lto, so codegen-units = 1 propagates into dist, where it previously picked up cargo's default of 16. That changes the build configuration of the shipped release artifacts.

No runtime-behavior change, and ci.yml's test job doesn't build --release so there's no CI-time cost. The price is dist build time. I think it's right to land — it aligns the shipped binary with what was actually measured, rather than benchmarking one configuration and shipping another — but it is a deliberate change to dist, not a side effect.

2. dev/measure.sh — hyperfine mean plus a per-phase table derived from the --debug tracing already in the tool. No new instrumentation was needed for the phase breakdown; the tracing subscriber already timestamps every span.

PKS_APP=~/src/my_rails_app bash dev/measure.sh

3. Four trace points around the tail after the checkers finish. Dropping the reference vector, diffing package_todo.yml, writing output, and final teardown were previously one unexplained gap before process exit. They're now attributable — which immediately paid off: a gap I had estimated at 0.435 s turned out to be 0.120 s, and the "optimization" I was about to write for it would have been worth ~12 ms.

4. dev/run_benchmarks.sh — honors PKS_ROOT/PKS_BIN instead of hardcoding a sibling ../pks checkout, creates tmp/ if missing, checks for hyperfine, and errors clearly if the binary isn't built. Also drops the single-file benchmark block, since that command is buggy and slated for removal and we shouldn't defend a number for it.

Guards against reporting numbers that mean nothing

Added in review, partly borrowed from rubyatscale/codeowners-rs#121, which builds the same kind of harness and systematizes these failure modes. The theme: a measurement tool's worst failure is a plausible number, not an error.

• Refuses to time a binary that doesn't work. --ignore-failure is required because pks check exits 1 on violations — but it also accepts a panic (101) or internal error (2) as a valid run, so a change that broke the tool outright would report a fast, clean-looking mean. Now probes once, accepts only 0 or 1, and greps for panicked at. Verified against tests/fixtures/app_with_monkey_patches, which panics: refused, with the panic printed.
• Warns loudly under 1000 files. My own smoke test printed 19.3 ms ± 3.0 ms for a 9-file fixture — that looks like a measurement and isn't one.
• Reports the noise floor next to the mean, and states that it's within-batch spread which understates between-session drift. An unchanged binary measured 5.1 s and 8.1 s on this machine hours apart — larger than most effects worth hunting. The guidance printed is to A/B two builds in one hyperfine run, not two separate runs.
• Records provenance — corpus file count, pack count, commit, dirty state, plus the pks commit and branch. A mean without the corpus it came from isn't comparable to anything, and mixing two was previously silent.

==> corpus: 50158 files, 761 packs, at 198479029f1d
    pks:    2f2ea9b on perf/measure-setup
==> [baseline] verifying the binary works before timing it
    exit 0 (0 = no violations, 1 = violations found) -- ok to time
==> [baseline] hyperfine: pks check (warm cache, 2 warmup / 5 runs)
  Time (mean ± σ):      8.092 s ±  0.052 s
    noise floor: +/-52ms stddev, 122ms spread (1.5% of mean)
    -> treat any delta under ~122ms as within noise
    -> this is WITHIN-batch spread and understates drift BETWEEN sessions;
       machine load moved one unchanged binary 5.1s -> 8.1s across a day,
       so A/B two builds in one hyperfine run, not in two separate runs

Verification

• cargo test — 257 passing
• cargo clippy --all-targets --all-features — clean
• cargo fmt --all -- --check — clean
• dev/measure.sh exercised against a 50k-file application, a 9-file fixture (warning fires), a panicking fixture (refused), a missing binary (refused), a missing packwerk.yml (refused), and a slash-containing default label.

Smoke-testing caught three real bugs during review that a passing-looking run had hidden: the default label is the branch name and contains /, which broke the JSON export path; grep-filtering hyperfine's output swallowed its error message so failures printed nothing; and pipefail aborted the script after the phase table because pks check exits 1 on violations.

🤖 Generated with Claude Code

[dduugg]

Approving with nits. I fetched the head, ran cargo build --release, and smoke-tested dev/measure.sh against tests/fixtures/simple_app — it runs end-to-end and produces a correct phase table, so the tooling works rather than merely looking right.

Verified

• set -euo pipefail in both scripts. No rm -rf anywhere — the only rm is a guarded rm tmp/packs_benchmarks.md behind an -f check, so there's no unset-variable path-deletion hazard.
• PKS_APP/PKS_ROOT/PKS_BIN all error clearly before use (empty PKS_APP, missing packwerk.yml, non-executable binary).
• Phase-breakdown parsing matches the real subscriber format in logger.rs (0.000116875s DEBUG file:line: message); the awk field-skip logic is correct and I confirmed it live. It is fragile to a tracing-format change with no test coverage, but that's a reasonable tradeoff for a dev-only script.
• macOS/BSD portability is fine — no date -d, sed -i, grep -P, or readlink -f; the sed -E/awk usage is portable.
• All four new trace points sit on paths that always execute for the thing they measure (checked get_all_violations, check_all, packs::check, main.rs) — no early return skips a trace and silently understates a phase.
• FmtSpan::ACTIVE in logger.rs (pre-existing) only affects tracing::span!/#[instrument] spans, and there are none in src/, so it doesn't interleave extra lines into the phase table.

The one substantive point is the [profile.dist] interaction, inline on Cargo.toml. Two cosmetic nits also inline.

[dduugg]

This makes "No behavior changes" in the description slightly imprecise, and it's worth a line in the body.

[profile.dist] immediately below uses inherits = "release" and explicitly overrides only lto = "thin". So adding codegen-units = 1 here propagates into dist too, where it previously picked up cargo's default of 16. That changes the build configuration of the shipped cargo-dist artifacts, not just local benchmarking.

No runtime-behavior change, and ci.yml's test job doesn't build --release so there's no CI-time impact there — the cost is release/dist build time. Landing it is fine (arguably an improvement, since it aligns dist with what you measured). The description just shouldn't describe it as inert.

[dduugg]

Cosmetic: this script never checks command -v hyperfine before use, whereas measure.sh:48 does and prints an actionable brew install hyperfine. Under set -euo pipefail a missing hyperfine here just aborts with the shell's own exit-127 "command not found". Functional, just less friendly than its sibling.

[dduugg]

Noting the residual gap rather than the one you already handled — the comment above plus this -x guard covers "binary doesn't exist" well.

What --ignore-failure still can't distinguish is "exited 1 because it found violations" (expected) from "panicked at exit 101" on a binary that does exist and is executable. Both get timed as valid runs. Since the purpose of this script is validating later perf changes, a change that panics on every invocation would report a fast, clean-looking mean rather than failing.

Cheap hardening if you want it: assert a known-good exit code set, or grep the captured output for panicked at. Not worth blocking on.

[perryqh]

All three addressed, and the --ignore-failure one turned out to be the most valuable — thanks for pushing on it.

[profile.dist] inheritance. You're right and the description was wrong. dist overrides only lto, so codegen-units = 1 propagates into the shipped artifacts where it previously took cargo's default of 16. Description now calls this out as a deliberate change rather than a side effect. I still think it's right to land: benchmarking one configuration and shipping another is the worse option. But it should be chosen, not inherited by accident.

--ignore-failure can't distinguish violations from a panic. Fixed, and this is the one I'd have most regretted leaving. The script now probes once before timing, accepts only exit 0 or 1, and greps for panicked at:

$ PKS_APP=tests/fixtures/app_with_monkey_patches bash dev/measure.sh
==> verifying the binary works before timing it
error: pks check exited 101, so there is nothing meaningful to time
thread 'main' panicked at src/packs/parsing/ruby/zeitwerk/constant_resolver.rs:72:17:
Found two constants with the same name: ...

Worth noting that fixture panics on main today, so the guard has a live test case in-tree.

run_benchmarks.sh missing the hyperfine check. Added, matching its sibling.

---

On the wider point. Your framing — that the tool would report "a fast, clean-looking mean" for a broken binary — is the thing I'd underweighted. A measurement tool's worst failure mode is a plausible number, not an error, because a plausible number gets published. @perryqh pointed me at codeowners-rs#121, which systematizes exactly that, so I borrowed three more guards:

• Loud warning under 1000 files. My own smoke test earlier printed 19.3 ms ± 3.0 ms against a 9-file fixture. That reads as a measurement and is entirely process startup.
• Noise floor printed next to the mean, with the caveat that it's within-batch spread and understates between-session drift. This is a mistake I actually made in Compile inflector regexes once instead of per call #54: I published −12.1% and later measured −5.5% for the same change, because the machine had gotten busy. An unchanged binary measured 5.1 s and 8.1 s hours apart on this box — bigger than most effects worth hunting. The script now tells you to A/B two builds inside one hyperfine run.
• Provenance recorded — corpus file/pack count, corpus commit, dirty flag, plus the pks commit and branch. Mixing two corpora was previously silent.

Your point about the awk parsing being fragile to a tracing-format change with no test coverage stands and I've left it — but it's now less load-bearing, since a format change would produce an empty phase table and the script errors on that rather than printing zeros.

Also appreciate the check that all four trace points sit on paths that always execute. That was the failure mode I'd have found hardest to notice: a trace that gets skipped by an early return silently understates its phase, and the number still looks reasonable.