perf: fix top 5 high-performance-go violations (reflect-driven sorts + strconv) - #835
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:
|
buildDirIndex sorted each directory's []fs.DirEntry with sort.Slice, which drives reflect.Swapper internally — the "reflect in hot paths" anti-pattern in docs/development/high-performance-go.md, already fixed the same way in astutil.sortSectionHeadings and duplicatedcontent's match sort. buildDirIndex runs once per directory on every NewMemWorkspace call, the WASM/Obsidian/embedded-host path. Extracted the sort into sortDirEntries, using slices.SortFunc with cmp.Compare on Name() instead. An alloc test pins it at 0 allocs/op; sort.Slice measured 7. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MTRxsATsWYv5rHeBsuCZuj
itemVerdict built its diagnostic message with fmt.Sprintf and two %d verbs, driving fmt's reflection-based formatting path. Every other structural rule in this codebase (listindent, headingincrement, atxheadingwhitespace, noduplicateheadings) builds the same shape of message with strconv.Itoa + string concatenation instead — ~3x faster and allocation-free, per docs/development/high-performance-go.md "strconv over fmt.Sprintf". MDS061 is enabled by default, so this runs on every list-marker violation in every workspace file. An alloc test on itemVerdict pins the fix: 2 allocs/op before, 1 after. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MTRxsATsWYv5rHeBsuCZuj
rename.go, deps.go, backlinks.go, and buildpass.go each sorted a command-output result list (rename summaries, edits-per-line, dependency edges, backlink records, build targets) with sort.Slice/sort.SliceStable, which drive reflect.Swapper internally — the "reflect in hot paths" anti-pattern in docs/development/high-performance-go.md, already fixed the same way elsewhere in this codebase (astutil.sortSectionHeadings, mdsmith.sortDirEntries). Each runs once per CLI invocation over a command-scoped list, not per workspace file, so the win is small but free and brings these five call sites in line with the project's own convention. Extracted each into a named sortXxx helper using slices.SortFunc / slices.SortStableFunc (preserving the original stable-sort semantics where SliceStable was used) with cmp.Compare / cmp.Or. An alloc test per helper pins 0 allocs/op. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MTRxsATsWYv5rHeBsuCZuj
util.PrioritizedSlice.Sort sorted with sort.Slice, which drives reflect.Swapper internally — the "reflect in hot paths" anti-pattern in docs/development/high-performance-go.md. Every default block parser, inline parser, paragraph transformer, and AST transformer the goldmark parser registers, plus every node renderer the HTML renderer registers, is ordered by this one method (parser.go's Parse, guarded by sync.Once, and renderer.go's Render setup) — so fixing it once covers five call sites across the parse/render pipeline. Parser and renderer instances are pool-bounded rather than built per file, so each Sort call is amortized, but it is on the pool-fill path every worker exercises. slices.SortFunc now compares the concrete PrioritizedValue values directly. An alloc test pins the fix at 0 allocs/op; sort.Slice measured 3. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MTRxsATsWYv5rHeBsuCZuj
Four sort.Slice call sites in internal/fix sorted a run-scoped result list (the enabled fixable rules by ID, twice; the deduplicated diagnostics by file/line/column; a file's fixed-rule counts by rule ID) with sort.Slice, which drives reflect.Swapper internally — the "reflect in hot paths" anti-pattern in docs/development/high-performance-go.md. Each runs once per `mdsmith fix` invocation or once per rule-config cache-key build, not per workspace file, so the win is small but free. Extracted into sortFixableRulesByID, sortDiagnostics, and sortRuleFixCounts using slices.SortFunc with cmp.Compare / cmp.Or. An alloc test per helper pins 0 allocs/op. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MTRxsATsWYv5rHeBsuCZuj
abea711 to
a1a0f50
Compare
|
Review pass complete — no changes needed.
Pushed the merge commit ( Generated by Claude Code |
Security review — no findingsReviewed against the mdsmith threat model (PR/diff mode). This is a behavior-preserving performance change (
No §0 defense regressed. Signed off from a security standpoint. Generated by Claude Code |
…e-nghrks Resolve conflicts from #829 (rename→refactor redesign) and #827 (backlink algorithm extraction into internal/backlinks), preserving this PR's reflect-driven-sort fixes: - cmd/mdsmith/rename.go: keep #829's redesign; re-apply the descending-stable applyEdits sort as sortEditsByCharacterDesc (slices.SortStableFunc over refactor.Edit). The applyAndReport summaries sort this PR also swapped no longer exists after #829, so sortRenameSummaries is dropped. - cmd/mdsmith/backlinks.go: take main's version; the sort this PR optimized moved into internal/backlinks with #827. - internal/backlinks/backlinks.go: re-apply the record sort as sortBacklinkRecords (slices.SortStableFunc over Record) in its new home, with a matching 0-alloc test and the race-guard pair. - cmd/mdsmith/sortnoreflect_test.go: retarget to internal/refactor types; drop the now-obsolete rename-summaries and backlink cases (the latter moved to internal/backlinks/sortnoreflect_test.go). Validated: go build ./..., go test ./... (full suite), and mdsmith check . (588 files, 0 failures) all pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QYLYT2De8vAAEFQXRVjWw3
There was a problem hiding this comment.
🟢 Approval recommended
The changes are localized comparator/mechanism swaps with targeted allocation-gate tests, and no correctness issues were identified in review.
Pull request overview
This PR removes a handful of “reflect in hot paths” and fmt.Sprintf-driven formatting patterns by swapping sort.Slice/sort.SliceStable for slices.SortFunc/slices.SortStableFunc and fmt.Sprintf for strconv.Itoa + concatenation, and adds allocation-gate tests to lock in the wins.
Changes:
- Replace
sort.Slice/sort.SliceStablecall sites withslices.SortFunc/slices.SortStableFunc+cmp.Compare/cmp.Orin workspace indexing, CLI output ordering, fix output ordering, backlinks ordering, and goldmark’sPrioritizedSlice. - Replace
fmt.Sprintfdiagnostic message formatting inlistmarkerspacewithstrconv.Itoa+ string concatenation. - Add alloc-focused tests (skipping under
-shortand-race) plus per-packageraceEnabledbuild-tag sentinels where needed.
File summaries
| File | Description |
|---|---|
| pkg/mdsmith/workspace.go | Extracts sortDirEntries and swaps to slices.SortFunc + cmp.Compare for directory entry ordering. |
| pkg/mdsmith/sortdirentries_alloc_test.go | Adds an allocation-gate test ensuring the directory entry sort avoids reflect-driven sorting. |
| pkg/goldmark/util/util.go | Updates PrioritizedSlice.Sort to use slices.SortFunc + cmp.Compare. |
| pkg/goldmark/util/race_on_test.go | Adds raceEnabled sentinel for alloc-test skipping under -race. |
| pkg/goldmark/util/race_off_test.go | Adds non-race raceEnabled sentinel variant. |
| pkg/goldmark/util/prioritizedslice_alloc_test.go | Adds alloc-gate test for PrioritizedSlice.Sort. |
| internal/rules/listmarkerspace/rule.go | Replaces fmt.Sprintf with strconv.Itoa + concatenation for the diagnostic message. |
| internal/rules/listmarkerspace/race_on_test.go | Adds raceEnabled sentinel for alloc-test skipping under -race. |
| internal/rules/listmarkerspace/race_off_test.go | Adds non-race raceEnabled sentinel variant. |
| internal/rules/listmarkerspace/alloc_test.go | Adds alloc-gate test pinning itemVerdict message-build allocation behavior. |
| internal/fix/fix.go | Replaces several sort.Slice call sites with helper functions using slices.SortFunc. |
| internal/fix/sortnoreflect_test.go | Adds alloc-gate tests for the new sort helpers in internal/fix. |
| internal/fix/race_on_test.go | Adds raceEnabled sentinel for alloc-test skipping under -race. |
| internal/fix/race_off_test.go | Adds non-race raceEnabled sentinel variant. |
| internal/backlinks/backlinks.go | Replaces sort.SliceStable with slices.SortStableFunc via sortBacklinkRecords. |
| internal/backlinks/sortnoreflect_test.go | Adds alloc-gate test for backlink record sorting. |
| internal/backlinks/race_on_test.go | Adds raceEnabled sentinel for alloc-test skipping under -race. |
| internal/backlinks/race_off_test.go | Adds non-race raceEnabled sentinel variant. |
| cmd/mdsmith/rename.go | Replaces reflect-driven stable sort with slices.SortStableFunc for edit ordering. |
| cmd/mdsmith/deps.go | Replaces reflect-driven stable sort with slices.SortStableFunc for deps output records. |
| cmd/mdsmith/buildpass.go | Replaces reflect-driven stable sort with slices.SortStableFunc for build target output. |
| cmd/mdsmith/sortnoreflect_test.go | Adds alloc-gate tests covering the CLI sort helpers. |
Review details
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The comment described a "benchmark loop's Sort" avoiding an already-sorted no-op pass, but the helper is called once (to seed the fixture) and the alloc-count loop deliberately re-sorts the same already-sorted slice repeatedly, since the allocation cost doesn't depend on input order. Fixed per Copilot review feedback on PR #835. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MTRxsATsWYv5rHeBsuCZuj
|
🟢 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
Scheduled performance audit against
docs/development/high-performance-go.md.
Three background agents scanned
internal/rules/, the rest ofinternal/, andpkg/+cmd/for allocation-budget violations, "Patterns to avoid"anti-patterns, and struct-layout/reflect issues. The codebase turned out to
already be heavily scrubbed for these patterns (most candidates the agents
flagged were already-correct, documented optimizations, or — in one case
(
internal/config.resolveEffectiveKinds) — a map allocation that Go's escapeanalysis already proves never reaches the heap, verified with
go build -gcflags="-m=2").Two candidate fixes were caught and dropped during review. A first pass
picked pre-sizing two allocation-heavy maps/slices
(
internal/lint.CollectCodeBlockLines/CollectPIBlockLines, and thediagnostics slice in MDS006/007/008) as part of the top 5, backed by
allocation-count tests on synthetic worst-case fixtures. The xhigh-severity
/code-reviewpass (round 1) empirically wall-clock-benchmarked both againstorigin/mainon realistic input rather than trusting the alloc-only tests,and found both were real regressions on the common case — a two-pass
count-then-build costs ~80% more CPU than a single append pass when there are
few or zero violations (the typical file), for zero allocation benefit, since
growing a nil slice to a small result already costs one allocation either way;
similarly, an eager map-size estimate walk adds a full extra AST traversal for
zero benefit on the common code/PI-block-free file. Both were reverted and
replaced with two other real, safely-fixable violations, keeping the set at
five:
pkg/mdsmith/workspace.go(buildDirIndex) — sorted each directory's[]fs.DirEntrywithsort.Slice, which drivesreflect.Swapperinternally (the "reflect in hot paths" anti-pattern), already fixed the
same way elsewhere in this codebase. Runs once per directory on every
NewMemWorkspacecall — the WASM/Obsidian/embedded-host path. Extractedinto
sortDirEntriesusingslices.SortFunc.listmarkerspace, default-on) —itemVerdictbuilt itsdiagnostic message with
fmt.Sprintfand two%dverbs, driving fmt'sreflection-based formatting path, unlike every sibling structural rule
(
listindent,headingincrement,atxheadingwhitespace,noduplicateheadings), which usesstrconv.Itoa+ string concatenation.Only builds on a confirmed violation, so no double-pass tradeoff.
cmd/mdsmithCLI output sorts (rename.go,deps.go,backlinks.go,buildpass.go) — fivesort.Slice/sort.SliceStablecall sites sortingcommand-output result lists, same reflect-driven anti-pattern as Add heading-max, code-block-max, and stern mode to line-length rule #1.
Extracted each into a named
sortXxxhelper usingslices.SortFunc/slices.SortStableFunc(preserving stability where the original usedSliceStable).pkg/goldmark/util/util.go(PrioritizedSlice.Sort) — samereflect-driven anti-pattern; this one method orders every default block
parser, inline parser, paragraph/AST transformer, and node renderer
goldmark registers, so fixing it once covers five call sites across the
parse/render pipeline (pool-bounded, not per-file, but on every worker's
pool-fill path).
internal/fixsorts (fix.go) — four moresort.Slicecall sites(fixable rules by ID ×2, deduplicated diagnostics, per-file rule fix
counts), same anti-pattern, run once per
mdsmith fixinvocation or cachebuild.
Every fix in this final set is a pure comparator-mechanism swap (single pass,
no added traversal), so none carries the common-case-regression risk that
sank the two dropped candidates. Each carries a dedicated allocation test
(0 allocs/op after, 3–7 before) that was verified red against the pre-fix
code before the fix made it green.
Code review: three xhigh-severity
/code-reviewpasses, as required.Round 1 found the two regressions described above; they were reverted and
replaced, and the corrected set was re-validated (full suite, twice, plus
golangci-lintandmdsmith check). Rounds 2 and 3, run against thecorrected set, both came back clean (no findings) — including a
-racebuild/test pass and a stability/comparator-direction re-trace of every
extracted sort helper in round 3.
Test plan
go build ./...,go build -race ./...go vet ./...,gofmt -lcleango test ./...(full suite green, twice in a row, plus-shortand-raceon touched packages)go tool -modfile=tools/go.mod golangci-lint run(0 issues, repo-wide)go run ./cmd/mdsmith check .(584 files, 0 failures)origin/mainto confirm the regression before reverting (not just alloc-count tests)
/code-reviewpasses — round 1 found and fixed 2regressions (described above); rounds 2 and 3 clean
🤖 Generated with Claude Code
https://claude.ai/code/session_01MTRxsATsWYv5rHeBsuCZuj