perf: audit against high-performance-go.md, fix 5 measured hot paths - #774
Draft
jeduden wants to merge 5 commits into
Draft
perf: audit against high-performance-go.md, fix 5 measured hot paths#774jeduden wants to merge 5 commits into
jeduden wants to merge 5 commits into
Conversation
matchesAny called doublestar.Match, which re-runs doublestar.ValidatePattern's parse walk on every file x pattern comparison during the workspace walk, even though validatePatterns already filters w.patterns to the valid set up front. Per docs/development/high-performance-go.md's "skip work you don't need", internal/globpath already fixed this exact shape for the config-driven glob surfaces; this mirrors it for file discovery, which runs on every check/fix/lsp invocation over every file in the tree. BenchmarkMatchesAny: ~780 ns/op -> ~685 ns/op (0 allocs/op both ways).
orderFilesLeavesFirst called index.(*Index).BuildSerial even though its loader (bytelimit.ReadFileLimited keyed by a read-only relToAbs map, each call opening its own file handle) is safe for concurrent use. index.(*Index).Build fans the same extractor across runtime.GOMAXPROCS(0) workers for exactly this shape of workload (internal/index's own Build-vs-BuildSerial benchmarks show ~2x on a 1k-file synthetic corpus); this call site paid the fully serial cost on every `mdsmith fix` run over the workspace before the fix pass even starts. BenchmarkOrderFilesLeavesFirst (500 files): ~30ms -> ~26ms.
sortEntries' numeric-sort comparator called parseSortInt (a fieldinterp.ParseCUEPath tokenize plus a map walk plus strconv.ParseInt) on every comparison, i.e. O(n log n) times, even though allParseAsInt already parses every entry once just to decide whether numeric mode applies. Per docs/development/high-performance-go.md's "memoize per-input computations" (the same pattern the string-sort path already applies to its lowercase keys), the parsed int64 is now cached alongside the existing sortKey/tiebreakerKey precompute and reused in the comparator. BenchmarkSortEntriesNumeric (5000 entries): ~23.4ms/307798 allocs -> ~3.9ms/29806 allocs.
BaseNode declared childCount (a scalar) between its Node pointer fields and attributes ([]Attribute, pointer-ish), extending the struct's GC ptrdata over a field that carries no pointer. Per docs/development/high-performance-go.md's "group pointer fields first, scalars last", every other struct-layout test in this package (TestAutoLink_PointerFieldsBeforeScalars et al.) checks this rule via fieldorder.AssertPointersBeforeScalars, but all of them skip index 0 to exclude the embedded BaseNode itself, so its own layout had never been checked. BaseNode is embedded in BaseBlock and BaseInline, i.e. in every concrete AST node type parsed, making it the highest-multiplicity struct in the parser. No unsafe code depends on the field order (confirmed via grep across pkg/goldmark); reordering only changes GC scan cost per node, not the struct's size.
SectionBody linearly scanned the entire paragraph slice on every call, filtering by line range. MDS057 (required-text-patterns) and MDS058 (required-mentions) both call it once per heading over the same paragraph slice, turning a single Check into O(headings * paragraphs) instead of O(headings * log paragraphs). Both headings and paragraphs are already sorted by source line (CollectSectionParagraphs walks the AST in document order), so per docs/development/high-performance-go.md's "sorted slice + binary search" pattern, sort.Search now bounds the matching range directly instead of scanning every paragraph on every call. BenchmarkSectionBodyPerHeading (500 headings x 5 paragraphs each): ~1.44ms/1500 allocs -> ~0.12ms/1000 allocs.
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:
|
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
Audited the Go core against
docs/development/high-performance-go.mdusing a fleet of scanning agents (internal/rules/**,internal/lint/internal/mdtext/pkg/markdown/pkg/goldmark/internal/engine, andcmd/mdsmith/internal/lsp/internal/discovery/internal/index) plus targeted before/after benchmarking of each candidate (per the doc's own "profile before you rewrite" methodology).This repo has already been through several rounds of this exact audit (#762, #764, #765, #767), and — while scanning — I found #770 (branch
claude/kind-darwin-v8cn6a) already open with a fifth, independent round coveringHeadingText/HeadingTextBasememoization,schema.ExtractDocHeadingsmemoization,Slugifypresizing, a goldmark bulk-copy fix, and a disabled-logger allocation. This PR deliberately does not touch any of that ground — all five fixes below are distinct hot paths #770 doesn't cover, so the two PRs should merge cleanly independent of order.Five issues survived measurement and are fixed here, each its own commit with a dedicated before/after benchmark:
internal/discovery—matchesAnyre-validated already-validated patterns.validatePatternsfiltersw.patternsto the valid set once up front, butmatchesAnythen calleddoublestar.Match, which re-runsdoublestar.ValidatePattern's parse walk on every file × pattern comparison during the workspace walk.internal/globpathalready fixed this exact shape for the config-driven glob surfaces (its own comment cites "~4% of check CPU"); this mirrors it for file discovery, which runs on everycheck/fix/LSP-index invocation over every file in the tree.BenchmarkMatchesAny: ~780 ns/op → ~685 ns/op (0 allocs/op both ways).cmd/mdsmith—orderFilesLeavesFirstbuilt its dependency index serially. The loader (bytelimit.ReadFileLimitedkeyed by a read-only map, each call opening its own file handle) is safe for concurrent use, but the call usedBuildSerialinstead of the parallelBuildthatinternal/indexships specifically for this shape (its own benchmarks show ~2x on a 1k-file corpus). Everymdsmith fixrun paid the fully serial cost before the fix pass even starts.BenchmarkOrderFilesLeavesFirst(500 files): ~30 ms → ~26 ms.catalog(MDS-catalog) — numeric sort re-parsed every entry on every comparison.sortEntries' numeric-mode comparator calledparseSortInt(a CUE-path tokenize + map walk +strconv.ParseInt) on every comparison — O(n log n) re-parses — even thoughallParseAsIntalready parses every entry once just to decide whether numeric mode applies. The parsedint64is now cached alongside the sort's existing lowercase-key precompute.BenchmarkSortEntriesNumeric(5000 entries): ~23.4 ms / 307,798 allocs → ~3.9 ms / 29,806 allocs.pkg/goldmark/ast—BaseNode'schildCountsat between pointer fields.BaseNodedeclared a scalar (childCount) between itsNodepointer fields andattributes([]Attribute, pointer-ish), extending GC ptrdata over it for nothing. Every other struct-layout test in the package skips index 0 (the embedded base), soBaseNode's own layout had never been checked — despite being embedded in every concrete AST node type parsed. Nounsafecode depends on the field order (verified via grep).astutil.SectionBodyscanned every paragraph on every call. MDS057 (required-text-patterns) and MDS058 (required-mentions) each callSectionBodyonce per heading over the same paragraph slice, making a singleCheckO(headings × paragraphs). Both headings and paragraphs are already sorted by source line, sosort.Searchnow bounds the matching range directly instead of a full scan.BenchmarkSectionBodyPerHeading(500 headings × 5 paragraphs): ~1.44 ms / 1500 allocs → ~0.12 ms / 1000 allocs.Test plan
go build ./...go test ./...(full suite green, 161 packages)go vet ./...go test -raceon every touched package (discovery, cmd/mdsmith, catalog, goldmark/ast, astutil, index)go tool -modfile=tools/go.mod golangci-lint runon changed packages (0 issues)internal/integrationper-rule alloc/timing budget gates and the rule-walk-audit manifest pass unchanged (MDS057/MDS058's budgets specifically re-verified after theSectionBodychange)mdsmith check .on the repo (567 files, 0 failures)Co-Authored-By: Claude Sonnet 5
Generated by Claude Code