perf: fix top 5 high-performance-go.md violations from a 3-agent audit - #783
perf: fix top 5 high-performance-go.md violations from a 3-agent audit#783jeduden wants to merge 10 commits into
Conversation
fixGitHubAlerts rebuilt every line of a fixed file through a string(line) conversion into an unsized, growing []string, even for lines that pass through unchanged. Pre-size the accumulator from len(f.Lines) and only copy the lines that actually need the "> " prefix rewrite (docs/development/high-performance-go.md "Pre-size slices" / "Stay in []byte"). 68 allocs -> 12 on a 50-alert fixture. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ebZ9KF9xR46jGhS14hUiQ
snapshotOutputs read every declared build output fully into memory via os.ReadFile to diff two verify-pass runs byte-for-byte. Declared outputs are user-defined artifacts of unbounded size (binaries, bundles, generated sites), so this held two full copies of a large output per verify pass — the "os.ReadFile on huge inputs" anti-pattern (docs/development/high-performance-go.md). Stream each output through sha256 instead and compare hashes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ebZ9KF9xR46jGhS14hUiQ
collectParagraphs only needs each paragraph's word count, never its text (the paragraph struct has no text field), but called ExtractPlainText + CountWords, paying a pooled-buffer round trip and a string copy per paragraph for a value it immediately discarded. Use mdtext.CountWordsInNode, which counts directly off the AST and is already used by paragraph-readability for the identical reason (docs/development/high-performance-go.md "skip work you don't need"). 36 allocs -> 6 on a 30-paragraph fixture. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ebZ9KF9xR46jGhS14hUiQ
…p sets
builtinLayouts, knownFMKeys, and bodyTemplateReserved are read only via
truth-tests, never a stored false value, so map[string]bool wasted a
byte per entry over docs/development/high-performance-go.md's
"map[K]struct{} for sets — zero-byte value type". Convert all three to
map[string]struct{}, updating call sites to the comma-ok form.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016ebZ9KF9xR46jGhS14hUiQ
The high-performance-go.md audit's top finding — headingincrement and noduplicateheadings each running their own full ast.Walk instead of joining the engine's shared kind-scoped dispatch — needs a per-file state-reset mechanism the NodeChecker contract does not currently provide (a rule instance is reused sequentially across files by its worker, and CheckNode has no "start of file" hook). Converting either rule without that mechanism risks a state-leak correctness bug, so this plan tracks the design work instead of a same-PR fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ebZ9KF9xR46jGhS14hUiQ
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:
|
Codecov flagged builddiag.go at 88.23% patch coverage: os.Open's error branch (missing file) was covered, but io.Copy's (an unreadable open file) was not. A directory path opens successfully and fails on the subsequent read, exercising the missing branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ebZ9KF9xR46jGhS14hUiQ
An xhigh-effort adversarial review of the 5 perf fixes found 7
confirmed issues, all addressed here:
- markdownflavor's new alloc gate lacked the raceEnabled/short-mode
skips every other alloc gate in this repo uses, so it failed under
-race. Added the package's race_on/off_test.go sentinel pair.
- The same gate's fixture never exercised fixGitHubAlerts' addPrefix
(lazy-continuation) rewrite branch — the actual code this PR
changed most. Rebuilt the fixture with an indented lazy continuation
and added a dedicated correctness test for that shape; re-measured
the alloc baseline honestly against the corrected fixture (176 -> 120).
- Removed markdownflavor's newlineSep package var: escape analysis
shows the inline literal doesn't heap-allocate either way, so the
var's stated rationale was false and it was an unneeded abstraction.
- maxsectionlength's collectParagraphs duplicated the exact AST walk
astutil.CollectSectionParagraphs already performs and memoizes per
File for MDS023/MDS024 — reuse it instead of re-walking, which also
lets the result slice be exactly presized. 36 -> 1 alloc on the
package's alloc gate.
- builddiag's outputHash comparison is stricter than the old
bytes.Equal(nil, []byte{}) semantics for a missing-vs-empty output;
fixed the doc comment to state this as deliberate (a recipe run that
goes from absent to present-but-empty output is real
non-determinism) and added a test pinning it.
- TestSnapshotOutputs_ExistingFile asserted against hashOutputFile
itself instead of an independent content oracle; switched to hashOf.
- Documented (via a new nil-return test) that fixGitHubAlerts now
returns nil rather than a non-nil empty slice for a whole-file-alert
document, matching the "return nil, not []T{}" convention this PR
otherwise applies; traced every consumer and confirmed none
distinguishes the two.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016ebZ9KF9xR46jGhS14hUiQ
- buildAlertSkipMaps still converted each continuation line to a string via strings.TrimLeft(string(...)) one function above the code this PR set out to fix. Switched to bytes.TrimLeft/HasPrefix. fixGitHubAlerts drops from 120 to 70 allocs on the alloc gate's fixture as a result; comment and budget updated to match. - The new indented-lazy-continuation test's comment claimed the continuation line's indent must always be preserved, but that's only true within CommonMark's 0-3-space lazy-continuation range — at 4+ spaces the line is indented code, a pre-existing, separate limitation. Narrowed the comment to the range the test covers. - Fixed a plan file wording inconsistency: "one per category" don't match the five categories listed in the same sentence (three subagents covered five categories, not one each). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ebZ9KF9xR46jGhS14hUiQ
- The round-2 comment claiming a 4+-space lazy continuation is CommonMark indented code was itself wrong: an indented chunk cannot interrupt a paragraph, so it is still lazy continuation and addPrefix still rewrites it — the rewrite result is what parses as indented code, silently turning an alert body into a code block. That bug predates this PR and is out of scope to fix here; corrected the comment to describe it accurately instead of misdescribing it a second time. - slidevstructure's builtinLayouts lookup ran unconditionally ahead of the !hasLayout short-circuit it used to sit behind. Restored the short-circuit so the map lookup only runs when hasLayout is true. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016ebZ9KF9xR46jGhS14hUiQ
There was a problem hiding this comment.
Pull request overview
This PR applies several verified performance fixes across mdsmith’s lint rules and CLI build verification path, aligned with the project’s docs/development/high-performance-go.md guidance (alloc avoidance, skipping unnecessary work, bounded memory, and set representation). It also records a follow-up plan for the higher-risk “KindScopedChecker reset” design needed to safely eliminate redundant AST walks in stateful heading rules.
Changes:
- Reduce allocations in
markdownflavorGitHub-alert fixing by staying in[]byteand pre-sizing accumulators. - Make
--build-verifyoutput comparison bounded-memory by hashing outputs instead of reading full files into RAM, with regression tests (including missing-vs-empty behavior). - Reuse memoized AST paragraph collection in
maxsectionlengthand convert pure-membership “set maps” tomap[string]struct{}with guard tests.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| plan/2608020650_kindscoped-heading-rules.md | Adds a plan to design a safe per-file reset mechanism before converting stateful heading rules to KindScopedChecker. |
| PLAN.md | Registers the new plan in the repository plan index. |
| internal/rules/slidevstructure/setsize_test.go | Adds tests pinning set maps to map[string]struct{} value type. |
| internal/rules/slidevstructure/rule.go | Converts builtinLayouts/knownFMKeys from map[string]bool to map[string]struct{} and updates membership checks. |
| internal/rules/recipesafety/setsize_test.go | Adds a test pinning bodyTemplateReserved to map[string]struct{}. |
| internal/rules/recipesafety/rule.go | Converts bodyTemplateReserved to map[string]struct{} and updates membership checks. |
| internal/rules/maxsectionlength/rule.go | Reuses astutil.CollectSectionParagraphs memo and counts words directly from AST nodes. |
| internal/rules/maxsectionlength/paragraphs_alloc_test.go | Adds an allocation regression test to prevent reintroducing text extraction / redundant walks. |
| internal/rules/markdownflavor/rule.go | Reworks GitHub-alert fix path to avoid per-line string() conversions and to pre-size output. |
| internal/rules/markdownflavor/rule_test.go | Adds a direct-return test for the nil-vs-empty behavior of fixGitHubAlerts. |
| internal/rules/markdownflavor/race_on_test.go | Introduces raceEnabled build-tag constant for race gating. |
| internal/rules/markdownflavor/race_off_test.go | Introduces raceEnabled build-tag constant for non-race builds. |
| internal/rules/markdownflavor/fix_test.go | Adds coverage for indented lazy-continuation rewriting behavior. |
| internal/rules/markdownflavor/alloc_test.go | Adds/updates allocation budget test for fixGitHubAlerts. |
| cmd/mdsmith/buildpass_diag_test.go | Updates tests to validate output hashing and the missing-vs-empty distinction. |
| cmd/mdsmith/builddiag.go | Switches build-verify output snapshotting from os.ReadFile to streamed sha256 hashing with stricter equality semantics. |
| cmd/mdsmith/builddiag_memcap_test.go | Adds a bounded-memory regression test for hashing large build outputs. |
- hashOutputFile: fill outputHash's own [32]byte array via
h.Sum(oh.hash[:0]) instead of h.Sum(nil) + copy, dropping a
32-byte-slice allocation per hashed output.
- Gate the 8 MiB TestSnapshotOutputs_LargeFile_BoundedMemory under
testing.Short(), matching this package's other heavier tests.
- buildAlertSkipMaps: replace bytes.HasPrefix(raw, []byte(">")) with
a direct raw[0] != '>' check — only the first byte matters here.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016ebZ9KF9xR46jGhS14hUiQ
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cmd/mdsmith/builddiag_memcap_test.go:51
- This memory-cap test uses
runtime.MemStats.TotalAlloc(a global counter) while this package also has severalt.Parallel()tests (e.g. lsp_unit_test.go), so unrelated concurrent allocations can be counted ingrewand make this assertion flaky. Consider loosening the threshold so it still catches the oldos.ReadFilebehavior (≈8 MiB alloc for an 8 MiB file) while tolerating background/parallel-test noise.
// The whole point of the fix: total heap growth must stay well under
// the output's size, proving the content was streamed through a
// hash rather than read whole into a []byte.
grew := after.TotalAlloc - before.TotalAlloc
if grew > size/4 {
t.Fatalf("snapshotOutputs on an %d-byte output allocated %d bytes; "+
"want well under the file size (no full-file buffering)", size, grew)
}
Summary
Three parallel subagents audited origin/main against
docs/development/high-performance-go.md,each covering a distinct slice of the guide (allocations/strings-bytes,
struct-layout/skip-work, concurrency/anti-pattern-table). This is the fifth
round of this recurring audit (after #762, #764, #765, #767), so most
low-hanging fruit was already gone; the findings below are what survived
verification against the current code.
Ranked by severity/impact, the top 5 real, safely-fixable issues found:
markdownflavor(MDS034)fixGitHubAlerts/buildAlertSkipMaps—both converted every scanned line to a
string(one via an unsized,growing
[]stringaccumulator, the other via a per-continuation-linestrings.TrimLeft(string(...))). Both now stay in[]byte, andfixGitHubAlertspre-sizes its accumulator. 176 → 70 allocs on anaddPrefix-heavy 50-alert fixture (measured against an
origin/mainworktree of the same fixture, not the original, less representative
fixture the first commit shipped with — see the review-round commits for
that correction).
cmd/mdsmith--build-verify'ssnapshotOutputs— read everydeclared build output fully into memory via
os.ReadFileto diff twoverify-pass runs byte-for-byte. Declared outputs are user-defined
artifacts of unbounded size; this held two full copies of a large output
per verify pass. Streams each output through sha256 instead. Behavior
change, intentional: the old
bytes.Equal(nil, []byte{})comparisontreated a missing output and a present-but-empty one as the same "no
content" case; the new hash comparison treats them as different, which
is real non-determinism
--build-verifyexists to catch. Documented onoutputHashand pinned byTestOutputsEqual_MissingVsEmpty_ReturnsFalse.maxsectionlength(MDS036)collectParagraphs— ran its ownast.Walk(duplicating the walkastutil.CollectSectionParagraphsalready performs and memoizes per File for MDS023/MDS024) and called
ExtractPlainText+CountWordsper paragraph for a value that neverneeded the materialized text. Now delegates to the shared memoized walk
and uses
mdtext.CountWordsInNode. 36 → 1 alloc on a 30-paragraphfixture (the single remaining alloc is the presized result slice; the
walk itself is a cache hit after the first call within a File).
slidevstructure/recipesafetypure-membership sets —builtinLayouts,knownFMKeys, andbodyTemplateReservedweremap[string]boolread only via truth-tests. Converted tomap[string]struct{}per the guide's "map[K]struct{} for sets" pattern.A sixth finding — MDS003/MDS005 each running a redundant full
ast.Walkinstead of joining the engine's shared
KindScopedCheckerdispatch — wasthe highest-severity hit (both rules are default-enabled and run on every
file with headings), but converting them safely needs a per-file
state-reset hook the
NodeCheckercontract doesn't currently provide (arule instance is reused sequentially across files by its worker, and
CheckNodehas no "start of file" signal). Shipping that conversionwithout solving the reset problem risks a state-leak correctness bug
across files, so it's tracked in
plan/2608020650_kindscoped-heading-rules.mdinstead of included here. Four more rules share the same double-walk shape
and blocker (
blockquotewhitespace,codeblockstyle,samefileanchor,requiredstructure) and are noted in that plan as follow-ups.Each fix follows red/green TDD: a failing allocation/memory-bound test
first, then the minimal fix to make it pass. Three independent xhigh-effort
adversarial review rounds ran against this diff after the initial 5 fixes;
each found real issues (a missing
-racetest skip, an under-exercisedtest fixture, a misleading/unnecessary package var, a duplicated AST walk,
a tautological test, an undocumented nil-vs-empty difference, a leftover
string()conversion, two inaccurate code comments, and a staleshort-circuit-order regression) — all fixed, commit-by-commit, in the
review-round commits below.
Test plan
go build ./...go test ./...(all packages green)go test -raceon every touched packagego vet ./...go tool -modfile=tools/go.mod golangci-lint run(0 issues)mdsmith check .(568 files, 0 failures)markdownflavoralloc + correctnesstests (including a lazy-continuation indent edge case) plus a
race_on_test.go/race_off_test.gopair,builddiagbounded-memorytest +
outputHash-typed unit tests including the missing-vs-emptydistinction,
maxsectionlengthalloc test,slidevstructure/recipesafetyzero-byte-set reflection tests