perf: audit against high-performance-go.md, fix top 5 hot-path violations - #775
Draft
jeduden wants to merge 8 commits into
Draft
perf: audit against high-performance-go.md, fix top 5 hot-path violations#775jeduden wants to merge 8 commits into
jeduden wants to merge 8 commits into
Conversation
BaseNode is embedded in every AST node (Document, Heading, Paragraph, Text, Emphasis, Link, ...), making it the single most-allocated struct in the parser. childCount (a scalar) sat between the five Node-typed pointer fields and attributes (a pointer-bearing slice), forcing GC ptrdata to span through it for nothing. Per docs/development/high-performance-go.md "Group pointer fields first, scalars last", move attributes ahead of childCount so all pointer-bearing fields precede the scalar tail. Adds TestBaseNode_PointerFieldsBeforeScalars, reusing the fieldorder helper already shared by AutoLink's equivalent fix.
lc0Pass carries one ClassifyLines walk's state and is heap-allocated once per file on the flat Layer-0 classification path. htmlEnd (a pointer-bearing []byte) sat in the middle of a long scalar run, and pendingBlanks (also pointer-bearing) was the very last field — between the two, GC ptrdata was forced to span the entire struct on every allocation. Per docs/development/high-performance-go.md "Group pointer fields first, scalars last", group lines/out/stack/htmlEnd/pendingBlanks ahead of every scalar field so GC ptrdata only covers the pointer prefix. Adds TestLC0Pass_PointerFieldsFirst using the repo's existing internal/structlayout helper.
…ion attempt
ids.Generate built each candidate disambiguation suffix with
fmt.Sprintf("%s-%d", result, i) inside its collision loop: a heading
whose slug collides N deep (common in changelogs and release notes,
where a heading like "Changed" repeats across every version section)
allocated N times just walking the loop, plus fmt's reflection
overhead on every attempt.
Reuse one []byte buffer across attempts via strconv.AppendInt,
per docs/development/high-performance-go.md "strconv over
fmt.Sprintf", and only allocate once the winning id is decided.
TestIDs_Generate_AllocBudget seeds a 30-deep collision chain and pins
the call at <=5 allocations; it fails at ~75 allocations/op against
the prior fmt.Sprintf-per-attempt implementation.
parseDelimiter already knows the exact column count from
bytes.Split(line, []byte{'|'}) before the alignment loop runs, but
grew the alignments slice via a bare append() from nil — forcing
repeated reallocate-and-copy as it doubles past its starting capacity
on every table row in every file with a table.
Per docs/development/high-performance-go.md "Pre-size slices", size
alignments to len(cols) up front. TestParseDelimiter_AllocBudget pins
a 40-column row at <=3 allocations; it fails at 8 allocations/op
against the prior unbounded-append implementation.
MET006 (conciseness, enabled by default), MET009 (sentences), and MET010 (avg-words-per-sentence) each independently called mdtext.CountSentences on the same Document's plain text — a run computing more than one of these metrics on one file (e.g. `mdsmith metrics get` with no --metric filter, or `metrics rank` over several columns) recounted sentences up to three times per file. Per docs/development/high-performance-go.md "Memoize per-input computations", add Document.SentenceCount() following the existing PlainText/WordCount/HeadingCount lazy-cache pattern, and wire all three metrics through it. concisenessScore now takes a precomputed sentence count instead of recounting internally. TestMetrics_MET006AndMET009_ShareSentenceCount substitutes a counting stub for the package's countSentencesFn and asserts all three metrics together count sentences exactly once; it fails at 0 calls (the registry wasn't wired through Document yet) before this change.
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:
|
Code review on PR #775 caught a correctness regression in the pre-sizing commit (b63320e): make([]ast.Alignment, 0, len(cols)) is non-nil even when len(cols) == 0, but Transform (table.go) uses `alignments == nil` as its "not a delimiter row" sentinel. A bare "|" line passes isTableDelim and bytes.Split's blank-column trims reduce cols to zero, so the pre-sized empty-but-non-nil slice now satisfied that sentinel by accident, reached the header/child-count mismatch check, and returned early -- aborting the whole paragraph's table scan instead of just skipping that line. A real table later in the same paragraph silently disappeared from the parse. Return nil explicitly when cols is empty, before allocating. TestParseDelimiter_EmptyColsReturnsNil and TestTable_BarePipeLineBeforeRealTable (an end-to-end Convert regression test) pin this; both fail against the pre-fix code with the table missing from the output entirely. Also moves the correctness check in TestParseDelimiter_AllocBudget out of the testing.AllocsPerRun closure: t.Fatalf's runtime.Goexit should not fire mid-measurement over a correctness failure that a plain pre-check already catches more clearly.
Round 2 of code review on PR #775 found that TestParseDelimiter_AllocBudget and TestIDs_Generate_AllocBudget both fail under `go test -race`: the race detector's own allocation bookkeeping perturbs the count (39-75/op measured under -race vs. 2-4/op without it), well past each test's bound. The underlying fixes are correct; only the measurement is unreliable under the detector. Add the raceEnabled build-tag sentinel (race_on_test.go/race_off_test.go) to both packages, mirroring the existing pattern used across internal/rules/*/alloc_test.go and internal/integration, and skip each alloc gate when it is set.
…ndant copy Round 3 (final) of code review on PR #775 caught two issues: 1. The lc0Pass reorder's comments (lineclass.go, lineclass_test.go) claimed it was "heap-allocated once per ClassifyLines call" running "on every file's Layer-0 pass." Both claims are false: `go build -gcflags=-m` confirms `&lc0Pass{...} does not escape` (stack-allocated), and ClassifyLines is reached only through the default-off MDSMITH_SPIKE_FLAT_L0 measurement seam per its own doc comment 20 lines above. Reworded both comments to describe the reorder as layout hygiene / insurance against a future escape, not a measured production GC win. 2. ids.Generate's collision loop did `key := string(buf); ...; return []byte(key)` -- two copies of the same bytes where one suffices. buf is not aliased anywhere once a non-colliding suffix is found, so it can be returned directly instead of round-tripping through a second []byte(key) conversion. No behavior change; all existing tests (including the alloc-budget and struct-layout pins) still pass.
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
2 participants
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 fresh audit against
docs/development/high-performance-go.md, run after 13+ prior rounds of the same audit had already cleared most low-hanging fruit ininternal/rules/**. Four parallel scans (allocation budget, string/byte patterns, data structures/struct layout, and skip-work/anti-patterns) turned up real, currently-present violations concentrated inpkg/goldmark(the vendored parser — excluded from.golangci.ymllinting, so it had drifted from the linted tree) andinternal/metrics. Each fix below is a separate red/green TDD commit: a failing test first (confirmed failing against the pre-fix code), then the minimal fix to make it pass.Fixes
pkg/goldmark/ast:BaseNodefield reorder —BaseNodeis embedded in every AST node (Document, Heading, Paragraph, Text, Emphasis, Link, …), making it the single most-allocated struct in the parser.childCount(a scalar) sat between the pointer-typed sibling/parent fields andattributes(a pointer-bearing slice), forcing GC ptrdata to scan past it for nothing. Reordered per "Group pointer fields first, scalars last."internal/lint:lc0Passfield reorder — grouped its pointer-bearing fields ahead of its scalars for layout hygiene. (Reviewed down to:lc0Passis actually stack-allocated and only reached via a default-off measurement seam today, so this is insurance against a future escape rather than a measured production win — comments were corrected to say so.)pkg/goldmark/parser: heading-ID collision loop —ids.Generatebuilt each disambiguation suffix withfmt.Sprintf("%s-%d", result, i)inside its collision loop. A heading colliding N deep (common in changelogs/release notes where a heading like "Changed" repeats every version) allocated N times just walking the loop. Now reuses one[]bytebuffer viastrconv.AppendInt.pkg/goldmark/extension: table delimiter alignments pre-size —parseDelimiteralready knows the exact column count frombytes.Splitbefore its alignment loop, but grewalignmentsvia a bareappend()from nil, causing repeated reallocate-and-copy on wide tables. Now pre-sized withmake([]ast.Alignment, 0, len(cols)).internal/metrics: memoized sentence count — MET006 (conciseness, enabled by default), MET009 (sentences), and MET010 (avg-words-per-sentence) each independently calledmdtext.CountSentenceson the sameDocument's plain text, recounting up to 3x per file when more than one is requested. AddedDocument.SentenceCount()following the existingPlainText/WordCount/HeadingCountlazy-cache pattern.Caught during review (3 rounds of xhigh-severity review, each addressed before the next)
alignmentsslice in fix Add required-structure and include rules for document validation #4 is non-nil even when empty, butTransformusedalignments == nilas its "not a delimiter row" sentinel — a bare|line ahead of a real table silently dropped the whole table. Fixed with an explicitlen(cols) == 0guard, plus an end-to-end regression test.go test -race(the race detector's own bookkeeping perturbs allocation counts) and a factually incorrect test comment. Both fixed.lc0Passreorder (claimed heap allocation on every file in production; it's actually stack-allocated behind a default-off flag) and a minor redundant copy inids.Generate. Both corrected.Test plan
go build ./...go vet ./...go test ./...— full suite greengo test -raceon all changed package trees — green (repo-wide-racehas pre-existing, unrelated flakiness in other packages not touched by this PR)go tool -modfile=tools/go.mod golangci-lint run— 0 issuesgo run ./cmd/mdsmith check .— 567 files checked, 0 failures🤖 Generated with Claude Code