perf: five high-performance-go fixes from codebase audit - #697
Conversation
Audited the codebase against docs/development/high-performance-go.md
and fixed the five highest-impact violations ranked by severity:
1. pkg/goldmark/extension/footnote.go — replace fmt.Sprintf("%v", n.RefIndex)
with strconv.Itoa in renderFootnoteLink/renderFootnoteBacklink. The %v
format uses reflection and bypasses the small-integer string cache; Itoa
skips reflection and returns a cached literal for single-digit indices
(the common case). Removes the now-unused fmt import.
2. pkg/goldmark/parser/html_block.go — change allowedBlockTags from
map[string]bool to map[string]struct{}. The 64-entry table is queried
on every HTML block line in the parser hot path; zero-byte struct{}
values eliminate the 8-byte bool-per-entry overhead and reduce GC scan
work. Lookup callers already used the two-value _, ok form so no
usage changes are needed.
3. internal/lint/layer0_html.go — same map[string]bool → map[string]struct{}
fix for the parallel allowedBlockTags table in the Layer-0 line scanner
(the per-file hot path). Updates tagInAllowedSet to use the _, ok lookup
form required by the struct{} value type.
4. cue/cuelite/engine.go — replace fmt.Sprintf with strconv for the three
scalar cases in describe(): kInt → strconv.FormatInt, kFloat →
strconv.FormatFloat, kBool → strconv.FormatBool. The kBool case is the
most impactful: strconv.FormatBool returns a string literal (0 allocs)
whereas fmt.Sprintf allocates a new string every call.
5. internal/schema/compose.go + shortcuts.go — change two map[string]bool
set usages to map[string]struct{}: the local seen map in unionStrings
(called for every schema merge) and the package-level cueBuiltinTypes
lookup table. Updates the single bool-value read in resolveBareName to
the _, ok two-value form.
Each fix is covered by a new test written before the code change:
- describe_alloc_test.go: TestDescribe_Bool_ZeroAlloc was red (1 alloc)
before the strconv.FormatBool fix and is green (0 allocs) after.
- Behavioral correctness tests pin the unchanged semantics of footnote
RefIndex rendering, allowedBlockTags lookups, tagInAllowedSet, and
unionStrings deduplication.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3D9gHDajSj9MUVVavzYQT
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:
|
…n up test comments
Add "meta" and "search" to lineclass_scan.go's htmlType6Tags, aligning it
with layer0_html.go and pkg/goldmark/parser/html_block.go which both carry
the full CommonMark spec list. A new test pins the complete list.
Remove PR-temporal wording from four test file comments ("RED before the
fix", "changed from map[string]bool to map[string]struct{}") that described
the refactor history rather than a permanent invariant.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01G3D9gHDajSj9MUVVavzYQT
… add meta/search to layer0 test TestHTMLType6Tags_CommonMarkComplete previously only checked "meta" and "search" (the two tags just added), leaving the other 61 CommonMark type-6 tags unprotected against accidental deletion. Replace the two-entry spot check with the complete CommonMark 0.31.2 type-6 tag enumeration. Also add "meta" and "search" to the known-tags list in TestTagInAllowedSet_KnownTags so allowedBlockTags in layer0_html.go is pinned for those entries too. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3D9gHDajSj9MUVVavzYQT
…dex comment TestAllowedBlockTags_KnownAndUnknown checked only 38 of the 63 CommonMark type-6 tags; expand it to the same full enumeration used by TestHTMLType6Tags_CommonMarkComplete so allowedBlockTags in the goldmark fork has parity coverage with lineclass_scan's htmlType6Tags. Also fix two misleading comments in TestFootnote_MultiRefRendersRefIndex: the second reference has RefIndex=1 (zero-based), not RefIndex=2, and the example id in the doc comment was "fnref2:1" instead of "fnref1:1". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3D9gHDajSj9MUVVavzYQT
…nMark list The known-tags list covered only 36 of 63 CommonMark type-6 tags, leaving 27 entries (base, basefont, caption, center, dir, fieldset, frame, etc.) unchecked against tagInAllowedSet. Replace with the complete enumeration, matching the coverage now used by TestHTMLType6Tags_CommonMarkComplete and TestAllowedBlockTags_KnownAndUnknown. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01G3D9gHDajSj9MUVVavzYQT
|
🔍 Merge Queue — bisecting A larger batch failed CI. Bisection is isolating the culprit: this run tests up to 3 of 5 candidate PRs on Next: No action needed — you'll be notified when the culprit is isolated, this PR merges, or this PR returns to the queue for a later batch. |
|
⏳ 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. |
|
🔍 Merge Queue — bisecting A larger batch failed CI. Bisection is isolating the culprit: this run tests up to 2 of 3 candidate PRs on Next: No action needed — you'll be notified when the culprit is isolated, this PR merges, or this PR returns to the queue for a later batch. |
|
✅ Merge Queue — merged This PR landed on Next: Done — nothing more to do here. |
Summary
Audited the codebase against
docs/development/high-performance-go.mdand fixed the five highest-impact violations, ranked by severity and hot-path position. Three rounds of xhigh-effort code review surfaced and resolved follow-on issues.Fix 1 —
footnote.go:fmt.Sprintf("%v", n.RefIndex)→strconv.ItoaFiles:
pkg/goldmark/extension/footnote.gorenderFootnoteLinkandrenderFootnoteBacklinkcalledfmt.Sprintf("%v", n.RefIndex)to format the back-reference index into HTML.fmt.Sprintfuses reflection to dispatch the format verb and allocates a new string every call.strconv.Itoais a direct integer→string conversion and returns a cached literal for single-digit indices (the common case for most documents), making this call zero-allocation forRefIndex ≤ 9. The now-unusedfmtimport is also removed.Red test:
TestFootnote_MultiRefRendersRefIndex— new coverage of the RefIndex > 0 code path; verifies that a doubly-referenced footnote rendersfnref:1andfnref1:1with the correct ref-index suffix.Fix 2 —
html_block.go:map[string]bool→map[string]struct{}Files:
pkg/goldmark/parser/html_block.goThe 64-entry
allowedBlockTagspackage-level lookup table usedmap[string]boolwhere the bool value is alwaystrueand never read — only the presence check (_, ok :=) matters.map[string]struct{}eliminates 8 bytes of bool-per-entry overhead (≈512 bytes for 64 entries) and reduces GC scan work on each cycle. Both call sites already used the two-value_, okform so no lookup code changes are needed.Red test:
TestAllowedBlockTags_KnownAndUnknown— internal package test verifying the map contains all 63 CommonMark type-6 block tags and excludes inline tags.Fix 3 —
layer0_html.go:map[string]bool→map[string]struct{}Files:
internal/lint/layer0_html.goIdentical fix for the parallel
allowedBlockTagstable in the Layer-0 line scanner — the innermost per-file hot path that classifies every Markdown line. The single callertagInAllowedSetreturned the bool value directly; updated to the_, oktwo-value lookup required by the new value type.Red test:
TestTagInAllowedSet_KnownTags/TestTagInAllowedSet_UnknownTags— verify case-insensitive lookup returns true for all CommonMark block tags and false for inline tags.Fix 4 —
engine.go:fmt.Sprintf→strconvfor scalar formattingFiles:
cue/cuelite/engine.godescribe()in the CUE-subset engine usedfmt.Sprintffor three concrete scalar cases, all of which have zero-overheadstrconvequivalents:kIntfmt.Sprintf("%d", v.i)strconv.FormatInt(v.i, 10)kFloatfmt.Sprintf("%g", v.f)strconv.FormatFloat(v.f, 'g', -1, 64)kBoolfmt.Sprintf("%t", v.b)strconv.FormatBool(v.b)Red test:
TestDescribe_Bool_ZeroAlloc— allocation gate:testing.AllocsPerRunexpected 0 allocs per call. Was failing (1 alloc) withfmt.Sprintf; passes withstrconv.FormatBool.TestDescribe_IntBool_Valuespins the exact string output for both cases.Fix 5 —
compose.go+shortcuts.go:map[string]bool→map[string]struct{}Files:
internal/schema/compose.go,internal/schema/shortcuts.gounionStrings(called for every schema merge during kind composition) created a localmap[string]bool{}deduplication set. Changed tomap[string]struct{}{}— same lookup semantics, 8 fewer bytes per unique string entry.cueBuiltinTypes(package-level lookup table, queried for every bare frontmatter value during schema parsing) changed frommap[string]booltomap[string]struct{}. Updated the single bool-read caller inresolveBareNameto the_, oktwo-value form.Red test:
TestUnionStrings_Dedup— covers disjoint inputs, overlapping inputs, all-duplicates, and nil inputs; verifies first-seen order is preserved and empty results returnnil(project convention).Follow-on fix —
lineclass_scan.go: add missing CommonMark tagsCode review (Round 1) found that
lineclass_scan.go'shtmlType6Tagswas missing"meta"and"search"— both present in the other two copies of the tag list. Added both entries and expandedTestHTMLType6Tags_CommonMarkCompleteto enumerate all 63 CommonMark 0.31.2 type-6 tags (previously only checked the two newly-added ones).Code review (Round 3) found that
TestAllowedBlockTags_KnownAndUnknownin the goldmark fork only checked 38 of 63 tags. Expanded it to the full 63-tag enumeration, matching the lineclass_scan gate.Test plan
go test ./...— zero failures across all packagesTestDescribe_Bool_ZeroAllocwas red (1 alloc) before Fix 4, green (0 allocs) afterReferences
docs/development/high-performance-go.md— the guideline this PR audits againstinternal/lint/lineclass_scan.go:453-455— thehtmlType6Tagsmap already usingmap[string]struct{}with the guideline comment that motivated Fixes 2 and 3🤖 Generated with Claude Code
https://claude.ai/code/session_01G3D9gHDajSj9MUVVavzYQT