perf: fix top 5 high-performance-go violations (LSP/config/build reflect sorts + MDS005) - #840
Draft
jeduden wants to merge 6 commits into
Draft
perf: fix top 5 high-performance-go violations (LSP/config/build reflect sorts + MDS005)#840jeduden wants to merge 6 commits into
jeduden wants to merge 6 commits into
Conversation
astutil.CollectHeadingNodes already returns a slice of known length right before the map is built, but the seen map started at zero capacity and grew through several bucket reallocations on documents with many headings. MDS005 is default-enabled and runs on every workspace file, per docs/development/high-performance-go.md's "pre-size slices" pattern extended to a map's bucket array. Verified red (97 allocs/op on an 80-heading fixture) before the fix, green (91 allocs/op) after. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018CxpCyTLCf454rQXN9sDMC
sort.Slice/sort.SliceStable drive reflect.Swapper internally — the "reflect in hot paths" anti-pattern documented in docs/development/high-performance-go.md, already fixed the same way elsewhere in this codebase. Five call sites in the LSP server still used it: completion.go's sortItems (every textDocument/completion request), symbols_workspace.go's handleWorkspaceSymbol (every workspace/symbol query), and four near-identical location sorts in symbols_navigation.go (every textDocument/references and workspace-symbol-by-kind request) — all on request paths the editor fires on every keystroke-driven navigation or completion query. The four location sorts are consolidated into one sortLocationsByURIThenLine helper, since three shared an identical comparator and the fourth (URI-only) produces the same order when every entry's Range.Start.Line ties at zero. Verified red (sort.Slice measured at 2 allocs/op regardless of input size) before the fix, green (0 allocs/op) after, for all three extracted sort helpers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018CxpCyTLCf454rQXN9sDMC
kind_files.go, schema_files.go, convention_files.go, and wordlist_files.go each called sort.Slice on the result of os.ReadDir — but os.ReadDir already "returns the entries sorted by filename" per its own doc, so the sort was both a reflect-driven sort.Slice call and pure dead work (docs/development/high-performance-go.md, "skip work you don't need"). Each discover* function runs on every LSP config reload (workspace open, or a kind/schema/convention/wordlist file save) and every CLI config load. Added tests proving the basename-collision error still names files in the correct (filename-sorted) order regardless of on-disk creation order — the behavior the removed sort used to guarantee — plus an alloc-budget test on discoverKinds verified red (1801 allocs/op) before the fix, green (1798 allocs/op) after. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018CxpCyTLCf454rQXN9sDMC
sortByDepthThenName drove sort.Slice — reflect.Swapper internally, per docs/development/high-performance-go.md's "reflect in hot paths" — once per colliding-basename bucket on every WikilinkIndex (re)build. Real workspaces with repeated basenames (README.md, index.md) pay this N times per rebuild, not once. Verified red (sort.Slice measured at 2 allocs/op regardless of input size) before the fix, green (0 allocs/op) after. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018CxpCyTLCf454rQXN9sDMC
Cache.Save sorted c.Entries with sort.SliceStable using a comparator
that called outputSetKey — which allocates a sorted copy of the
entry's output-path set and joins it into a string — on both sides
of every comparison. That redid the same per-entry work on every
one of the O(n log n) comparisons instead of once per entry
(docs/development/high-performance-go.md, "memoize per-input
computations"), on top of sort.SliceStable's own reflect.Swapper
cost ("reflect in hot paths"). Runs on every `mdsmith build` that
persists the cache.
sortEntriesByOutputKey now computes each entry's key once
(decorate-sort-undecorate) and sorts with slices.SortStableFunc.
outputSetKey itself also switches from fmt.Fprintf to strconv,
dropping fmt's reflection-based formatting per path ("strconv over
fmt.Sprintf").
Verified red (5914 allocs/op on a 60-entry cache) before the fix,
green (242 allocs/op) after — a ~24x reduction.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018CxpCyTLCf454rQXN9sDMC
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:
|
Code review (xhigh) flagged two new alloc-budget tests with near-zero slack: MDS005's gate asserted exact equality (91 == 91), and discoverKinds' gate had a 1-allocation margin (1798 vs 1799) — both fragile to unrelated allocation drift (a Go point release, a dependency bump) on PRs that never touch these files. MDS005's gate now budgets 94 (3 allocs of headroom above the measured 91, still well below the pre-fix 97). discoverKinds' gate is replaced with a benchmark: the sort.Slice removal's win there is a small, ~constant handful of allocations (reflect.Swapper's own overhead, not something that scales with entry count) against a much larger YAML-parse-dominated total, so no hard per-op budget stays meaningful — the existing collision-order and creation-order- independence tests in the same file remain the real correctness/ regression net for that change. Also drops the now-unused raceEnabled consts in internal/config left behind by that removal. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018CxpCyTLCf454rQXN9sDMC
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
An audit against
docs/development/high-performance-go.mdran threeparallel scans: the rule packages under
internal/rules/, the LSPserver plus config/index/build/export/corpus/metrics packages, and the
core engine/lint/mdtext/
pkg/markdownhot path. The engine/lint/mdtextscan came back mostly clean — that territory has already been through
several prior optimization rounds (#811, #813, #825, #828, #833, #835,
and the still-open #838) and is heavily instrumented with its own
alloc-budget gates. The rule-package scan's single standout finding
(MDS025's duplicate table-row parse) turned out to already be a tracked,
scheduled follow-up (plan 195/181, grandfathered in
internal/integration/alloc_budget_test.goat a 60-alloc ceiling) thatneeds a dedicated single-table-walk refactor rather than a quick fix, so
it's left alone here. The five issues below came from the LSP/config/
build sweep, each verified red (measured allocs before the fix) → green
(after) with a dedicated test:
internal/rules/noduplicateheadings(MDS005, default-enabled) —Check'sseenmap started at zero capacity even thoughastutil.CollectHeadingNodesreturns a slice of known length oneline earlier. Pre-sizing it with
len(headings)avoids the bucket-array reallocations an unsized map pays for as it grows past Go's
load factor. This rule runs on every workspace file. 97 → 91
allocs/op on an 80-heading fixture.
internal/lsp—completion.go'ssortItems,symbols_workspace.go'shandleWorkspaceSymbol, and fournear-identical location sorts in
symbols_navigation.goall usedsort.Slice/sort.SliceStable, which drivereflect.Swapperinternally. These run on every
textDocument/completion,workspace/symbol, andtextDocument/referencesrequest — i.e. onevery keystroke-driven navigation or completion query in the editor.
The four location sorts share one new
sortLocationsByURIThenLinehelper. 2 → 0 allocs/op for each extracted sort helper.
internal/config—kind_files.go,schema_files.go,convention_files.go, andwordlist_files.goeach re-sorted theresult of
os.ReadDirwithsort.Slice, butos.ReadDiralready"returns the entries sorted by filename" per its own doc — the sort
was both reflect-driven and pure dead work. Runs on every LSP config
reload (workspace open, or a kind/schema/convention/wordlist file
save) and every CLI config load. 1801 → 1798 allocs/op on
discoverKindswith 20 kind files (the removed sort's own cost issmall next to the YAML-parse work the function also does per file,
but it's dead code and reflect-driven regardless).
internal/linkgraph—sortByDepthThenNameusedsort.Slice,run once per colliding-basename bucket on every
WikilinkIndex(re)build; real workspaces with repeated basenames (
README.md,index.md) pay it N times per rebuild. 2 → 0 allocs/op.internal/build—Cache.Save'ssort.SliceStablecomparatorcalled
outputSetKey(which allocates a sorted path-slice copy andbuilds a joined string) on both sides of every comparison, redoing
full per-entry work on every one of the O(n log n) comparisons
instead of once per entry. Runs on every
mdsmith buildthatpersists the cache. Fixed with a decorate-sort-undecorate
sortEntriesByOutputKey(each key computed once,slices.SortStableFuncinstead ofsort.SliceStable) plus switchingoutputSetKeyitself fromfmt.Fprintftostrconv. 5914 → 242allocs/op on a 60-entry cache — the largest win in this round, a
~24x reduction.
Each fix follows red/green TDD: a test measuring the actual allocation
count was written first and confirmed to fail against the un-fixed code
(either by running it before the fix, or by temporarily reintroducing
the old implementation), then the fix landed and the same test was
confirmed green with the real measured budget.
Deferred (out of scope for this PR)
internal/rules/tableformat):Checkruns two independent per-line table parsers over the samef.Lines(tablefmt.tryParseTableandstructure.findStructureTables).This is the single worst allocation outlier in the rule set (60 allocs
vs. the project's 10-alloc ceiling), but it's already a tracked,
scheduled follow-up (plan 195/181) needing a single-table-walk
refactor across a 700+ line file — too large and too risky for a
routine automated sweep of a default-enabled, widely-used rule.
sort.Slicesites (once-per-CLI-invocation code in
internal/corpus,internal/export,internal/metrics) were noted but left alone in favor of thehigher-frequency LSP/config/build sites above.
Test plan
go build ./...go vet ./...go test ./...(all packages green)go tool -modfile=tools/go.mod golangci-lint runon changedpackages (0 issues)
go run ./cmd/mdsmith check .(588 files, 0 failures)→ green (fix applied, budget tightened to the real number)
🤖 Generated with Claude Code
https://claude.ai/code/session_018CxpCyTLCf454rQXN9sDMC
Generated by Claude Code