perf: fix top 5 high-performance-go.md violations found by codebase audit - #764
Conversation
ColumnOfOffset hand-rolled a backward byte scan to find the start of
the current line. LineOfOffset already binary-searches a memoized
newline index for the same purpose; extract that search into
newlineSearch and reuse it from both, so a diagnostic on a very long
line (a minified table, a long URL list) no longer pays for a full
backward scan. Benchmarked on an 8 KB line: ~5.3µs/op -> ~9ns/op.
Ref: docs/development/high-performance-go.md ("Memoize per-input
computations").
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UCh2j2oeSw6UgNsmpjRonL
matchVariant converted every paragraph line to a string before
regexp matching, in both the Check and Fix paths. regexp.Match
accepts []byte directly and bytes.TrimRight covers the same
trimming strings.TrimRight did, so the conversion was pure overhead
on every scanned line. Drops the alloc-budget fixture from 6 to 0
allocs/op; alloc_test.go's budget is tightened to match.
Ref: docs/development/high-performance-go.md ("Stay in []byte").
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UCh2j2oeSw6UgNsmpjRonL
rawLines and rows grew via plain append with no capacity hint, so a
multi-row table paid several slice-growth reallocs during parsing.
countDataRows mirrors the data-row loop's stopping condition without
allocating, so both slices can be pre-sized once up front. Measured
174 -> 162 allocs/op parsing a 52-row table (the residual allocs come
from splitRowBytes's per-row cells slice, unrelated to this fix).
Ref: docs/development/high-performance-go.md ("Pre-size slices").
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UCh2j2oeSw6UgNsmpjRonL
The type-6/type-7 HTML block openers called
strings.ToLower(string(tagBytes)) on every trigger candidate line
(any line starting with '<'), allocating a new string each time.
internal/lint/layer0_html.go already solved the identical problem
with a stack-buffer lowering (tagBuf.lowerInto); this mirrors that
pattern locally in pkg/goldmark/parser, which cannot import
internal/lint. Confirmed 0 allocs/op for the new tagInAllowedSet/
isRawTextTag helpers.
Ref: docs/development/high-performance-go.md ("Stay in []byte").
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UCh2j2oeSw6UgNsmpjRonL
collectFile used a bare os.ReadFile while walking cloned third-party
repositories for the training corpus — the one file-ingestion loop
in the codebase reading genuinely untrusted external content without
the byte-cap every other read site already applies (bytelimit,
internal/rules/githooksync, internal/schema). A single oversized file
in a source repo used to be read into memory in full; it now fails
the walk instead.
Ref: docs/development/high-performance-go.md ("os.ReadFile on huge
inputs: one giant alloc, all resident").
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UCh2j2oeSw6UgNsmpjRonL
The comment described an earlier bytes.LastIndexByte approach that was abandoned (benchmarking showed no SIMD assembly backs LastIndexByte on any platform) in favor of the binary-search fix actually shipped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UCh2j2oeSw6UgNsmpjRonL
Review found that returning bytelimit's error straight from collectFile made it fatal: collectFromRoot's WalkDir callback treats any error as abort-the-walk, and Collect returns on the first source error, so one big file discarded every record already collected from every source in the run. Stat the file first and skip it (matching the existing too-small-word/char-count skip pattern) instead of reading and failing; a real cloned repository can have one oversized file (a big CHANGELOG, a vendored spec) without that being reason to fail the whole corpus build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UCh2j2oeSw6UgNsmpjRonL
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:
|
Review found the stat-based size pre-check protected only the common case: a file that grows past the cap between the Stat and the subsequent bytelimit.ReadFileLimited call still returned a hard error, reintroducing the exact whole-build-abort bug the pre-check was meant to fix, just for a narrower race window. Both the Stat error branch and the fallback ReadFileLimited error branch now skip-and-report instead of failing collectFile's caller. Added direct collectFile unit tests for both branches (a vanished path, and a directory passed as a file, which reliably fails at Read without depending on file-permission bits that root ignores) and rewrote the oversized-file test to use two Sources, proving an earlier source's records survive a later source's bad file — Collect's per-source loop returns on first error, discarding every prior record, which a single-source test can't exercise. Also: correct a stale comment in internal/linkgraph/linkgraph.go describing ColumnOfOffset's old O(column) backward scan (it's now O(log lines) via binary search), and add an integration-level test driving pkg/goldmark/parser's full HTML block Open() path with mixed-case tags — the existing equivalence/upstream-parity harness and TestHTMLBlock_AllSevenTypes only ever exercise lowercase tags, so a wiring mistake in the new tag-lookup helpers could otherwise only be caught by their own unit tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UCh2j2oeSw6UgNsmpjRonL
The binary-search rewrite only clamped the upper bound (offset > len(Source)); a negative offset now echoed back out unclamped (ColumnOfOffset(-5) == -4) instead of the pre-refactor behavior of always returning 1 for any negative offset. No shipped rule call site can currently trigger this (all pass an already-guarded, non-negative value), but the contract should still match LineOfOffset's and the prior implementation's handling of out-of-domain input. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UCh2j2oeSw6UgNsmpjRonL
|
🟢 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
A fleet of scan agents audited
internal/andpkg/againstdocs/development/high-performance-go.md,covering allocation/regex patterns, string/bytes handling, data-structure
layout, and concurrency/misc anti-patterns. Findings were ranked by how hot
the code path is (per-file, per-line, per-diagnostic) and whether the
benefit was empirically confirmed with a benchmark or alloc count — not
just plausible on paper. Two initially promising findings didn't survive
that check and were dropped (see "Findings considered and not taken",
below).
This PR fixes the top 5 that did survive, each with a red/green test
(a benchmark or
testing.AllocsPerRunassertion that fails before the fixand passes after) and a measured before/after number. Three rounds of
xhigh-severity adversarial code review then found and fixed a real
behavioral regression in one of the five fixes (see "Review findings",
below) plus a latent edge-case bug and a stale comment.
Fixes
internal/lint/file.go—ColumnOfOffset: hand-rolled backward bytescan → binary search reusing
LineOfOffset's cached newline index.O(line length) → O(log newline count). Benchmarked on an 8 KB line:
~5.3µs/op → ~9ns/op. This function has ~26 call sites (once per
diagnostic across many rules), so a single very long line used to make
every diagnostic on it pay for a full backward scan.
internal/rules/tocdirective/rule.go(MDS035) —matchVariant:converted every paragraph line to a
stringbefore regexp matching, inboth the Check and Fix paths. Switched to
regexp.Match([]byte)+bytes.TrimRight. 6 → 0 allocs/op on the rule's alloc-budgetfixture; the budget constant is tightened to match.
internal/rules/tablefmt/tablefmt.go(MDS025) —tryParseTable:rawLines/rowsgrew via plainappendwith no capacity hint, so amulti-row table paid several slice-growth reallocs. Added a
non-allocating
countDataRowspre-scan (mirroring the sibling ruletablereadability's established pattern) so both slices are pre-sizedonce. 174 → 162 allocs/op parsing a 52-row table (residual allocs
are
splitRowBytes's per-row cell slice, out of scope for this fix).pkg/goldmark/parser/html_block.go: the type-6/type-7 HTML blockopeners called
strings.ToLower(string(tagBytes))on every triggercandidate line (any line starting with
<).internal/lint/layer0_html.goalready solved the identical problem with a stack-buffer lowering
(
tagBuf.lowerInto); this mirrors that pattern locally (this packagecan't import
internal/lint). Confirmed 0 allocs/op for the newtagInAllowedSet/isRawTextTaghelpers, plus a newTestHTMLBlock_TagCaseInsensitiveintegration test driving the fullparser
Open()path with mixed-case tags. Fullpkg/goldmarksuite,including the upstream-equivalence harness, still passes.
internal/corpus/collect.go—collectFile: used a bareos.ReadFilewhile walking cloned third-party repositories for thetraining corpus — the one file-ingestion loop reading genuinely
untrusted external content without the byte cap every other read site
in the codebase already applies (
bytelimit,githooksync,schema).Now stats the file first and skips it (via
reportProgress+return Record{}, false, nil) when it exceedsbytelimit.DefaultMaxInputBytes— falling through tobytelimit.ReadFileLimitedas a second check against the file growingpast the cap between the stat and the read. Either check failing skips
the file; it does not fail the walk (see below for why that
distinction needed its own fix).
Review findings (fixed during the 3 xhigh review passes)
file. The first version of fix Refactor rule management to use dynamic rule registry #5 returned
collectFile's read erroras fatal. Since
collectFromRoot'sWalkDircallback aborts its wholewalk on any error, and
Collect's loop overcfg.Sourcesreturns on thefirst source error, a single oversized (or momentarily unreadable) file
anywhere discarded every record already gathered from every source in
the run — the opposite of resilient batch collection. Fixed in two
steps: the stat-based size pre-check skips instead of erroring, and the
fallback
bytelimit.ReadFileLimitederror (covering the TOCTOU racewhere a file grows past the cap between the stat and the read) does
too. Added direct
collectFileunit tests for both error branches, andrewrote the oversized-file test to use two
Sources(not one) to provean earlier source's records survive a later source's bad file.
ColumnOfOffset. Thebinary-search rewrite only clamped the upper bound; a negative offset
echoed back out unclamped instead of the prior implementation's
always-return-1 behavior. Not reachable through any shipped rule (every
call site already guards against negative input), but fixed to match
LineOfOffset's and the old implementation's contract.internal/linkgraph/linkgraph.gostill describedColumnOfOffset's old O(column) backward scan after fix Add heading-max, code-block-max, and stern mode to line-length rule #1 changed itto O(log lines).
Findings considered and not taken
ColumnOfOffset's backward scan as fixable viabytes.LastIndexByte(claimed SIMD-accelerated like the forwardbytes.IndexByte). Benchmarking showed no measurable difference(~5.3µs/op both ways) — Go's
bytes.LastIndexBytehas no assemblyimplementation on any platform, only a plain Go loop
(
internal/bytealg/lastindexbyte_generic.go). Went with the binary-searchfix above instead, which is a real algorithmic win.
context.Background()created per-request ininternal/rules/externallink/probe_net.goloses cancellation(Ctrl-C won't interrupt in-flight probes). Real, but the
rule.Rule.Checkinterface has no
context.Contextparameter anywhere in its ~150implementations — threading one through is an interface-wide change out
of scope for this fix set. Worth a dedicated follow-up plan.
docHeading,heading,revMatch)reduce GC scan surface (
ptrdata) but don't changeunsafe.Sizeof, sothere's no straightforward way to pin the improvement with a red/green
test. Left for a follow-up that can also cover the small
map[string]bool→
map[string]struct{}set candidates found in the same pass.Test plan
go build ./...go test ./...(repo-wide, all green)go vet ./...go tool -modfile=tools/go.mod golangci-lint run ./...(0 issues)go test -tags goldmark_upstream ./pkg/goldmark/...(equivalence harness green; one pre-existing, unrelated failure inpkg/markdown's arena tests confirmed present onmaintoo)internal/integrationalloc-budget and per-rule bench-budget gates passmdsmith check .— 563 files checked, 0 failures🤖 Generated with Claude Code
https://claude.ai/code/session_01UCh2j2oeSw6UgNsmpjRonL