perf: fix top 5 high-performance-go.md violations#723
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:
|
…hip checks Code review on PR #723 flagged that the map[string]bool -> map[string]struct{} conversion hand-rolled the two-value lookup instead of reusing internal/setutil.Contains, the established idiom already used by 9 other rule packages for this exact shape. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
…w internal package Code review on PR #723 flagged that pkg/goldmark/ast/structlayout_test.go and pkg/goldmark/parser/structlayout_test.go duplicated the same isPointerish helper verbatim. Extract it to pkg/goldmark/internal/fieldorder, importable by both packages, so the pointer-ish-kind rule lives in one place. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
…sized support table Code review on PR #723 flagged that flavorCount = FlavorMyST + 1 and featureCount = FeatureGitHubAlerts + 1 are a "last enum value" idiom with no compiler enforcement: adding a new Flavor or Feature constant without also bumping these two lines would silently undersize the support array, and Supports would return false for the new value on every flavor with no build failure. Add tests that scan the actual enum surface (AllFeatures, and every Flavor Flavor.String recognises) against the two constants, so that drift fails a test instead of degrading silently. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
…canConfig Round-2 code review on PR #723 flagged that buildCorpusIndex, indexFileIfEligible, and candidateParagraphs had grown to 9/12/8 positional parameters respectively as each perf fix (RunCache, rootDir, keySuffix) bolted on another one -- and that sibling cross-file rules (crossfilereferenceintegrity's checkCtx/targetFile, among others) already use a bundled-settings struct for exactly this shape. Also fixes a garbled doc comment the previous commit introduced ("rootDir =="\n// "" (FS-only..."). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
There was a problem hiding this comment.
Pull request overview
Performance-focused refactors across mdsmith’s rule engine and vendored Goldmark fork to address issues called out by docs/development/high-performance-go.md, primarily by reducing repeated work (memoization/run-cache) and shrinking GC scanning overhead (struct field order), while preserving public API behavior.
Changes:
- Replace the flavor feature support lookup with a fixed 2D array and add bounds checks + contract tests for out-of-range inputs.
- Add memoization/caching to eliminate repeated AST/corpus scans (per-file
MemoFilefor headings; per-runRunCachefor duplicate-content corpus parsing). - Reorder hot structs’ fields to place pointer/interface fields before scalars, plus add layout-pin tests.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/markdown/flavor/feature.go | Replaces nested map support table with a fixed 2D array and adds explicit bounds checks. |
| pkg/markdown/flavor/feature_test.go | Adds tests pinning support table shape, out-of-range behavior, and enum/table sizing consistency. |
| pkg/goldmark/parser/structlayout_test.go | Adds tests pinning pointer-fields-before-scalars ordering for hot parser structs. |
| pkg/goldmark/parser/link.go | Reorders linkLabelState fields to reduce GC ptrdata scanning. |
| pkg/goldmark/parser/fcode_block.go | Reorders fenceData fields and switches to keyed literal initialization. |
| pkg/goldmark/parser/delimiter.go | Reorders Delimiter fields so pointer-bearing fields precede scalars. |
| pkg/goldmark/internal/fieldorder/fieldorder.go | Introduces shared reflection helper used by struct layout tests. |
| pkg/goldmark/ast/structlayout_test.go | Adds a struct-layout test for ast.AutoLink. |
| pkg/goldmark/ast/inline.go | Reorders AutoLink fields to keep scalar tail out of GC ptrdata. |
| internal/rules/duplicatedcontent/rule.go | Memoizes corpus paragraph extraction via RunCache to avoid O(N²) re-parsing across host files. |
| internal/rules/duplicatedcontent/rule_test.go | Adds tests proving RunCache reuse across host files and preventing front-matter offset double-application. |
| internal/rules/build/rule.go | Converts membership maps from map[string]bool to map[string]struct{} and uses set helper. |
| internal/rules/build/rule_test.go | Pins reservedDeviceNames value type as zero-byte struct{}. |
| internal/rules/astutil/astutil.go | Memoizes CollectSectionHeadings via File.MemoFile to share AST walks between rules. |
| internal/rules/astutil/astutil_test.go | Adds a test ensuring section heading collection is memoized (same backing slice). |
| internal/lint/runcache.go | Adds a DuplicateParagraphs cache with invalidation support keyed by absPath + settings suffix. |
| internal/lint/runcache_test.go | Adds tests pinning DuplicateParagraphs “build once per key” and invalidation behavior. |
Copilot review on PR #723 flagged that IsPointerish only checked reflect.Kind, so a struct field whose own Kind is reflect.Struct (or reflect.Array) but which itself wraps a pointer-bearing field (e.g. a nested struct holding a []byte) would be misclassified as scalar -- letting a real GC-ptrdata regression slip past these layout tests while the package doc comment claims to check "pointer-bearing fields". None of the four structs this package currently checks happen to have such a field today, but the gap is real for future callers. IsPointerish now takes a reflect.Type and recurses into struct fields and array element types. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
|
🟢 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. |
|
This PR could not be merged into the batch branch without conflicts with Next: Rebase onto or merge |
reservedDeviceNames and warnUnknownParams' known-param set only ever
check presence; map[string]bool wastes a byte (plus padding) per
entry for a value that is never read. Per
docs/development/high-performance-go.md "map[K]struct{} for sets —
zero-byte value type."
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
Flavor and Feature are small dense int enums (9 and 13 values), so map[Flavor]map[Feature]bool paid two hash-map indirections per Supports call -- run once per candidate AST node during flavor detection. Direct array indexing removes both, matching docs/development/high-performance-go.md's small-fixed-set guidance. Supports keeps its "false for an out-of-range value" contract (this is a public package) via an explicit bounds check instead of relying on a map's missing-key zero value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
…structs Delimiter (one per emphasis-run candidate), linkLabelState (one per bracket candidate), fenceData (one per fenced-code-block open), and AutoLink (one per autolink node) all declared scalar fields before their pointer/interface fields. GC ptrdata spans through the last pointer field, so the scalar tail was being swept for no reason on every file's inline parse. Reorder per docs/development/high-performance-go.md "Group pointer fields first, scalars last", and add a reflection-based test in each package pinning the invariant. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
buildCorpusIndex re-read, re-parsed, and re-fingerprinted every sibling markdown file from scratch on every host file's Check call -- an O(N^2) cost across a workspace of N files whenever MDS037 is enabled, with no use of the RunCache the engine already provides for exactly this "N host files, 1 shared corpus" shape (see RunCache.Anchors, the crossfilereferenceintegrity precedent this mirrors). Cache each candidate's fingerprinted paragraphs keyed by its absolute on-disk path plus min-chars (the one rule setting that can vary per file kind), so a run collapses to one read+parse per sibling instead of one per (host file, sibling) pair. The key's absPath prefix lets RunCache.Invalidate(absPath) -- already wired to the LSP's per-edit document lifecycle -- evict the right slots, so the memo stays correct across incremental edits without new LSP-side plumbing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
…hoist cache-key suffix Codecov's patch-coverage gate caught the gap: no test exercised a corpus candidate with front matter (a non-zero LineOffset) read through the shared RunCache, so the offset-add inside candidateParagraphs' build closure was untested. Add a test that Checks the same host file twice against one RunCache, pinning that the offset is correct on both the cache-miss build and the cache-hit read. Also fold in two round-1 code-review findings on the same function: the "\x00"+minChars cache-key suffix was rebuilt on every corpus file even on a cache hit (it's fixed for the whole Check call, so buildCorpusIndex now builds it once and threads it down), and the `if other.LineOffset != 0` guard around the offset-add loop was protecting a no-op (adding 0 is already a no-op) rather than saving any real cost. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
…hip checks Code review on PR #723 flagged that the map[string]bool -> map[string]struct{} conversion hand-rolled the two-value lookup instead of reusing internal/setutil.Contains, the established idiom already used by 9 other rule packages for this exact shape. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
…w internal package Code review on PR #723 flagged that pkg/goldmark/ast/structlayout_test.go and pkg/goldmark/parser/structlayout_test.go duplicated the same isPointerish helper verbatim. Extract it to pkg/goldmark/internal/fieldorder, importable by both packages, so the pointer-ish-kind rule lives in one place. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
…sized support table Code review on PR #723 flagged that flavorCount = FlavorMyST + 1 and featureCount = FeatureGitHubAlerts + 1 are a "last enum value" idiom with no compiler enforcement: adding a new Flavor or Feature constant without also bumping these two lines would silently undersize the support array, and Supports would return false for the new value on every flavor with no build failure. Add tests that scan the actual enum surface (AllFeatures, and every Flavor Flavor.String recognises) against the two constants, so that drift fails a test instead of degrading silently. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
…canConfig Round-2 code review on PR #723 flagged that buildCorpusIndex, indexFileIfEligible, and candidateParagraphs had grown to 9/12/8 positional parameters respectively as each perf fix (RunCache, rootDir, keySuffix) bolted on another one -- and that sibling cross-file rules (crossfilereferenceintegrity's checkCtx/targetFile, among others) already use a bundled-settings struct for exactly this shape. Also fixes a garbled doc comment the previous commit introduced ("rootDir =="\n// "" (FS-only..."). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
…sPointersBeforeScalars Round-2 code review flagged that the earlier fieldorder extraction only moved isPointerish (the smaller helper) into the shared package, leaving assertOwnFieldsPointersBeforeScalars duplicated verbatim in both pkg/goldmark/ast/structlayout_test.go and pkg/goldmark/parser/structlayout_test.go -- and the two copies had already drifted (one carried an extra comment sentence the other lacked), which is exactly the divergence sharing code is meant to prevent. Move it into fieldorder as AssertPointersBeforeScalars(t testing.TB, ...). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
…t_CoversEveryValidFlavor Round-2 code review nitpicked the bare "+10" in the flavor-scan bound as an undocumented magic number. Name it flavorScanSlack with a comment explaining why any small headroom (not a specific count) suffices. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
Copilot review on PR #723 flagged that IsPointerish only checked reflect.Kind, so a struct field whose own Kind is reflect.Struct (or reflect.Array) but which itself wraps a pointer-bearing field (e.g. a nested struct holding a []byte) would be misclassified as scalar -- letting a real GC-ptrdata regression slip past these layout tests while the package doc comment claims to check "pointer-bearing fields". None of the four structs this package currently checks happen to have such a field today, but the gap is real for future callers. IsPointerish now takes a reflect.Type and recurses into struct fields and array element types. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
ff29aba to
3107dd6
Compare
|
🟢 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. |
… kind Copilot review on PR #723 flagged that TestSupportTable_IsArrayNotMap only checked reflect.TypeOf(support).Kind() == reflect.Array -- a regression to [flavorCount]map[Feature]bool would still satisfy that check (still an Array at the top level) while reintroducing a per-row map lookup. Assert the element type is itself an Array of bool, not another map. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
|
⏳ 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. |
…range contract Copilot review on PR #723 noted the Supports doc comment could read as contradicting itself: "an f or feat outside the declared enum range returns false" appears right after "FlavorAny accepts every feature", and FlavorAny short-circuits before the bounds check runs. FlavorAny is itself in-range, so there was no actual bug -- just ambiguous ordering. Reworded to state explicitly that FlavorAny never reaches the bounds check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ
|
🔵 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
An audit against
docs/development/high-performance-go.md(using a team of exploration agents covering allocations/regexp, data-structure/struct-layout, and memoization/concurrency anti-patterns) surfaced a ranked list of violations. Several strong candidates were already tracked by existing plans with deliberate grandfathered debt (plan/195fortableformat's double table-parse,plan/2606130838forcrossfilereferenceintegrity's 12-alloc budget,plan/189for the AST-walk migration, which explicitly excludes MDS003/MDS005/MDS059/MDS065 as stateful passes) — those were left alone to avoid duplicating or contradicting in-flight work.The five fixes below are fresh, safe, and each independently testable:
internal/rules/astutil: memoizeCollectSectionHeadingsviaf.MemoFile, mirroring the already-memoizedCollectSectionParagraphsin the same file. MDS057 and MDS058 each re-walked the AST from scratch; now the walk is shared.internal/rules/build:warnUnknownParams's per-directiveknownset and the package-levelreservedDeviceNamesset usedmap[string]boolfor membership-only checks. Converted tomap[string]struct{}per the "zero-byte value type" guidance, reading through the existinginternal/setutil.Containshelper.pkg/markdown/flavor: replaced themap[Flavor]map[Feature]boolsupport table (consulted once per candidate AST node during flavor detection) with a flat[flavorCount][featureCount]boolarray, removing two map indirections per call.Supportskeeps its "false for an out-of-range value" contract (this is a public package) via an explicit bounds check, and two new regression tests pinflavorCount/featureCountagainst the real enum surface so a future constant added without updating them fails a test instead of silently losing its row.pkg/goldmark/parserandpkg/goldmark/ast: reorderedDelimiter,linkLabelState,fenceData, andAutoLinkso pointer/interface fields precede scalar fields. GC ptrdata spans through the last pointer field, so the previous ordering forced the GC to scan scalar tails on every one of these hot per-token structs (allocated per emphasis-run, per bracket candidate, per fenced-code-block, and per autolink in every file's inline parse). The reflection-based layout tests share one helper (pkg/goldmark/internal/fieldorder) instead of duplicating it per package.internal/rules/duplicatedcontent: MDS037'sbuildCorpusIndexre-read, re-parsed, and re-fingerprinted every sibling markdown file from scratch on every host file'sCheckcall — O(N²) file parses across a workspace of N files. Now memoized on the engine'sRunCache, keyed by absolute path +min-chars, mirroring thecrossfilereferenceintegrity/RunCache.Anchorsprecedent.RunCache.Invalidate(absPath)(already wired to the LSP's per-edit lifecycle) evicts the right slots with no new LSP plumbing. The settings threaded through the corpus-scan helpers are bundled into onecorpusScanConfigstruct, matching the pattern sibling cross-file rules already use for grouped scan state.Each fix follows red/green TDD: a failing test first (an allocation/behavior/struct-layout assertion), then the minimal change to pass it. Full details and reasoning are in each commit message.
This PR went through three rounds of
xhigh-severity code review (24 finder agents total across the three rounds). Round 1 surfaced cleanup-tier findings — a Codecov coverage gap in the RunCache front-matter-offset path, an unhoisted cache-key allocation, a duplicated test helper across two goldmark packages, and an unenforced enum-count invariant in the flavor support table — all fixed in follow-up commits. Round 2 caught that the test-helper dedup was incomplete and that the corpus-scan helpers' parameter lists had grown unwieldy across the fixes; both addressed. Round 3 found no further issues.Test plan
go build ./...go test ./...(full suite green)go vet ./...go tool -modfile=tools/go.mod golangci-lint run(0 issues)go run ./cmd/mdsmith check .(531 files checked, 0 failures)git stashthat the new test fails against the pre-fix code)xhighcode review (24 finder agents), all findings resolved🤖 Generated with Claude Code
https://claude.ai/code/session_01LJTrn4DizYTMdYj4tbswVJ