perf: add benchmark harness for validate and generate_and_validate

[perryqh]

Summary

• Adds a benchmark harness so the performance work that follows can be measured instead of asserted. No optimizations here — this PR is the ruler, not a change to the tool.
• Names tracing spans at phase boundaries so a win can be attributed to a specific phase rather than to a function.
• Reports the harness's own precision, so a delta smaller than the run-to-run spread is labelled as noise instead of being published as a win.

Why the harness lands on its own first

Every later branch is one optimization idea, measured against this same ruler. If the harness shipped alongside the first optimization, there would be nothing to compare the first optimization against.

It has already earned its keep several times over, mostly by killing ideas:

• The generate_and_validate double-generation I expected to be worth ~0.9s has a 166ms ceiling. Dropped from near the top of the list to last.
• mapper_build measures 0ms, so "build mappers once instead of twice" is a code-quality change, not a perf one — and the follow-on idea of precomputing the vendored-gem map and package sort was abandoned without being written, because all of it lives inside that 0ms.
• Reading one line instead of the whole file cuts I/O 251x (402.2 MB → 1.6 MB, measured deterministically) and moves wall-clock not at all — the cold path is bound by ~91k open/stat syscalls, not bytes.
• A first measurement pass, run as one block per branch, produced three convincing wins that were entirely machine drift. The generate control case "improved" 36% on a branch that cannot touch it.

That last one is why this PR now reports dispersion rather than just best.

What the phase decomposition buys us

The reason to instrument rather than just time the process. One case, on a 130k-file monorepo:

validate_files_1000        12063ms
    per_file_query                        9800ms   <- 81%
    project_build                         2073ms   <- 17%
    cache_init                             109ms
    cache_persist                           73ms
    config_load                              0ms

Those five spans are disjoint — they open and close in sequence — so the percentages are sound. perf/README.md documents which spans nest (ownership_validate ⊃ validator_validate ⊃ validate_file_ownership ⊃ file_to_owners), because quoting those together as shares of one total would double-count.

Two caveats on that table worth stating plainly:

• config_load reading 0ms is an artifact, not good news. per_file_query calls a helper that wraps each path in a one-element slice and delegates to the batch API, whose first act is to reload the config. So a 1000-file run parses the config 1000 times and re-resolves the CODEOWNERS path 1000 times — all billed to per_file_query. (perf: batch the CODEOWNERS query so gv <paths> is 4x faster #124 fixes this by calling the batch function once.)
• The numbers above and the baseline table below are from different sessions (12,063ms vs 11,706ms for the same case). Both are internally consistent; they are not one dataset.

Cost model: affine, not proportional

"9.5ms per file" is the correct marginal rate, but the per-file cases fit ~2.0s fixed + ~9.9ms/file. So the per-file average is ~2,100ms at one file and ~11ms at two thousand.

For the common CI case — a PR touching a handful of files — essentially all of the time is the fixed project build and the per-file rate is nearly irrelevant. Both terms need quoting or the series optimizes the wrong end.

Reporting precision

compare prints the observed run-to-run spread per case and marks any delta smaller than it within noise:

| Case     | Baseline | Candidate | Delta | Speedup | Noise | Verdict          |
| generate |     14ms |      16ms |  +2ms |   0.88x |  ±8ms | **within noise** |

This generalizes the validate_all_cold problem rather than special-casing it: that case swings ~3s between runs, which is larger than most effects worth hunting, so it is useful as a guard against warm-only wins and not as a number to optimize against.

Cargo.toml also pins codegen-units = 1 and lto = "thin". Baseline and candidate are separate builds, so codegen nondeterminism contaminates an A/B comparison directly rather than merely widening error bars. Full release build goes to ~30s wall.

Two things the numbers still do not include, both documented in perf/README.md:

• Per-invocation setup is undercounted. All cases share one process and teams_by_github_team_name is #[memoize]d process-globally, so the warmup run pays the team-file parse and no timed run ever does. A real CLI invocation pays it every time. Published numbers are a floor for single-shot cost.
• validate_all_cold cannot resolve small effects, per above.

Reviewer guidance

Five commits, ordered so the shipped-code change is reviewable on its own:

• library instrumentation — span names only. The only commit touching shipped code, and worth the closest look.
• the harness binary and scripts — self-contained; nothing else imports it.
• tests for the harness mechanics.
• gv <paths> cases — closing a gap where the harness measured validate <paths> but not the path-taking form that is actually equivalent to a full validate.
• precision reporting — the spread/verdict column, the release-profile pin, and the caveats above.

On the library commit

Two spans were both named validate (Ownership::validate and Validator::validate) and collapsed into each other in any aggregation; mapper construction, the validator sub-steps, and the runner's config/cache work had no spans at all.

All spans are level = "debug", so they cost nothing without a subscriber that enables them. The only non-attribute changes are cache_init and cache_persist being wrapped in in_scope closures — note no closure contains a bare ?, so none of them can return from the closure instead of the function.

Verified: generating a CODEOWNERS for a 130,934-file repo produces byte-identical output, and the corpus repo was left clean.

Guards against measuring nothing

All of these live in src/bin/codeowners-perf.rs; perf/run.sh and perf/compare.sh are ~20-line wrappers that build and exec.

• Argument lists are built as real vectors in Rust — there is no subprocess boundary for timed work at all, so the shell word-splitting failure mode is structurally impossible rather than merely avoided. (The only Command::new is for git metadata.)
• Each case asserts it built exactly the number of paths it asked for.
• Cases needing more files than the corpus has are reported as skipped with a reason, never silently shrunk.
• compare refuses to diff reports from different corpora or corpus commits, and warns when the machine differs. A fixture-measured branch against a monorepo-measured baseline would otherwise read as a 1000x speedup.
• generate/gv write the corpus CODEOWNERS, so it is snapshotted and restored, and the harness refuses to start if that file already has uncommitted changes.

Corpus configuration

--corpus, then $CODEOWNERS_PERF_CORPUS, then the committed tests/fixtures/valid_project. No path to any specific monorepo is stored in the repo.

The fixture default is a genuine smoke test — 28 files, 41-line CODEOWNERS, single-digit milliseconds. It proves the harness works and is useless for comparison, so run.sh prints a loud banner under 1,000 tracked files and every report records corpus size.

Not wired into CI

Shared runners are too noisy for 2-20s wall-clock comparisons and have no corpus. cargo test covers the harness mechanics (8 tests, sub-second, on the committed fixture) so it cannot rot silently, but it never measures. Tradeoff: perf regressions are caught only when someone runs the harness deliberately.

No committed baseline, which departs from the original plan: wall-clock numbers are machine-specific, so a committed report would invite exactly the invalid comparison the guards prevent — and it would embed a local absolute path. perf/results/ is gitignored.

Baseline for the record

Not committed, for the reasons above. macOS/aarch64, 11 cpus; corpus of 130,934 tracked files, 91,206 owned, 17,981 CODEOWNERS lines. Best of 3, warm cache unless noted. gv_files_* were added by a later commit and measured separately (see #124).

Case	Best
generate	2,220 ms
validate_all	3,176 ms
gv	3,342 ms
validate_all_cold	6,060 ms
validate_files_1	2,123 ms
validate_files_100	3,081 ms
validate_files_1000	11,706 ms
validate_files_2000	21,945 ms

Cross-check: generate + validate_all − gv = 2,054ms, which independently reproduces project_build's 2,073ms from the phase table.

Verification

export CODEOWNERS_PERF_CORPUS=/path/to/a/large/monorepo
./perf/run.sh                                     # table + phase breakdown
./perf/run.sh --json > perf/results/mine.json
./perf/compare.sh perf/results/base.json perf/results/mine.json
cargo test --test perf_harness_test               # harness mechanics

🤖 Generated with Claude Code

[dduugg]

Approving. The shipped-code change is clean, and the harness is well built — in a couple of places the defenses are stronger than the description claims. Reviewed by splitting it five ways: the library instrumentation, the harness binary, the scripts, the tests, and an audit of the numbers themselves.

Most of what follows is about the description and the precision of the instrument, not defects in the code. Since every later branch gets justified against this ruler, those seemed worth getting right now.

Verified — what held up

• The in_scope refactor is safe, for the specific reason this pattern usually breaks: no closure contains a bare ?. Each either returns the Result value or has ? applied after in_scope() returns, so nothing returns from the closure instead of the function. cache_persist's closure borrows cache immutably, so it's still movable into Self afterward.
• Span hygiene: all new spans are level = "debug" with skip_all. Grepping all 15 span names across src/ (not just the diff) confirms no collisions remain after the ownership_validate / validator_validate rename.
• The phase spans in the quoted table are disjoint, not nested, so the 81% / 17% figures are trustworthy. config_load, cache_init, project_build, cache_persist open and close in sequence inside Runner::new; per_file_query opens later in validate_files, after Runner::new has returned. Worth noting the generic "nested spans are inclusive" caveat at codeowners-perf.rs:121 does not apply here — but it will apply to any validate_all breakdown you quote later, where ownership_validate ⊃ validator_validate ⊃ validate_file_ownership/file_to_owners. Those percentages would double-count.
• The unused project build is real. validate_files (runner.rs:145-195) reads only self.run_config and self.config; self.ownership appears nowhere in it. One caveat for whoever writes that optimization: Runner::new also persists the disk cache at :108, so skipping the build changes cache warmth for later invocations — not read on this path, but not side-effect-free either.
• Skip-not-shrink works, and no .take()/min() silently reduces a case. Confirmed against the fixture: validate_files_100/1000/2000 correctly report skipped with a reason.
• The word-splitting bug class is structurally impossible, not merely avoided: there's no subprocess boundary for timed work at all. The only Command::new is git_output() for metadata. That's a stronger guarantee than the description claims.
• The ~0.2s generate_and_validate ceiling checks out two ways. gv − validate_all = 3,342 − 3,176 = 166ms. And generate + validate_all − gv = 2,054ms, which independently reproduces project_build's 2,073ms from the phase table — a nice cross-check that the shared cost is the Runner::new you'd expect.
• Corpus precedence, the smoke-scale banner, the dirty-CODEOWNERS refusal, perf/results/ being gitignored, and the corpus/commit-mismatch refusal all verified by running them against the committed fixture. No absolute paths or usernames leaked anywhere in the diff.
• Tests: 8/8 pass, and the run leaves the tree clean. Skip logic, dirty-corpus refusal, restore-on-drop, and compare-refusal are all pinned non-vacuously, on tempdir copies with --corpus passed explicitly.

The per-file cost is mostly re-done setup

The most actionable thing I found, and I think it changes the optimization plan rather than just the description.

per_file_query calls team_for_file_from_codeowners once per file (runner.rs:167). That function (runner/api.rs:76) wraps each single path into a one-element slice and delegates to the batch API teams_for_files_from_codeowners, whose first line (runner/api.rs:64) is config_from_run_config(run_config)?.

So a 1,000-file changeset parses the config 1,000 times, plus re-resolves the CODEOWNERS path 1,000 times — and all of it is charged to per_file_query, which is why config_load reads 0ms and looks free. It isn't; it's just being billed to the wrong span.

The batch function already takes &[String] and does that setup once. Calling it once with all paths, instead of N times with one path, looks like it captures a large share of the 9.5ms/file without touching the query logic at all.

The cost model is affine, not proportional

"9.5ms per file, linearly" is right as a marginal rate — marginals between your four points are 9.68, 9.58, 10.24 ms/file, and a least-squares fit gives 2,043ms fixed + 9.89ms/file, predicting all four points within 0.5-3.3%. The 22s projection for 2,000 files matches.

But there's a ~2.05s fixed floor, which is ~92% of generate's entire 2,220ms. So the per-file average is never 9.5ms — it's 2,123ms at n=1, 30.8ms at n=100, 11.0ms at n=2,000. For the common CI case (a PR touching a handful of files) essentially all the time is fixed overhead, and the per-file rate is nearly irrelevant. That's a different optimization target than the per-file re-parse, and possibly a more valuable one — worth stating as "9.5ms/file marginal plus ~2.0s fixed" so the series doesn't optimize the wrong end.

Precision: can this ruler resolve 3-8%?

Not yet, for two of the cases, and it's currently impossible to tell for the rest:

• validate_all_cold can't resolve anything useful. Your own 6.0-9.1s range is a 3,040ms spread against a 3-8% target of 182-485ms — noise roughly 6x the largest effect being hunted. Recommend excluding it from compare output or marking it explicitly non-comparable.
• Warm variance is unknowable from this PR. The harness stores runs_ms and median_ms (codeowners-perf.rs:118-120) but the write-up publishes only best. Min-of-3 is a biased estimator with no dispersion attached. Reporting median + min + max (and --runs 10 when it matters) would make the ruler's own precision visible — arguably the one number a ruler PR has to include.
• [profile.release] is at cargo defaults (debug = true only — no lto, no codegen-units). Your own pks#53 measured that exact config as 8x worse run-to-run variance (±0.084s → ±0.010s). It matters more here than there: baseline and candidate are separate builds, so codegen nondeterminism contaminates the A/B comparison directly rather than just widening error bars. Cargo.toml isn't in this diff so I couldn't comment inline, but tightening it before generating baselines seems worth doing.
• In-process measurement omits a cost the real CLI pays. All cases share one process, and teams_by_github_team_name is #[memoize]d process-globally (ownership/codeowners_file_parser.rs:67), so the warmup run pays the team-file parse and no timed run ever does. Every published number is missing something a per-invocation CLI pays every time. Process-per-run would close it; at minimum worth a documented caveat. (Mechanism confirmed; magnitude I can't estimate without your corpus.)

Description details

• "Three commits" — the head has four. 69feb9d ("measure gv with explicit paths") came after, and the baseline table also omits the gv_files_100/gv_files_1000 cases it added.
• The phase breakdown and the headline table are different sessions. The breakdown totals 12,063ms for validate_files_1000; the table lists 11,706ms. Both fine individually, but they read as one dataset.
• The guards are credited to the wrong files. "compare.sh refuses…" and "run.sh prints…" — both behaviors live in src/bin/codeowners-perf.rs (banner at :497, refusal at :611-621). The scripts are ~20-line wrappers that build and exec; compare.sh's only own logic is an arg-count check. The behavior exists exactly as described, it's just implemented in the binary — worth saying "the harness" so a reader doesn't go looking in the shell.

Absolute numbers I could not verify without your 130k-file corpus; everything above is internal consistency plus code reading. Nothing here blocks merge.

[dduugg]

The guard's scope means cases 2-N don't see the corpus they claim to. This is the one finding I'd want addressed (or documented) before trusting a full-suite run on a real monorepo.

CodeownersGuard is acquired here, outside the for case in selected loop at :518, so the snapshot is restored once when cmd_run returns — after every case has run. generate is case #1 in CASES and rewrites the on-disk CODEOWNERS via runner::generate. So validate_all, gv, gv_files_*, validate_all_cold and validate_files_* all validate the freshly-generated file, not the corpus's committed one.

On any corpus where the committed CODEOWNERS has drifted from what the config would generate — precisely the state the validator exists to detect — that silently zeroes validation_errors for every case after the first, and makes their error-formatting and diff work artificially cheap. So it distorts the timings too, not just the counts.

The "corpus repo was left clean" claim is accurate about the end state; it doesn't hold for what cases 2-N observe mid-run. It doesn't show up on the committed fixture only because that fixture's generated output happens to be byte-identical to its tracked file — a property of the fixture, not a guarantee the harness enforces. --case <one> avoids it, since generate never runs.

Cheapest fix is probably to acquire/drop the guard per case inside the loop, so each case starts from the pristine file.

[dduugg]

This is where the 9.5ms/file goes, and it explains why config_load reports 0ms.

The loop calls team_for_file_from_codeowners once per file. That function (runner/api.rs:76) wraps the single path into a one-element slice and delegates to the batch API teams_for_files_from_codeowners — whose first line (runner/api.rs:64) is config_from_run_config(run_config)?, followed by resolve_codeowners_file_path.

So for validate_files_1000 the config is parsed 1,000 times and the CODEOWNERS path re-resolved 1,000 times, and all of it lands inside this per_file_query span. Meanwhile the config_load span wraps only the single load in Runner::new, which is why the breakdown shows config_load 0ms and makes config parsing look free. It isn't — it's being billed to per_file_query, inside the 81%.

Two implications:

1. The phase table slightly misdirects: some meaningful part of that 81% is repeated setup, not the per-file CODEOWNERS matching the name implies.
2. The fix is already available — teams_for_files_from_codeowners takes &[String] and does the setup once. Calling it a single time with filtered_paths instead of N times with one path each looks like it recovers a large share of the per-file cost without touching the matching logic.

Not a defect in this PR (the span is faithfully placed around the loop). Flagging it because it changes what the next optimization should target.

[dduugg]

The suite can't catch the harness recording garbage timings — which is the exact failure mode the README's zsh story is about.

This asserts only that runs_ms is non-empty. If the timing mechanism regressed to always producing zero (an Instant reused, elapsed() on the wrong instant, a timed loop that stops running the case), runs_ms would be [0] — still non-empty — and all 8 tests would stay green. A harness reporting confident zeros is worse than one that crashes, since the numbers get pasted into a PR body.

The asymmetry is what caught my eye: :72-73 do value-check the corpus fields (tracked_files > 0, codeowners_lines > 0), so the pattern is already established here — it just isn't applied to the measurements themselves.

Also, file_count is never read by any test (it appears only in the synthetic JSON at :229). The "each case asserts it built exactly N paths" guarantee therefore rests entirely on the assert_eq! at codeowners-perf.rs:426 — and that assert can't currently fail anyway, since the skip check at :406-419 already guarantees case.files <= pool.len() before .take() runs. So that guard is a tautology with no test-side net.

Two lines would close both: assert!(case["best_ms"].as_u64().unwrap() > 0) for a non-skipped case, and assert_eq!(case["file_count"], 1) on validate_files_1.

Noting also that median() is only exercised via pre-computed JSON — every real-run test uses --runs 1 --warmup 0, so the actual sort/median path never sees more than one sample.

[dduugg]

This comment contradicts both the code and the README sitting next to it: no perf/baseline.json is tracked anywhere in this PR, and perf/README.md explicitly says the opposite — that there is deliberately no committed baseline, because it would embed a local absolute path and invite the cross-corpus comparison the guards exist to prevent.

Reads like a rationale from an earlier version of the plan. Something like "so a local baseline export can't be committed by accident" would match what the line actually does.

[perryqh]

Thanks — this was a genuinely useful review, and the two findings I'd single out both changed what happens next rather than just the prose.

Pushed 4f955f3 addressing the actionable items. Disposition of each:

Fixed in code

Precision is now reported, not just best. You're right that min-of-3 with no dispersion attached is not a measurement. compare now prints the observed run-to-run spread per case and marks any delta smaller than it within noise:

| Case     | Baseline | Candidate | Delta | Speedup | Noise | Verdict          |
| generate |     14ms |      16ms |  +2ms |   0.88x |  ±8ms | **within noise** |

I took this over excluding validate_all_cold specifically, because the general problem is what bit me: measuring one-branch-per-block, the machine drifted ~35% mid-sweep and produced three convincing wins that were pure noise — the generate control "improved" 36% on a branch that cannot touch it. A per-case noise floor catches that class, not just the cold case. validate_all_cold is now documented as a guard against warm-only wins rather than a number to optimize against.

[profile.release] pinned to codegen-units = 1 and lto = "thin". Your point that this contaminates the A/B directly rather than widening error bars is the part I'd missed — baseline and candidate genuinely are separate builds here. Full release build goes to ~30s wall, which seems a fair price. (Noting for the record it also changes the shipped binary; the release workflow tars by binary name so packaging is unaffected.)

The memoize gap is documented. teams_by_github_team_name being process-global means the warmup pays the team-file parse and no timed run does, so every published number is a floor for single-shot CLI cost. Documented rather than fixed — process-per-run would close it but would also throw away the phase decomposition, which is the harness's main value. Worth revisiting if per-invocation cost becomes the thing under study.

Nesting is documented. perf/README.md now spells out which spans are disjoint (config_load/cache_init/project_build/cache_persist/per_file_query) and which nest (ownership_validate ⊃ validator_validate ⊃ validate_file_ownership ⊃ file_to_owners). Your catch is well-timed: I had already quoted a validate_all breakdown mixing those levels later in this series.

Fixed in the description

Commit count (five now), the guards credited to codeowners-perf.rs rather than the shell wrappers, the phase table and baseline table flagged as different sessions, and the gv_files_* cases noted as added by a later commit.

The affine cost model is now stated as "~2.0s fixed + ~9.9ms/file." This is the finding I think matters most, and I had it wrong. Quoting a flat 9.5ms/file implies the per-file term is the target; your fit shows that for a PR touching a handful of files essentially all the time is the fixed project build. The per-file average is ~2,100ms at n=1.

Where it went

Your observation that the per-file cost is mostly re-done setup — config_load reading 0ms because the config reload is billed to per_file_query — is exactly right, and is now #124: call the batch function once with all paths instead of N times with one. Measured 14.6s → 3.6s (4.1x) on gv <1000 paths>, with validate_all flat at +0.3% as a control.

One thing worth flagging since you raised the cache-warmth caveat on skipping the project build: I built that optimization, and then closed it (#123). validate <paths> resolves ownership by reading the CODEOWNERS file, so it validates a derived artifact against itself and silently passes a file whose annotation disagrees with CODEOWNERS, an annotation naming a nonexistent team, or a file owned two ways — all three reproduced. So the ~2.0s fixed cost you identified as the more valuable target is real, but it can't be claimed on that path until parity is fixed. That's the next piece of work, and it's a correctness fix rather than a perf one.