perf: fix top 5 high-performance-go.md violations found by codebase audit - #762
Merged
Conversation
File.Memo/MemoFile and RunCache's shared load() helper both paid two
allocations on every cache-hit call: LoadOrStore's second argument
(a throwaway &memoEntry{}/&runCacheEntry{}) is constructed before the
call and discarded whenever the key already exists, and
sync.Once.Do(func(){ e.val = build() }) allocates the wrapping
closure as an argument even when Do's internal check makes it a
no-op. These accessors run at least once per host file across a
workspace check, so the per-call cost multiplied by the corpus size.
Switch both to a Load-before-LoadOrStore check plus the
atomic.Bool+mutex double-checked-lock pattern (no wrapping closure),
bringing the warm path to zero allocations.
Ref: docs/development/high-performance-go.md ("Reuse loop-local
buffers", "sync.Pool for transient state").
Check/Fix unconditionally allocated a fresh visited map and chain slice, and took the rule-wide mutex serialising this default-enabled rule's Check calls across the engine's per-file parallel fan-out, before ever checking whether the file could contain an <?include?> directive. MDS021 runs on every host file in a workspace, so this cost (and the lost parallelism) applied even to files that never use the directive. Add a cheap bytes.Contains(f.Source, "<?include") pre-check so a file with no include marker returns immediately without allocating or locking, matching the "gate expensive analyzers behind a cheap pre-check" pattern already used elsewhere (e.g. nobareurls' mayContainURL). Ref: docs/development/high-performance-go.md.
…er file
Check called validateGlobSettings() on every file — re-running
doublestar validation over the rule's Include/Exclude glob lists
even though ApplySettings already validated the identical, static
list once when config was applied. MDS027 is default-enabled, so
every host file in a workspace paid this redundant revalidation.
Cache the verdict behind cachedGlobSettingsErr, using the same
atomic.Bool+mutex pattern as the RunCache/Memo fix so the warm path
costs one atomic load instead of a closure allocation. ApplySettings
resets the cache before recomputing so a rule instance that gets
reconfigured (or a test that bypasses ApplySettings entirely, setting
Include/Exclude directly) still sees a correct, freshly validated
result on the next Check.
The struct grew by 24 bytes (error + mutex + atomic.Bool); the
layout test's expected size is updated accordingly with the packing
math for why 144 (not more) is optimal.
Ref: docs/development/high-performance-go.md ("Memoize per-input
computations").
checkFootnotes ran two full-file regex passes (footnoteRefRE and
footnoteDefRE, each a FindAllSubmatchIndex over the whole source) on
every Check call, even for files with no footnote-style syntax at
all. Benchmarked at ~585µs/op on a 28KB prose file with no footnotes,
versus ~0.8µs/op once gated — both regexes require the literal bytes
"[^" to match anything.
Add mayContainFootnote, mirroring nobareurls' mayContainURL, and
return early from checkFootnotes when the source can't contain either
pattern. A benchmark with an inline budget (b.Fatalf at 50µs/op, two
orders of magnitude above the gated cost) pins the regression so CI
catches a dropped gate.
Ref: docs/development/high-performance-go.md ("Gate expensive
analyzers behind a cheap pre-check").
Scan called scanOne once per declared foreign-region marker pair, and
each call re-walked the entire f.Lines slice, converting every line
via strings.TrimSpace(string(line)) — an O(regions x lines) cost with
one string allocation per line per region, even though every region's
markers can be checked against the same per-line trim in one pass.
Restructure Scan to walk f.Lines once, computing bytes.TrimSpace(line)
a single time per line and evaluating it against every region's
regionScanState. This also drops the per-line string() conversion in
favor of staying in []byte with bytes.Equal comparisons. A test pins
that 5 regions costs close to what 1 region costs (a small per-region
constant), not 5x.
Ref: docs/development/high-performance-go.md ("Stay in []byte",
"Skip work you don't need").
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:
|
TestCheck_NoFS and TestCheck_NoIncludeDirectiveAllocatesNothing pinned these branches for Check, but Fix shares the same compound early-return condition and had no equivalent coverage — Codecov flagged the gate added in c2db2fa as only 50% patch-covered. Add the mirroring Fix tests.
The needle gate added in c2db2fa only checked for "<?include", which does not match a dangling "<?/include?>" end marker with no opening start marker ("/" sits where the needle expects "i"). Such a file would false-negative past the gate, silently dropping the engine's "unexpected generated section end marker" diagnostic that Check previously emitted — a regression a user would hit by deleting an opening <?include ... ?> block but leaving the closing marker behind. Check for both "<?include" and "<?/include" via mayContainIncludeDirective. Caught by round 1 of adversarial code review.
TestCheck_NoIncludeDirectiveAllocatesNothing asserts a zero-alloc delta, but the race detector's instrumentation adds its own allocations, so the test fails under `go test -race` even though the gate itself is correct — CI does not run -race, so this only bites a developer running it locally. Add the raceEnabled build-tag const pattern already used by internal/rules/nobareurls and internal/rules/crossfilereferenceintegrity, and skip the gate under it. Noted in round 2 of code review.
jeduden
marked this pull request as ready for review
July 21, 2026 21:15
jeduden
added
queue
Owner
Author
|
🟢 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. |
3 tasks
Owner
Author
|
🔵 Merge Queue — CI running Merged into batch branch Next: No action needed — you'll be notified when CI completes. |
jeduden
removed
the
queue:active
Owner
Author
|
✅ Merge Queue — merged This PR landed on Next: Done — nothing more to do here. |
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
A codebase-wide audit against
docs/development/high-performance-go.mdwas run using a fleet of agents scanninginternal/rules/**, the core parse/lint pipeline (internal/lint,internal/mdtext,internal/engine,pkg/markdown,pkg/goldmark), and the schema/config/discovery surfaces. Most of the codebase turned out to already be heavily tuned against this exact doc (many files carry inline comments citing prior alloc-budget work), which narrowed the field considerably. Every candidate below was independently verified by reading the actual code and, where the claim was about CPU/allocation cost, by writing a real benchmark before touching anything — one high-severity candidate (a claimeddoublestar.Matchrevalidation cost ininternal/discovery) did not survive that check and was dropped.The five fixes below are each backed by a red/green test that fails against the old code and passes against the new code, per this repo's TDD workflow. The PR then went through three rounds of adversarial xhigh-effort code review: round 1 found and fixed a real regression (see below), round 2 found and fixed a minor test-robustness issue, round 3 came back clean and recommended merge.
Fixes
internal/lint: closure-box allocation onRunCache/File.Memowarm path —File.Memo,File.MemoFile, andRunCache's sharedload()helper each paid 2 allocations per cache-hit call:sync.Map.LoadOrStore's second argument (a throwaway&memoEntry{}/&runCacheEntry{}) is constructed and discarded on every hit, andsync.Once.Do(func(){ e.val = build() })allocates its argument closure regardless of whetherDoends up invoking it. These are called at least once per host file across a workspace run. Fixed with aLoad-before-LoadOrStorecheck plus theatomic.Bool+mutex double-checked-lock pattern (no wrapping closure) — verified at 0 allocs/op on the warm path (was 2).MDS021 (
include): unconditional per-file alloc + mutex lock —Check/Fixallocated avisitedmap andchainslice and took a rule-widesync.Mutex(serializing this default-enabled rule'sCheckacross the engine's parallel per-file fan-out) before ever checking whether the file has an<?include?>directive. Gated behind a cheap byte-needle pre-check, matching themayContainURLpattern already used bynobareurls. (Round 1 of review caught that the first version of this gate only checked for"<?include", silently dropping the "unexpected generated section end marker" diagnostic for a file with a dangling<?/include?>and no opening marker — fixed by checking both the start and end needles.)MDS027 (
cross-file-reference-integrity): redundant per-file glob revalidation —CheckcalledvalidateGlobSettings()on every file, re-validating the same staticInclude/Excludeglob lists thatApplySettingsalready validated once at config-load time. Cached behindcachedGlobSettingsErrusing the same atomic.Bool+mutex pattern as fix Add heading-max, code-block-max, and stern mode to line-length rule #1;ApplySettingsresets the cache so reconfiguration (or a test that sets fields directly, bypassingApplySettings) still gets a fresh, correct verdict.MDS043 (
no-reference-style): ungated footnote regex scan —checkFootnotesran two full-file regex passes unconditionally on everyCheck, even for files with no footnote syntax at all. Benchmarked at ~585µs/op on 28KB of plain prose, ~0.5µs/op once gated behind amayContainFootnotebyte-needle check (both regexes require"[^"to match anything).internal/foreignregion: O(regions × lines) scan —Scanwalked the whole file once per configured region, re-converting every line viastrings.TrimSpace(string(line))each time. Restructured into a single pass overf.Linesthat evaluates every region's marker state against one sharedbytes.TrimSpaceper line, and switched to[]byte/bytes.Equalinstead of allocating astringper line per region.Motivation
mdsmith checkruns its rule set over every file in a workspace; the project's own performance doc states the budget philosophy plainly: "one extra alloc perCheckmeans tens of thousands per run." Fixes #1–4 hit default-enabled or otherwise commonly-invoked code paths, so their savings apply broadly across a workspace check rather than to one rule in isolation.Test plan
testing.AllocsPerRun, or a benchmark with an inline budget for the one CPU-bound fix)go build ./...go vet ./...gofmt -l .— cleango test ./...(full suite, including-raceon touched packages)go tool -modfile=tools/go.mod golangci-lint run ./...— 0 issuesgo run ./cmd/mdsmith check .— 563 files checked, 0 failures