perf: audit against high-performance-go.md, fix 3 confirmed hot paths - #767
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
🟢 Merge Queue — picked up This PR is in the queue and will be batched with other Next: No action needed — you'll get another comment when CI starts on the batch. View merge queue run. |
|
This PR could not be merged into the batch branch without conflicts with Next: Rebase onto or merge |
checkFootnotes ran footnoteRefRE and footnoteDefRE over the full source
on every Check call, even on files with no footnote syntax at all. Per
docs/development/high-performance-go.md ("Gate expensive analyzers
behind a cheap pre-check... byte-needles gate regex paths"), add a
bytes.Contains(f.Source, "[^") pre-check mirroring MDS012's
mayContainURL: every match either regex could produce requires that
literal byte pair. Measured on a 200-paragraph footnote-free fixture:
~253,800 ns/op -> ~4,500 ns/op (~56x), 0 allocs/op either way.
BenchmarkCheckFootnotes_NoFootnotes pins the regression with a hard
ns/op budget (b.Fatalf on overshoot), confirmed red against the
pre-fix code and green after.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016wWQmrtb8EMbn1hDgkESqE
hasReservedDeviceName called strings.ToUpper on every path segment before the reservedDeviceNames set lookup. strings.ToUpper is a no-op copy only when the input already matches the target case, so a typical (lowercase) segment always paid the allocation. Every reserved name is 3-4 bytes (CON, PRN, COM1-9, LPT1-9), so segments outside that length range can never match; gating the ToUpper call behind a length check skips the allocation entirely for the common case. Per docs/development/high-performance-go.md's "Compile regexes at package scope" sibling guidance on cheap pre-checks gating expensive work. Measured on a 3-path, 7-segment fixture: 602 ns/op & 10 allocs/op -> ~380 ns/op & 5 allocs/op (the two segments that do fall in the 3-4 byte range still allocate, as they must to compare). TestHasReservedDeviceName_AllocBudget pins the allocation ceiling, confirmed red against the pre-fix code and green after. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wWQmrtb8EMbn1hDgkESqE
driftParts computed githooks.ResolveHooksDir(repoRoot) twice on a cache miss: once inside peekHookSource and again inside preMergeCommitHookDrift. Each call spawns a `git rev-parse --git-path hooks` subprocess, exactly the "never exec a subprocess per item (git rev-parse once did)" anti-pattern docs/development/high-performance-go.md calls out under "Skip work you don't need". Resolve it once in driftParts and thread the result into both helpers. The result is already cached per repoRoot for the process lifetime (driftCache), so this halves one subprocess spawn per repo per process rather than per file — a small but real, unambiguous win with no measurement needed to justify avoiding a redundant fork/exec. TestDriftParts_ResolvesHooksDirOnce substitutes a counting stub for the new resolveHooksDir package var and asserts it is called exactly once per driftParts cache miss. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016wWQmrtb8EMbn1hDgkESqE
8ec143a to
e8d7647
Compare
|
🟢 Merge Queue — picked up This PR is in the queue and will be batched with other Next: No action needed — you'll get another comment when CI starts on the batch. View merge queue run. |
|
🔵 Merge Queue — CI running Merged into batch branch Next: No action needed — you'll be notified when CI completes. |
|
✅ Merge Queue — merged This PR landed on Next: Done — nothing more to do here. |
Summary
Audited the Go core against
docs/development/high-performance-go.mdusing a fleet of scanning agents plus CPU/alloc profiling ofBenchmarkCheckCorpusLarge(per the doc's own "profile before you rewrite" methodology). Most textbook anti-patterns the doc calls out (regex compiled per-call, missing pre-sized slices, redundant AST walks,[]T{}instead ofnil,ToLower/ToUpperallocations) do not survive measurement in this codebase — it already went through a dedicated optimization pass (the "plan 195" / "plan 2606…" comments throughoutinternal/). Several promising-looking leads were implemented, benchmarked, and then reverted when they showed zero measurable improvement (e.g. convertingast.Walkclosures to direct recursion — Go's escape analysis already keeps the walker off the heap here).Three issues did survive rigorous red/green verification (failing benchmark/test against the pre-fix code, passing after) and are fixed here, ranked by measured impact:
no-reference-style(MDS043) — missing byte-needle gate before two full-source regex scans.checkFootnotesranfootnoteRefRE/footnoteDefREover the entire source on everyCheck, even when the file has no footnote syntax at all. Added the samebytes.Contains(source, "[^")pre-check the doc's own example (mayContainURLin MDS012) already uses. Measured: ~253,800 ns/op → ~4,500 ns/op (~56x) on a footnote-free fixture.build(MDS039) —strings.ToUpperallocation on every path segment.hasReservedDeviceNameuppercased every segment before a set lookup;ToUpperonly allocates when the input isn't already the target case, so real (lowercase) segments always paid for it. Every reserved name is 3–4 bytes, so a length pre-check skips the allocation for segments that can never match. Measured: 602 ns/op & 10 allocs/op → ~380 ns/op & 5 allocs/op.git-hook-sync(MDS048) — redundantgit rev-parsesubprocess.driftPartsresolved the hooks directory twice on a cache miss (once inpeekHookSource, again inpreMergeCommitHookDrift), each spawning a real OS subprocess — the exact "never exec a subprocess per item" anti-pattern the doc names. Resolved once and threaded through both helpers.Each fix has a dedicated regression test/benchmark with a pinned budget (
b.Fatalf/t.Fatalfon overshoot), verified to fail against the pre-fix code and pass after.Test plan
go build ./...go test ./...(full suite green)go vet ./...go tool -modfile=tools/go.mod golangci-lint runon changed packages (0 issues)internal/integrationalloc-budget and rule-walk-audit manifest gates pass unchangedgit stash/git applyround-trips during development)mdsmith check .on the repo (563 files, 0 failures)Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_016wWQmrtb8EMbn1hDgkESqE
Generated by Claude Code