perf: fix top 5 high-performance-go violations (struct layout + allocations) - #833
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:
|
MDS070's anchorChecker declared built (a bool) before diags (a []lint.Diagnostic), forcing the GC ptrdata to scan 8 bytes it did not need to. Moving built after diags — all pointer-containing fields first, scalars last — shrinks ptrdata to just what the pointer fields need. Per docs/development/high-performance-go.md "Struct layout". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RcWK6hTskkM6F6qVTpnWJb
MDS069's pathEntry declared line (an int) between the value and firstPath strings, forcing GC ptrdata to scan past it. Moving line after firstPath — pointer fields first, scalars last — shrinks ptrdata by one word. pathEntry is stored once per violation in the run-wide scopeIndex.byPath, so the saving scales with corpus size. Per docs/development/high-performance-go.md "Struct layout". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RcWK6hTskkM6F6qVTpnWJb
MDS062's revMatch declared col0/matchEnd (two ints) before the text/url byte slices, so GC ptrdata had to span both scalars plus both slices. Grouping the pointer-containing slices first and the scalars last shrinks ptrdata from 48 to 32 bytes — the largest of this pass's three struct-layout fixes. Per docs/development/high-performance-go.md "Struct layout". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RcWK6hTskkM6F6qVTpnWJb
MDS041's extractTag built the tag-name string via strings.ToLower(string(m[1])): the string(m[1]) copy, then a second buffer from strings.ToLower whenever the tag had an uppercase byte. tagNameRe only ever matches ASCII, so asciiLowerTag lowercases the matched bytes directly into a single strings.Builder allocation, cutting a mixed-case tag from 3 allocs/call to 2. Per docs/development/high-performance-go.md "Allocations". Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RcWK6hTskkM6F6qVTpnWJb
MDS025's checkColumnCount always called make([]lint.Diagnostic, 0,
len(t.rows)-1) up front, even though most tables are column-count
compliant and the function then discards that slice and returns nil.
Starting diags as nil and growing it lazily via append matches the
project's own "return nil, not []T{}" convention and drops one
allocation per compliant table — the common case on every workspace
file with well-formed tables. Per
docs/development/high-performance-go.md "Allocations".
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RcWK6hTskkM6F6qVTpnWJb
- Switch the three new layout tests (linkvalidity, samefileanchor, uniquefrontmatter) from hardcoded unsafe.Offsetof byte constants to the project's existing internal/structlayout.AssertPointerFieldsFirst helper, matching the pattern already used by tablereadability, propernames, and astutil. This asserts the same invariant (pointer fields precede scalars) without hardcoding pointer/slice/string header sizes that only hold on 64-bit platforms. - Drop the //nolint:prealloc pragma added to checkColumnCount: the prealloc linter does not flag this conditional-append pattern (the unmodified, identically-shaped checkPipeStyle has no such pragma either), so the directive suppressed nothing and only misled a reader into thinking it did. The explanatory comment stays as a plain comment. Round 2 of 3 code-review passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RcWK6hTskkM6F6qVTpnWJb
c306486 to
e66c831
Compare
|
Scheduled code-review pass complete — no code changes needed.
This PR already went through three xhigh-severity review rounds before this pass (see the PR description); this check-in found nothing new. Generated by Claude Code |
Security review — no findingsReviewed against the mdsmith threat model (PR/diff mode). This is a behavior-preserving performance change with no trust-boundary impact.
No recipe/exec, path-resolution, include/catalog, or Git-integration code touched (§0/§1/§7 unaffected). No §0 defense regressed. Signed off from a security standpoint. Generated by Claude Code |
|
🟢 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. |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are low-risk performance improvements with targeted tests added to prevent regressions.
Pull request overview
This PR applies a focused performance-audit pass (per docs/development/high-performance-go.md) to reduce GC ptrdata scanning and avoid unnecessary allocations in several default-on rules, adding targeted tests to keep those optimizations from regressing.
Changes:
- Reordered struct fields in a few hot structs to place pointer-containing fields first (shrinking GC ptrdata) and added
structlayout.AssertPointerFieldsFirsttests to pin the layout. - Removed an eager slice allocation on the common “no diagnostics” path in
MDS025column-count checking, and added an alloc test to ensure the compliant path stays allocation-free. - Replaced
strings.ToLower(string(submatch))inMDS041tag extraction with an ASCII-specific lowercasing helper and added an allocation budget test.
File summaries
| File | Description |
|---|---|
| internal/rules/uniquefrontmatter/rule.go | Reorders pathEntry fields to reduce GC ptrdata. |
| internal/rules/uniquefrontmatter/layout_test.go | Adds layout test enforcing pointer-fields-first for pathEntry. |
| internal/rules/tableformat/structure.go | Avoids allocating diagnostics slice on compliant tables in checkColumnCount. |
| internal/rules/tableformat/checkcolumncount_alloc_test.go | Adds alloc test ensuring compliant checkColumnCount stays at 0 allocs/op and returns nil. |
| internal/rules/samefileanchor/rule.go | Reorders anchorChecker fields to reduce GC ptrdata. |
| internal/rules/samefileanchor/layout_test.go | Adds layout test enforcing pointer-fields-first for anchorChecker. |
| internal/rules/noinlinehtml/rule.go | Introduces ASCII-only lowercasing helper to reduce allocations in extractTag. |
| internal/rules/noinlinehtml/race_on_test.go | Adds raceEnabled constant for alloc-test gating under -race. |
| internal/rules/noinlinehtml/race_off_test.go | Adds raceEnabled constant for non-race builds. |
| internal/rules/noinlinehtml/alloc_test.go | Adds allocation budget test for extractTag on uppercase tags. |
| internal/rules/linkvalidity/rule.go | Reorders revMatch fields to reduce GC ptrdata. |
| internal/rules/linkvalidity/layout_test.go | Adds layout test enforcing pointer-fields-first for revMatch. |
Review details
- Files reviewed: 12/12 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.
Copilot review on PR #833: starting diags as a bare nil slice kept the zero-alloc compliant-table path, but a table with several column-count mismatches paid growslice's doubling reallocations on top of that (10 allocs/op for 6 mismatches). Presizing to len(t.rows)-1 lazily, on the first mismatch, keeps the compliant path at 0 allocs while cutting the many-mismatch path to 7 — one alloc for the slice plus one per diagnostic message. Mirrors reversedInLine's lazy presize in internal/rules/linkvalidity. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RcWK6hTskkM6F6qVTpnWJb
|
⏳ Merge Queue — requeued The merge queue hit a transient error while processing this PR:
Next: No action needed — the queue will retry automatically on the next run. |
There was a problem hiding this comment.
🟢 Approval recommended
The changes are narrowly scoped, performance-motivated, and each behavior change is protected by targeted layout/allocation tests with no issues found in review.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
🔵 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/andpkg/for allocation-budgetviolations, "Patterns to avoid" anti-patterns, and struct-layout/data-structure
issues. The codebase turned out to already be heavily scrubbed for these
patterns (most candidates the agents flagged were already-correct, documented
optimizations); the five real, safely-fixable violations below were selected
and fixed with red/green TDD, each in its own commit:
internal/rules/linkvalidity(MDS062, default-on) —revMatchdeclared its two ints (
col0,matchEnd) before its two byte slices(
text,url). Go's GC ptrdata spans from offset 0 through the lastpointer-containing field, so with scalars first, ptrdata still had to
cover both slices anyway. Grouping the slices first and the scalars last
shrinks ptrdata from 48 to 32 bytes — the largest win in this pass.
internal/rules/uniquefrontmatter(MDS069, default-on) —pathEntrysandwiched an
intbetween two strings.pathEntryis stored once perviolation in the run-wide
scopeIndex.byPathindex, so the 8-byte ptrdatasaving scales with corpus size.
internal/rules/samefileanchor(MDS070, default-on) —anchorCheckerdeclared a
boolbefore its trailing[]lint.Diagnostic. Moved after,saving 8 bytes of ptrdata per file that contains a
#.internal/rules/noinlinehtml(MDS041) —extractTagbuilt the tagname via
strings.ToLower(string(m[1])): one allocation for thestring(m[1])copy, plus a second whenever the tag had an uppercase byte(since
strings.ToLowercan't return the input unchanged).tagNameReonly ever matches ASCII, so the new
asciiLowerTaglowercases the matchedbytes directly into one
strings.Builderallocation — a mixed-case tagdrops from 3 allocs/call to 2.
internal/rules/tableformat(MDS025, default-on, per-file) —checkColumnCountunconditionally calledmake([]lint.Diagnostic, 0, len(t.rows)-1), then discarded it andreturned
nilon the common column-count-compliant path. Now startsdiagsasniland grows it lazily viaappend, matching the project'sown "return nil, not
[]T{}" convention — one fewer allocation percompliant table, on every workspace file.
Each fix carries a dedicated test (a
structlayout.AssertPointerFieldsFirstlayout assertion matching the pattern in
tablereadability,propernames,astutil, or atesting.AllocsPerRunallocation budget) that was verifiedred against the pre-fix code (via
git stash, or a direct diff against thepre-PR commit) before the fix made it green.
MDS025's full fix (merging the structure pass with
tablefmt's ownalignment scan) is a larger, already-documented "single-table-walk refactor"
(see the comment in
internal/rules/tableformat/alloc_test.go) that this PRdoes not attempt — it's out of scope for a set of small, safely verifiable
fixes.
Code review: three xhigh-severity
/code-reviewpasses, as required.Round 1: no findings. Round 2: three findings — two were valid (the three
new layout tests used hardcoded, pointer-size-dependent
unsafe.Offsetofconstants instead of the repo's existing portable
structlayout.AssertPointerFieldsFirsthelper; a//nolint:preallocpragmaon
checkColumnCountsuppressed nothing, confirmed empirically) and werefixed in a follow-up commit; the third (a claimed ptrdata-savings miscalculation
in a comment) was independently verified against Go's actual
PtrDataSizealgorithm and found to be incorrect — the original 48→32 byte figure was
correct, so no change was made there. Round 3 (after the fix commit):
no findings.
Test plan
go build ./...go vet ./...go test ./...(full suite green)go tool -modfile=tools/go.mod golangci-lint run(0 issues, repo-wide)go test ./internal/integration/... -run TestPerRuleAllocBudget(per-rule alloc gate green)mdsmith check .(584 files, 0 failures)git stash/pre-fix diff) before green/code-reviewpasses, findings addressed🤖 Generated with Claude Code
https://claude.ai/code/session_01RcWK6hTskkM6F6qVTpnWJb