perf: audit against high-performance-go.md, fix 5 confirmed hot paths - #785
Draft
jeduden wants to merge 14 commits into
Draft
perf: audit against high-performance-go.md, fix 5 confirmed hot paths#785jeduden wants to merge 14 commits into
jeduden wants to merge 14 commits into
Conversation
…tion resolveEffectiveKinds ran matchKindAssignmentEntry for every kind-assignment entry on every workspace file and always built formatSelector's provenance string, then threw it away. Only provenance.go needs that string; resolveEffectiveKinds only needs the bool. Split out kindAssignmentEntryMatches so the hot per-file path skips the wasted allocation. Ref docs/development/high-performance-go.md "Skip work you don't need". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y1J29TYsYdQrUeMxSAbox
walker.matchesAny called doublestar.Match directly once per configured pattern for every file the workspace walk visits, which re-validates the pattern's syntax on every call. globpath already memoizes that validation for every other config glob surface; add MatchRaw, which gives the top-level files: key the same cached-validation path while preserving its documented raw-path semantics (no basename or cleaned-path fallback, unlike globpath.Match). Ref docs/development/high-performance-go.md "Skip work you don't need". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y1J29TYsYdQrUeMxSAbox
MDS057 and MDS058 call SectionBody once per heading, and each call rescanned the full paragraphs slice from index 0 looking for the ones inside that heading's line range. Heading start lines are non-decreasing in document order, so a single forward-only cursor into paragraphs can skip past everything before the current section without ever revisiting it. Add SectionBodies, which computes every heading's body in one sweep, and switch both callers to it. Ref docs/development/high-performance-go.md "Skip work you don't need". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y1J29TYsYdQrUeMxSAbox
…heck CheckStaleness resolved a target's inputs and outputs, then called ComputeActionID, which re-ran the same resolveInputs/resolveOutputs from scratch -- a second glob-expansion and symlink-resolution pass over the same target on every check/fix run that declares a <?build?> target. Reuse the already-resolved slices via computeActionIDFromResolved, matching what RecordBuild already does. Ref docs/development/high-performance-go.md "Skip work you don't need". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y1J29TYsYdQrUeMxSAbox
mergeAny deep-cloned the earlier side of a list-typed settings leaf via toAnySlice before checking whether the rule's merge mode was Append or the default Replace, discarding that clone on the (common) Replace path. It then made a second independent copy of the later side, which toAnySlice had already cloned into a fresh backing array. Check the merge mode first, only clone earlier when Append actually needs it, and return the already-independent later clone directly on Replace. Ref docs/development/high-performance-go.md "Skip work you don't need". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y1J29TYsYdQrUeMxSAbox
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:
|
… walk" This reverts commit 09f7af5.
mergeAny's list-replace path started returning toAnySlice's clone
directly instead of append([]any(nil), ll...). toAnySlice always
returns a non-nil make([]any, 0) even for an empty input, so an empty
later list started merging to a non-nil []any{} instead of nil --
observable in JSON (`null` vs `[]`) and reflect. Fold the empty case
back to nil explicitly.
Found by an xhigh-severity review pass on PR #785.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016Y1J29TYsYdQrUeMxSAbox
SectionBodies's forward-only cursor breaks out of a heading's collection loop as soon as it sees a paragraph past that heading's end, unlike SectionBody, which tolerates an unordered slice. That's only safe because CollectSectionParagraphs happens to return paragraphs in ascending Line order (an artifact of AST walk order plus lint's parser installing no node-relocating extension). Document both sides of that contract explicitly so a future caller doesn't pass SectionBodies a differently-sourced, unsorted paragraph slice. Found by an xhigh-severity review pass on PR #785; verified with a 1,034,800-execution differential fuzz test against the old per-heading SectionBody+SectionEnd loop across the repo's full Markdown corpus, no divergence found -- this commit only closes the documentation gap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y1J29TYsYdQrUeMxSAbox
collectBlocksL0 (MDS065's default-on Layer 0/nil-AST path, run on every workspace file when Layer 0 gating applies) grew `blocks` via unsized append even though len(lint.Layer0(f).BlockSpans) is a ready upper bound known before the loop starts -- BlockSpans covers every block kind, code blocks being a subset. Presize to it. Replaces the discovery.go/globpath.MatchRaw change reverted earlier in this branch: an xhigh-severity review pass benchmarked it and found no measurable improvement (benchstat p=0.912) while it also introduced a matching-semantics gap between MatchRaw and doublestar.Match. This is its replacement in the top-5 audit against docs/development/high-performance-go.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y1J29TYsYdQrUeMxSAbox
Presizing to len(BlockSpans) counted every block kind, not just code blocks. An xhigh-severity review pass measured this as a net regression on the common case: on this repo's own L0-eligible corpus, most files have zero code blocks against dozens of other spans (~20x over-allocation on average), turning a 0-alloc/338ns path into a 1-alloc/~4800ns one. Count the actual code-block spans first (a cheap second switch-only pass, no allocation) and presize to that instead -- a code-block-free file now allocates nothing, same as before this fix's first draft, while a code-heavy file still gets the tight presize. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y1J29TYsYdQrUeMxSAbox
… memory trade-off Round 2 of xhigh-severity review on PR #785 found the sorted-paragraphs precondition documented in the prior commit omitted that headings must also be ascending by Line -- the same forward-only cursor depends on both orderings, and CollectSectionHeadings's own sort is what makes it safe today. Also documents the peak-live-memory trade-off (every section body stays live for the whole call instead of becoming garbage per heading) so it is a legible, deliberate choice rather than an unstated side effect. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y1J29TYsYdQrUeMxSAbox
collectBlocksL0's counting pass used `for _, span := range spans`. lint.BlockSpan is a 32-byte struct, so range-by-value copies the full struct to the stack every iteration just to read Kind. An xhigh-severity review pass (round 3) measured this as making the common code-block-free case ~1.85x slower than origin/main -- the opposite of what the counting pass exists to protect. Index instead (`spans[i].Kind`), which benchmarks faster than both the range-copy version and origin/main's original single-pass loop, while keeping round 2's win on code-heavy files intact. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y1J29TYsYdQrUeMxSAbox
SectionBodies declared `var parts []string` inside the per-heading loop, so every heading regrew a fresh nil slice from cap 1 up. An xhigh-severity review pass (round 3) measured this as MORE total allocations (10) than the per-heading SectionBody loop it replaces (6, on a 3-heading nested fixture) -- the opposite of what the O(H×P)-> O(H+P) rewrite was for. Hoist `parts` out of the loop and reuse it via `parts[:0]`, the project's documented pattern for loop-local buffers. Corrects the doc comment's now-stale "total allocation volume is unchanged" claim and pins the fixed budget with a new test. Also adds a consistency test between isAnySliceType and toAnySlice in internal/config (a lower-severity finding from the same review round): the two type switches are hand-maintained in sync with nothing enforcing agreement, so a future slice type added to one but not the other would silently change list-merge behavior with no other test to catch it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y1J29TYsYdQrUeMxSAbox
Split out from the previous commit -- the file was missed in that git add. See that commit's message for the rationale: isAnySliceType and toAnySlice are two hand-maintained copies of the same type switch with nothing enforcing they stay in sync. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016Y1J29TYsYdQrUeMxSAbox
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
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
Scheduled performance audit of the codebase against
docs/development/high-performance-go.md, following that doc's"apply best-practice patterns first, then measure" playbook. Five
parallel agents scanned the core lint pipeline, all
internal/rules/*packages (split alphabetically), and the supporting/infra packages for
violations of the documented patterns. Findings were ranked by how hot
the call site is and how confidently the fix preserves existing
behavior; the top 5 were fixed, each with a red/green test.
Three rounds of xhigh-severity adversarial review ran against this
PR, each fixing what the previous round found, including two cases
where a "fix" was itself measurably wrong — exactly the failure mode
this kind of PR is most at risk of. The corrections are their own
commits so the history shows what was found and how it was fixed,
rather than being squashed away.
Two other candidate findings from the initial scan were investigated
and deliberately not applied:
mdtext.ExtractPlainTextinforbiddentext/forbiddenparagraphstarts(MDS055/MDS056) lookedpromising but is unsafe: markdown markup stripped during plain-text
extraction (e.g.
wor**d**→word) can join a configuredsubstring that never appears contiguously in the raw source, so a
raw-source pre-check would introduce false negatives.
occurrence(MDS060) andmaxsectionlength(MDS036), usingdifferent aggregation logic than
astutil.SectionBody. Left for afollow-up rather than risking a rushed fix across three different
per-range aggregation loops.
Final fixes (5)
internal/config: skip a discarded provenance-selectorallocation in kind resolution.
resolveEffectiveKindsruns onevery workspace file (it feeds
EffectiveSignature, the configcache key) and, for every matching
kind-assignmententry, calledmatchKindAssignmentEntry, which unconditionally builtformatSelector's provenance string and threw it away. Split outa bool-only
kindAssignmentEntryMatchesfor the hot path.internal/rules/astutil: turnSectionBody's O(H×P) per-headingrescan into O(H+P). MDS057/MDS058 call
SectionBodyonce perheading, each call rescanning the entire paragraphs slice from
index 0. Heading start lines are non-decreasing in document order,
so a single forward-only cursor can skip past already-passed
paragraphs. Added
SectionBodies, computing every heading's bodyin one sweep, with its per-heading text buffer hoisted and reused
(
parts[:0]) rather than re-declared each iteration — the firstversion of this fix actually allocated more than the code it
replaced (10 allocs vs. 6 on a nested-heading fixture); review
round 3 caught it.
internal/build: resolve build-target inputs/outputs once perstaleness check.
CheckStalenessresolved a target's inputs andoutputs, then called
ComputeActionID, which re-ran the sameresolveInputs/resolveOutputsfrom scratch. Reuses thealready-resolved slices via
computeActionIDFromResolved, matchingwhat
RecordBuildalready did.internal/config: skip cloning the discarded side of alist-setting merge.
mergeAnydeep-cloned the earlier side ofa list-typed settings leaf before checking whether the merge mode
was
Appendor the defaultReplace, discarding the clone on thecommon
Replacepath, then made a second redundant copy of thealready-cloned
laterside. Check the mode first; only cloneearlierwhenAppendneeds it; returnlater's clone directlyon
Replace— with an explicit fold back tonilfor an emptylater list, since review round 1 caught that the direct-return
version silently turned a merged
nilinto a non-nil[]any{}(observable in
mdsmith kinds resolve --json).internal/rules/codeblockstyle: pre-sizecollectBlocksL0'sblock slice to the actual code-block count, not
len(BlockSpans)(which counts every block kind — headings,paragraphs, lists, quotes — so presizing to it over-allocates on
the common case of a mostly-prose file). Counts the real code-block
spans in a first cheap pass, indexing rather than range-copying the
32-byte
BlockSpanstruct (review round 3 caught that range-by-valuemade the common case measurably slower than doing nothing).
Reverted from the original top 5
internal/discovery/internal/globpath: caching pattern validationfor the
files:glob walk. Review round 1 benchmarked it on thereal walk shape and found no significant change (benchstat p=0.912,
above the project's own p<0.05 bar), while the new
MatchRawhelperwas not actually equivalent to
doublestar.Matchon patterns thatfail
ValidatePatternbut thatMatchstill accepts. No live bug atthe one call site, but not worth carrying a correctness trap for a
change with no measured benefit — reverted outright and replaced by
fix 5 above.
What each review round found and fixed
[]correctness regression in fix 4's replacepath (fixed); no measured benefit + a semantic gap in the
discovery/globpath change (reverted); missing precondition docs on
SectionBodies(added).len(BlockSpans)) wasa measured regression on the common case — ~20x over-allocation,
~14x slower on a prose-only fixture (fixed with a count-first pass);
the
SectionBodiesprecondition doc named only paragraph ordering,missed that headings must also be sorted (added); an undocumented
peak-live-memory trade-off in the batch API (documented).
32-byte
BlockSpanstruct, making the fix for the regression~1.85x slower than doing nothing on the common case (fixed by
indexing instead); fix 2's hoisted
partsbuffer wasn't actuallyhoisted, so it allocated more than the O(H×P) code it replaced
(fixed); a doc comment claiming unchanged allocation volume was
measurably false (corrected); added a consistency test between two
hand-maintained type switches (
isAnySliceType/toAnySlice) thathad nothing enforcing they stay in sync.
Every fix above and every correction was independently verified with
its own differential/fuzz test, allocation-budget test, or benchmark —
not just asserted. Full details are in the individual commit messages.
Test plan
testing.AllocsPerRunbudget test, or an equivalence/call-count test where allocation
counting doesn't capture the win) written before the fix, shown
failing, then passing after.
go build ./...go test ./...(all 161 packages pass)go vet ./...go tool -modfile=tools/go.mod golangci-lint run ./...(0 issues,full repo)
go run ./cmd/mdsmith check .(567 files checked, 0 failures)internal/integrationcorpus/allocation-budget suites passunchanged
every confirmed finding from the previous round
Ref:
docs/development/high-performance-go.md🤖 Generated with Claude Code