perf: batch the CODEOWNERS query in validate_files (parked pending #125)

[perryqh]

Summary

• generate-and-validate <paths> on a 1000-file changeset goes from 14.6s to 3.6s (4.1x) on a 130k-file monorepo.
• One change: query the CODEOWNERS file once for all paths instead of once per path. No new abstraction — the batch function already existed and was being called N times with one path each.
• Stacked on perf: add benchmark harness for validate and generate_and_validate #121 (the harness). Generated CODEOWNERS is byte-identical.

What was wrong

validate_files looped over paths calling team_for_file_from_codeowners. That helper wraps the path in a one-element slice and hands it to the batch query — which reloads the config and re-reads and re-parses the entire 17,981-line CODEOWNERS every time. parse_codeowners_entries is not memoized, unlike teams_by_github_team_name directly beside it.

Cost: ~9.5ms per path, linear. On a 1000-path changeset that is ~10s spent re-parsing the same file 1000 times.

The batch function already accepts a slice and already parallelizes across it with par_iter. It was simply being called wrong.

Why this targets gv <paths> and not validate <paths>

An earlier version of this work also bypassed the project build, which made validate <paths> ~1500x faster. That was closed (#123) because validate <paths> is not equivalent to validate — it resolves ownership by reading the CODEOWNERS file, so it validates a derived artifact against itself and cannot detect a stale CODEOWNERS, an annotation naming a nonexistent team, or a file owned two ways. Making that path faster is not a win.

Warning

This section was wrong, and it was the load-bearing claim. It read: "gv <paths> is correct: it regenerates before validating."

Regenerating cures only the staleness gap. gv <paths> still skips validate_invalid_team and validate_file_ownership, because generate() merely writes the generated string and validates nothing. Worse, on a dual-owned file, regenerating writes it into CODEOWNERS under one of its owners — so the per-path check finds an owner and exits 0. Regenerating conceals that defect rather than exposing it.

#125 has failing tests for all of it. So this PR optimizes a check that misses two of three defect classes, and the claim that it "does not extend or entrench" the weak path does not survive: a 4x faster weak check is a more attractive weak check.

This change does not make anything less correct — the batching itself is sound and the generated CODEOWNERS is byte-identical. But whether it should land depends on #125's outcome: if the fix routes <paths> through the mappers or drops the param, validate_files may cease to exist and this work is moot. If the fast path survives behind an explicit flag, this still applies.

Measured

Corpus: 130,934 tracked files, 91,206 owned, 17,981-line CODEOWNERS. macOS/aarch64, 11 cpus.

Interleaved A/B, 6 rounds, min-of-N. validate_all is included as a control: this change should not affect it, and it doesn't — which is what makes the other two rows trustworthy.

Case	base	this branch	Delta
gv <1000 paths>	14,555 ms	3,584 ms	-75.4%
gv <100 paths>	4,570 ms	3,538 ms	-22.6%
validate_all (control)	4,335 ms	4,350 ms	+0.3%

Interleaving matters here: a first attempt measured each branch in its own block and the machine drifted ~35% mid-run, which produced three convincing but entirely fake wins. Running every variant once per round makes drift shared rather than attributed, and the flat control confirms it worked.

The remaining 3.6s is the project build, which generate genuinely needs — this change removes the per-file term, not the fixed cost.

Correctness

• 807 mixed paths through both binaries — owned files, paths that are glob-filtered out, paths matching unowned_globs, absolute paths, and paths absent from CODEOWNERS. gv stdout, stderr and exit code all byte-identical, and the generated CODEOWNERS (17,981 lines) is byte-identical.
• validate <paths> output also unchanged, so this is not a behavior change to that path — just a faster one.
• Full suite green, including all 12 existing validate_files tests and the absolute-path case.
• Corpus repo left clean.

Unowned files are still reported using the caller's original path string and in input order, so absolute paths render exactly as before. The query is keyed by project-relative path, so the original string is carried alongside rather than recomputed.

Error handling: no behavior change after all

An earlier revision of this description flagged lost per-path IO error attribution as a decision needed before merge. On tracing it, there is nothing to decide — the error arm is unreachable from this call site:

• The only failure teams_from_files_paths reports is a non-UTF-8 path (codeowners_file_parser.rs:24-35), and the runner has already put every path through to_string_lossy, so to_str() always succeeds.
• parse_codeowners_entries returns Vec, not Result. A missing or unreadable CODEOWNERS is not an error on this path at all: the parser logs to stderr and yields no entries (:95-101), so every path is reported unowned instead.

So io_errors was already always empty here, before and after this change. The Err arm is kept for completeness and commented as such.

Worth a separate issue: that swallowed-read behavior means a missing CODEOWNERS reports the entire changeset as unowned rather than surfacing the real cause. Pre-existing, untouched here.

Follow-up

The parity gap in validate <paths> is unfixed and is the more valuable piece of work: resolve ownership through the mappers for the supplied paths rather than reading CODEOWNERS. That would make validate <paths> trustworthy, and only then is bypassing the project build for it worth doing.

🤖 Generated with Claude Code

[perryqh]

Parking this pending #125.

The justification in "Why this targets gv <paths> and not validate <paths>" was wrong. It claimed gv <paths> is correct because it regenerates first. Regenerating cures staleness only — validate_invalid_team and validate_file_ownership are still skipped, since generate() writes the generated string and validates nothing.

And on a dual-owned file it is worse than a miss. Regenerating writes the file into CODEOWNERS under one of its two owners, so the per-path check finds an owner and exits 0 with no output. Regenerating conceals the defect. #125 has a failing test: gv ruby/app/services/multi_owned.rb → code=0, empty stdout, on a file that full gv flags as owned two ways.

So the honest description of this PR is: it makes a check that misses two of three defect classes 4x faster. Nothing here is incorrect — the batching is sound, the generated CODEOWNERS is byte-identical, and it removes ~10s of genuinely redundant CODEOWNERS re-parsing. But the subject may not survive.

Not closing it, because the outcome depends on how #125 is resolved:

• <paths> routed through the mappers, or the files param dropped — validate_files likely ceases to exist and this is moot. Close it then.
• fast path kept behind an explicit opt-in flag — this still applies and should be revived.

Two notes for whoever picks it up:

The perf table was measured at c0e5546, before review changes were amended in. I do not expect movement — the amend swapped a clone pass for unzip and a hand-rolled relativization for path_utils::relative_to — but it has not been re-measured, and neither has the 807-path differential.

The description also still says "all 12 existing validate_files tests" (now 15) and "stacked on #121" (since merged).

[perryqh]

Closing as obsolete, superseded by #125.

This optimized reading the generated CODEOWNERS back once per supplied path. #125 removes that step entirely — ownership for supplied paths is now resolved through the mappers, the same way the whole-project run does — so the code path this batched no longer exists.

That was the necessary change rather than a preference. Reading CODEOWNERS back could only ever answer "does this path have an owner", which cannot see a file owned two ways: generation picks one winner and writes it, so the file looks owned. gv <dual-owned file> exited 0 with empty output. Making that check 4x faster would have made a check that misses two of three defect classes more attractive to keep.

The batching itself was correct, and the ~10s of redundant re-parsing it removed was real. It just had the wrong subject.

Two things worth salvaging from here rather than losing:

• The measurement methodology. Interleaved A/B with a validate_all control row, after an earlier attempt was contaminated by ~35% machine drift mid-run. That approach is worth reusing on fix: run the real ownership checks when validate/gv are given paths #125's open question — whether the files param is worth keeping at all now that the scoped check is correct.
• gv with no paths is still not a case in the perf harness. That is the number needed to decide between dropping the param and adding escalation. Adding it is a small change to src/bin/codeowners-perf.rs.

Not reviving this branch. If the fast path later earns an explicit opt-in flag, the batching question can be revisited from scratch against whatever that path actually does.