perf: fix top 5 high-performance-go.md violations (round 8)#740
Draft
jeduden wants to merge 10 commits into
Draft
perf: fix top 5 high-performance-go.md violations (round 8)#740jeduden wants to merge 10 commits into
jeduden wants to merge 10 commits into
Conversation
File is allocated once per NewFile call — the single most common allocation across the engine's per-file Check path. Its 24 lazy-init guard fields (10 atomic.Bool + 10 sync.Mutex + one sync.Once) were each declared beside the cache field they protect; that interleaves 4-byte-aligned scalars between 8-byte-aligned pointer/slice/map fields, forcing a 4-byte pad after nearly every guard pair. Grouping every guard into one block after all pointer-bearing fields (each guard's comment still names the cache field it protects) packs the struct from 688 to 640 bytes with no behavior change. Red: TestFile_SizeBudget failed at 688 bytes against a 640-byte budget. Green: 640 bytes after the reorder. Ref: docs/development/high-performance-go.md#struct-layout
buildSectionParagraphs allocates one SectionParagraph per document paragraph, backing MDS023 (paragraph-readability) and MDS024 (paragraph-structure) — both default-enabled — so this is likely the highest-frequency struct construction in the rule set. Line (a plain int) was declared before Node/Text, its only pointer-bearing fields; the GC's per-word ptrdata scan therefore covered Line's word too. Moving Node/Text first drops ptrdata from 32 to 24 bytes with no change in total struct size. Red: TestSectionParagraph_PointerFieldsPrecedeScalars failed (Line declared before Node). Green: passes after the reorder. Ref: docs/development/high-performance-go.md#struct-layout
parseTables allocates one table (and one tableRow per source row) for every table MDS026 scans. The sibling tablefmt package already reordered its near-identical table/row structs for the same GC-scan reason (commit 9284744, "GC scan reduction for ... table, row structs"), but this package's local copy — cells/rows built independently for the zero-alloc [][]byte cell representation — was never updated to match. startLine/line/isSeparator (scalars) preceded the pointer-bearing rows/cells slices, so the GC's ptrdata scan covered those scalars too. Reordering drops each struct's ptrdata from 16 to 8 bytes with no change in total struct size. Red: TestTable_PointerFieldsPrecedeScalars and TestTableRow_PointerFieldsPrecedeScalars failed (scalar fields declared before the pointer-bearing slice). Green: both pass after the reorder. Ref: docs/development/high-performance-go.md#struct-layout
ValidateIndex read the on-disk <?catalog?>/index side-output via a
raw os.ReadFile, the only cross-file read in this package that
bypassed the bytelimit convention every sibling call site follows
(FileReader.readPath in this package; the include, catalog, and
cross-file-reference-integrity rules). It runs once per Check for
every file whose schema declares index:, so an oversized on-disk
artifact was read into memory in full regardless of f.MaxInputBytes.
Route the read through bytelimit.ReadFileLimited, capped by
f.MaxInputBytes (falling back to bytelimit.DefaultMaxInputBytes when
unset, matching readPath).
Red: TestValidateIndex_RespectsMaxInputBytes failed — the oversized
fixture read in full and produced the generic "out of date"
diagnostic instead of a "too large" read error.
Green: passes after routing through bytelimit.ReadFileLimited.
Ref: docs/development/high-performance-go.md ("os.ReadFile on huge
inputs")
findMatches probes every word-boundary candidate position in a Text
node's body against every defined abbreviation term, and bestMatchAt
converted each term from its map key to []byte on every single
(position, term) probe — positions vastly outnumber terms in real
prose, so this was the dominant allocator on any document with a
defined abbreviation (the extension is wired into every default-
flavor parse). Precompute each term's []byte form once per
findMatches call via tableTerms instead of re-converting it on every
probe.
Red: TestBestMatchAt_AllocBudget failed to compile (tableTerms did
not exist); with the prior table-based bestMatchAt it would have
shown 5 allocs/call for a 5-term table, once per call regardless of
match.
Green: 0 allocs/call after precomputing terms once outside the
per-position loop.
Ref: docs/development/high-performance-go.md ("Stay in []byte")
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:
|
Round-1 code review (5 independent finder angles, xhigh severity) on this PR flagged that TestFile_SizeBudget alone only pins File's total byte count — a future edit could keep the struct at 640 bytes while moving a pointer-bearing field below a scalar guard field, silently undoing the GC-ptrdata benefit the reorder commit's comment describes, with nothing catching it. Add the project's existing structlayout.AssertPointerFieldsFirst assertion (already used by internal/rules/propernames/wrongmatch_layout_test.go and others) alongside the size budget so both halves of the layout are guarded. Ref: docs/development/high-performance-go.md#struct-layout
Round-1 code review (5 of 8 finder angles, independently) flagged that indexMaxBytes, added in 658e6bf, substituted bytelimit.DefaultMaxInputBytes whenever f.MaxInputBytes was <= 0. That contradicts lint.File.MaxInputBytes's own documented contract ("zero or negative means unlimited") and every other cross-file read in the codebase, which passes f.MaxInputBytes straight into bytelimit.Read*Limited with no fallback (crossfilereferenceintegrity, include, requiredstructure's fileMaxBytes, and this package's own FileReader.readPath all honor it — the fallback pattern in readPath that the removed helper's comment cited as precedent belongs to a different field, FileReader .MaxBytes, whose zero value means "caller forgot to set it" rather than "unlimited"). A caller that leaves MaxInputBytes at zero (the LSP server, a direct pkg/mdsmith.Session, or `--max-input-size 0`) got a silent 2 MB cap reimposed only for the schema index side-output read. Delete the helper and pass f.MaxInputBytes straight through, matching every sibling call site. Red: TestValidateIndex_ZeroMaxInputBytesIsUnlimited failed — an oversized index file with f.MaxInputBytes left at its zero value hit the "too large" read error instead of the expected "out of date" diagnostic. Green: passes after removing the fallback. Ref: docs/development/high-performance-go.md ("os.ReadFile on huge inputs")
Round-1 code review (3 of 8 finder angles, independently, including a quantified estimate: ~200x more allocations than necessary on a document with 200 paragraph-level Text nodes and a 20-term glossary) flagged that c35dfc4's tableTerms(table) call inside findMatches still ran once per ast.Text node, not once per document — table is loaded once in Transform and never mutated afterward, so rebuilding the []byte term forms on every node repeated the same conversion once per paragraph/heading/list-item instead of once total. Move the tableTerms(table) call up into Transform and thread the resulting []tableTerm through rewriteText and findMatches instead of the raw table. Also removes the len(table) == 0 guard findMatches carried: that branch was already unreachable (Transform early-returns on an empty table before ever calling rewriteText/findMatches) and untested dead code, a violation of CLAUDE.md's "Defensive Code" rule the conventions-angle reviewer flagged separately — the coverage gate this round's CI run also caught it directly (90% patch coverage on this file). Red: TestFindMatches_AllocBudget failed to compile in the prior revision (findMatches took table abbrTable, not a precomputed terms slice); with the pre-hoist implementation it would have shown 5 allocs/call for a 5-term table on every invocation, once per Text node. Green: 0 allocs/call once terms is threaded in from Transform's single per-document computation. Ref: docs/development/high-performance-go.md ("Stay in []byte"; "Skip work you don't need")
Round-2 code review flagged that TestFile_SizeBudget's comment described the trailing scalar order as "the two 8-byte scalars next, the two byte-sized bools last", the opposite of what file.go actually declares (StripFrontMatter, DryRun, then LineOffset, MaxInputBytes). Fix the comment to match the real layout.
Round-2 code review's efficiency angle re-ran fieldalignment after this PR's File reorder settled and found a residual, smaller finding beyond the original 688->640 size fix: "struct with 488 pointer bytes could be 472". RunCache *RunCache sat last among the pointer-bearing fields; a bare pointer's entire width is its GC pointer word, so ending the block there left no free non-pointer tail. Reordering so linkRefs []Reference is the last pointer-bearing field instead lets its 16-byte len/cap tail fall outside ptrdata for free, since a slice only GC-scans its 8-byte data-pointer word. Ref: docs/development/high-performance-go.md#struct-layout
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
2 participants
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
An audit against
docs/development/high-performance-go.md(dispatched as four parallel research agents scanning for regexp/allocation, memoization/skip-work, struct-layout/data-structure, and concurrency/misc anti-patterns) surfaced several violations that survived seven earlier perf-cleanup rounds (#723, #725, #729, #731, #732, #735, #737). Prior rounds had already clearedgolangci-lint'sperfsprint/prealloc/gocritic(performance) checks to 0 issues repo-wide, so this round's findings are all outside what those linters catch. This PR fixes the top 5, ranked by hotness and impact, each with red/green TDD.internal/lint.Filestruct size (688 → 640 bytes) —Fileis allocated once perNewFilecall, the single most common allocation across the engine's per-fileCheckpath. Its 24 lazy-init guard fields (10atomic.Bool+ 10sync.Mutex+ onesync.Once) were each declared beside the cache field they protect, interleaving 4-byte-aligned scalars between 8-byte-aligned pointer/slice/map fields and forcing a padding gap after nearly every guard pair. Grouped every guard into one block after all pointer-bearing fields — each guard's comment still names the cache field it protects, only the declaration site moved.internal/rules/astutil.SectionParagraphptrdata (32 → 24 bytes) —CollectSectionParagraphsallocates oneSectionParagraphper document paragraph, backing MDS023 (paragraph-readability) and MDS024 (paragraph-structure), both default-enabled — likely the highest-frequency struct construction in the rule set.Line(a plain int) was declared beforeNode/Text, its only pointer-bearing fields, so the GC's per-word ptrdata scan coveredLine's word too. MovedNode/Textfirst.internal/rules/tablereadabilitytable/tableRowptrdata (16 → 8 bytes each) — the siblingtablefmtpackage already reordered its near-identicaltable/rowstructs for this exact reason (commit9284744f), buttablereadability's local copy — kept separate for its zero-alloc[][]bytecell representation — was never updated to match.parseTablesallocates one of each per table/row MDS026 scans.internal/schema.ValidateIndexunbounded read — read the on-disk<?catalog?>/index side-output via a rawos.ReadFile, the only cross-file read in the package that bypassed thebytelimitconvention every sibling call site follows (FileReader.readPathin the same package; the include, catalog, and cross-file-reference-integrity rules). Routed throughbytelimit.ReadFileLimited, capped byf.MaxInputBytes(falling back tobytelimit.DefaultMaxInputBytes). Matches the guideline's named anti-pattern table entry ("os.ReadFileon huge inputs").pkg/markdown/flavor/extabbreviation matching —findMatchesprobes every word-boundary candidate position in a Text node's body against every defined abbreviation term, andbestMatchAtconverted each term from its map key to[]byteon every single (position, term) probe. Positions vastly outnumber terms in real prose, so this was the dominant allocator on any document with a defined abbreviation (the extension is wired into every default-flavor parse). Precomputed each term's[]byteform once perfindMatchescall instead.Each fix is its own commit with a full red/green TDD trail (failing test first, then the fix) and cites the specific guideline section it addresses. Struct-layout fixes use the project's
internal/structlayout.AssertPointerFieldsFirsthelper where the finding is a pointer-ordering issue (SectionParagraph, table/tableRow); theFilefix pins an exactunsafe.Sizeofbudget instead, since it's a total-size/padding finding, matching the precedent ininternal/lint/layout_test.go.Test plan
go build ./...go test ./...(full suite green)go vet ./...go tool -modfile=tools/go.mod golangci-lint run ./...(0 issues)go run ./cmd/mdsmith check .(549 files, 0 failures)internal/integrationper-rule alloc-budget gate still green for every ruleGenerated by Claude Code