Merge PR #762: perf: fix top 5 high-performance-go.md violations found by codebase audit

Author: merge-queue-bot

internal/foreignregion/scan.go

@@ -9,6 +9,7 @@
 package foreignregion
 
 import (
+	"bytes"
 	"fmt"
 	"strings"
 
@@ -35,16 +36,80 @@ func Scan(f *lint.File, regions []config.ForeignRegion) ([]lint.LineRange, []lin
 	if len(regions) == 0 || f == nil {
 		return nil, nil
 	}
+	states := make([]regionScanState, len(regions))
+	for i, reg := range regions {
+		states[i] = regionScanState{
+			start: []byte(strings.TrimSpace(reg.Start)),
+			end:   []byte(strings.TrimSpace(reg.End)),
+		}
+	}
+
 	var ranges []lint.LineRange
 	var diags []lint.Diagnostic
-	for _, reg := range regions {
-		rs, ds := scanOne(f, reg)
-		ranges = append(ranges, rs...)
-		diags = append(diags, ds...)
+	for i, line := range f.Lines {
+		lineNum := i + 1
+		trimmed := bytes.TrimSpace(line)
+		for s := range states {
+			rs, ds := states[s].step(trimmed, lineNum)
+			ranges = append(ranges, rs...)
+			diags = append(diags, ds...)
+		}
+	}
+	for s := range states {
+		if ds := states[s].unclosed(); ds != nil {
+			diags = append(diags, ds...)
+		}
 	}
 	return ranges, diags
 }
 
+// regionScanState tracks one declared region's marker pair across a
+// single walk of f.Lines. A single pass evaluates every region's
+// state against the line's shared trim, instead of Scan walking
+// f.Lines once per region and re-converting every line to a string
+// each time — the redundant re-scanning docs/development/
+// high-performance-go.md calls out.
+type regionScanState struct {
+	start, end []byte
+	openLine   int // 1-based line of the current unclosed start; 0 when none
+}
+
+// step evaluates one already-trimmed line against this region's
+// markers and returns any newly closed range or malformed-region
+// diagnostic.
+func (st *regionScanState) step(trimmed []byte, lineNum int) ([]lint.LineRange, []lint.Diagnostic) {
+	switch {
+	case bytes.Equal(trimmed, st.start):
+		if st.openLine != 0 {
+			return nil, []lint.Diagnostic{diag(lineNum, fmt.Sprintf(
+				"duplicate foreign-region start marker %q before %q closed the open region",
+				st.start, st.end))}
+		}
+		st.openLine = lineNum
+	case bytes.Equal(trimmed, st.end):
+		if st.openLine == 0 {
+			return nil, []lint.Diagnostic{diag(lineNum, fmt.Sprintf(
+				"foreign-region end marker %q without a matching start marker %q",
+				st.end, st.start))}
+		}
+		r := lint.LineRange{From: st.openLine, To: lineNum}
+		st.openLine = 0
+		return []lint.LineRange{r}, nil
+	}
+	return nil, nil
+}
+
+// unclosed reports the missing-end diagnostic for a region whose
+// start marker was never closed, once the whole file has been walked.
+func (st *regionScanState) unclosed() []lint.Diagnostic {
+	if st.openLine == 0 {
+		return nil
+	}
+	return []lint.Diagnostic{diag(st.openLine, fmt.Sprintf(
+		"foreign-region start marker %q has no matching end marker %q",
+		st.start, st.end))}
+}
+
 // Apply appends the foreign-region spans for path to f.GeneratedRanges —
 // the same exclusion set that keeps style rules and fixers out of
 // `<?include?>` / `<?catalog?>` bodies — and returns the malformed-region
@@ -94,46 +159,6 @@ func resolve(f *lint.File, cfg *config.Config, path string) ([]lint.LineRange, [
 	return ranges, diags
 }
 
-// scanOne walks f.Lines once for a single marker pair, matching a line
-// against a marker by trimmed-line equality so leading indentation does
-// not defeat the match while incidental in-prose mentions do not.
-func scanOne(f *lint.File, reg config.ForeignRegion) ([]lint.LineRange, []lint.Diagnostic) {
-	start := strings.TrimSpace(reg.Start)
-	end := strings.TrimSpace(reg.End)
-	var ranges []lint.LineRange
-	var diags []lint.Diagnostic
-	openLine := 0 // 1-based line of the current unclosed start; 0 when none
-	for i, line := range f.Lines {
-		lineNum := i + 1
-		trimmed := strings.TrimSpace(string(line))
-		switch trimmed {
-		case start:
-			if openLine != 0 {
-				diags = append(diags, diag(lineNum, fmt.Sprintf(
-					"duplicate foreign-region start marker %q before %q closed the open region",
-					start, end)))
-				continue
-			}
-			openLine = lineNum
-		case end:
-			if openLine == 0 {
-				diags = append(diags, diag(lineNum, fmt.Sprintf(
-					"foreign-region end marker %q without a matching start marker %q",
-					end, start)))
-				continue
-			}
-			ranges = append(ranges, lint.LineRange{From: openLine, To: lineNum})
-			openLine = 0
-		}
-	}
-	if openLine != 0 {
-		diags = append(diags, diag(openLine, fmt.Sprintf(
-			"foreign-region start marker %q has no matching end marker %q",
-			start, end)))
-	}
-	return ranges, diags
-}
-
 // diag builds one malformed-region diagnostic at the given 1-based line
 // (in f.Lines coordinates; the caller adds any front-matter offset).
 func diag(line int, msg string) lint.Diagnostic {

internal/foreignregion/scan_test.go

@@ -103,3 +103,49 @@ func TestScanIndentedMarkerMatches(t *testing.T) {
 	require.Len(t, ranges, 1)
 	assert.Equal(t, lint.LineRange{From: 1, To: 3}, ranges[0])
 }
+
+// TestScanMultipleRegions_AllocsScaleWithLinesNotRegions pins
+// docs/development/high-performance-go.md's "stay in []byte" and
+// "skip redundant re-scanning" patterns for Scan with more than one
+// declared region. Before this test, Scan called scanOne once per
+// region, and each scanOne call re-walked the whole 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 a single per-line trim can be checked against
+// every region's markers in the same pass.
+//
+// The assertion: allocs for 5 regions must not exceed roughly what 1
+// region costs by more than a small per-region constant (each region
+// still needs its own open-marker state and, on this fixture, no
+// diagnostics) — a multi-pass implementation would instead scale
+// allocs linearly with len(f.Lines) times len(regions).
+func TestScanMultipleRegions_AllocsScaleWithLinesNotRegions(t *testing.T) {
+	var src []byte
+	for i := 0; i < 100; i++ {
+		src = append(src, []byte("A representative line of prose in the file.\n")...)
+	}
+	f := newFile(t, string(src))
+
+	oneRegion := []config.ForeignRegion{apm}
+	fiveRegions := []config.ForeignRegion{
+		apm,
+		{Start: "<!-- r2:start -->", End: "<!-- r2:end -->"},
+		{Start: "<!-- r3:start -->", End: "<!-- r3:end -->"},
+		{Start: "<!-- r4:start -->", End: "<!-- r4:end -->"},
+		{Start: "<!-- r5:start -->", End: "<!-- r5:end -->"},
+	}
+
+	oneAllocs := testing.AllocsPerRun(20, func() { Scan(f, oneRegion) })
+	fiveAllocs := testing.AllocsPerRun(20, func() { Scan(f, fiveRegions) })
+
+	// A single-pass, []byte-only scan pays a near-constant per-region
+	// setup cost (a states slice entry) regardless of file size; a
+	// multi-pass, string()-converting scan pays roughly 5x oneAllocs
+	// (100 lines re-converted per extra region). 3x oneAllocs+10 is
+	// comfortably below the multi-pass cost and above single-pass
+	// noise.
+	assert.LessOrEqualf(t, fiveAllocs, oneAllocs*3+10,
+		"Scan with 5 regions allocs (%v) must not scale with lines-per-region "+
+			"(1-region allocs: %v) — each region should reuse one per-line trim",
+		fiveAllocs, oneAllocs)
+}

internal/lint/file.go

@@ -263,6 +263,11 @@ type memoEntry struct {
 //
 // build is invoked directly (no wrapping closure) so the call adds
 // no per-Memo-call allocation beyond the cold-path memoEntry itself.
+// The warm path checks Load before LoadOrStore for the same reason:
+// LoadOrStore's second argument (&memoEntry{}) is constructed before
+// the call and discarded whenever the key already exists, so a plain
+// LoadOrStore would allocate one on every call regardless of hit or
+// miss.
 //
 // Panic safety mirrors sync.Once: if build panics, the entry is
 // still marked done (via the deferred Store) and the mutex is
@@ -271,8 +276,16 @@ type memoEntry struct {
 // Subsequent calls on the same key serve the zero-value cached
 // result instead of re-running build, matching upstream sync.Once.
 func (f *File) Memo(key string, build func() any) any {
+	if v, ok := f.scratch.Load(key); ok {
+		return memoLoad(v.(*memoEntry), build)
+	}
 	ei, _ := f.scratch.LoadOrStore(key, &memoEntry{})
-	e := ei.(*memoEntry)
+	return memoLoad(ei.(*memoEntry), build)
+}
+
+// memoLoad runs build at most once for e, then returns the cached
+// value — the double-checked-lock body shared by Memo and MemoFile.
+func memoLoad(e *memoEntry, build func() any) any {
 	if e.done.Load() {
 		return e.val
 	}
@@ -294,10 +307,16 @@ func (f *File) Memo(key string, build func() any) any {
 //
 // Panic safety matches Memo's contract: defer Unlock + defer
 // done.Store(true) keep the per-entry mutex from leaking a lock and
-// match sync.Once's "panic still marks done" semantics.
+// match sync.Once's "panic still marks done" semantics. The warm
+// path checks Load before LoadOrStore for the same reason Memo does.
 func (f *File) MemoFile(key string, build func(*File) any) any {
-	ei, _ := f.scratch.LoadOrStore(key, &memoEntry{})
-	e := ei.(*memoEntry)
+	var e *memoEntry
+	if v, ok := f.scratch.Load(key); ok {
+		e = v.(*memoEntry)
+	} else {
+		ei, _ := f.scratch.LoadOrStore(key, &memoEntry{})
+		e = ei.(*memoEntry)
+	}
 	if e.done.Load() {
 		return e.val
 	}

internal/lint/file_test.go

@@ -266,6 +266,30 @@ func TestFile_Memo(t *testing.T) {
 		"a distinct key must not re-run the first key's build")
 }
 
+// TestFile_Memo_WarmPathAllocatesNothing pins Memo's cache-hit cost at
+// zero allocs. MemoFile.CollectSectionParagraphs feeds every
+// paragraph-aware rule, so a per-call allocation on the warm path
+// multiplies by every rule pass that re-touches an already-memoized
+// key on the same File.
+//
+// Before this test, the warm path still paid for the throwaway
+// &memoEntry{} that f.scratch.LoadOrStore's second argument
+// constructs before the call — Go evaluates that argument whether or
+// not the key is already present, and the freshly built entry is
+// discarded on a hit. A Load-first check must run ahead of
+// LoadOrStore to avoid it.
+func TestFile_Memo_WarmPathAllocatesNothing(t *testing.T) {
+	f := &File{Path: "t.md"}
+	build := func() any { return 42 }
+
+	f.Memo("k", build)
+
+	allocs := testing.AllocsPerRun(200, func() {
+		f.Memo("k", build)
+	})
+	assert.Zero(t, allocs, "Memo's cache-hit path must not allocate")
+}
+
 // TestFile_Memo_ConcurrentSingleBuild pins that build runs exactly
 // once even under the concurrent readers the LSP can run against a
 // single document.

internal/lint/runcache.go

@@ -3,6 +3,7 @@ package lint
 import (
 	"strings"
 	"sync"
+	"sync/atomic"
 )
 
 // RunCache memoizes per-target-file reads (front matter, include
@@ -73,10 +74,16 @@ type RunCache struct {
 }
 
 // runCacheEntry guards a single cache slot so build runs exactly once
-// per key even when multiple goroutines race for it.
+// per key even when multiple goroutines race for it. atomic.Bool +
+// mutex is used instead of sync.Once, matching file.go's memoEntry:
+// once.Do takes a func() argument, and the closure load would pass
+// (`func() { e.val = build() }`) captures e and build, so it
+// allocates on every call regardless of whether Do's internal check
+// makes it a no-op.
 type runCacheEntry struct {
-	once sync.Once
 	val  any
+	done atomic.Bool
+	mu   sync.Mutex
 }
 
 // ParsedSchemaMetadata is the optional interface a parsed-schema
@@ -583,10 +590,33 @@ func (c *RunCache) InvalidateWikilinks() {
 	})
 }
 
-// load is the shared LoadOrStore + sync.Once primitive for both maps.
+// load is the shared cache-slot primitive for every RunCache map. It
+// checks Load before LoadOrStore so the warm (already-built) path
+// never constructs the throwaway &runCacheEntry{} that LoadOrStore's
+// second argument would otherwise allocate on every call — the same
+// value gets discarded whenever the key is already present, but Go
+// evaluates that argument before LoadOrStore can say so. build is
+// invoked directly (no wrapping closure), mirroring file.go's Memo.
 func load(m *sync.Map, key string, build func() any) any {
+	if v, ok := m.Load(key); ok {
+		return loadEntry(v.(*runCacheEntry), build)
+	}
 	ei, _ := m.LoadOrStore(key, &runCacheEntry{})
-	e := ei.(*runCacheEntry)
-	e.once.Do(func() { e.val = build() })
+	return loadEntry(ei.(*runCacheEntry), build)
+}
+
+// loadEntry runs build at most once for e, then returns the cached
+// value. The atomic.Bool fast path costs one atomic load on every
+// warm call; the mutex only guards the cold build.
+func loadEntry(e *runCacheEntry, build func() any) any {
+	if e.done.Load() {
+		return e.val
+	}
+	e.mu.Lock()
+	defer e.mu.Unlock()
+	if !e.done.Load() {
+		defer e.done.Store(true)
+		e.val = build()
+	}
 	return e.val
 }

internal/lint/runcache_test.go

@@ -1071,3 +1071,31 @@ func TestDuplicateParagraphs_InvalidateDropsEveryKeyForPath(t *testing.T) {
 	assert.Equal(t, 5, builds,
 		"both a.md keys must rebuild; b.md's slot must survive untouched")
 }
+
+// TestLoad_WarmPathAllocatesNothing pins load's cache-hit cost at zero
+// allocs. Every RunCache accessor (FrontMatter, Includes, GlobMatches,
+// ParsedSchema, DuplicateParagraphs, Wikilinks, CompiledCUE,
+// UniqueFieldIndex) is called at least once per host file across a
+// workspace run, so a per-call allocation on the warm path multiplies
+// by the corpus size.
+//
+// Before this test, load's warm path paid two allocations per call
+// regardless of hit/miss: LoadOrStore's second argument (&runCacheEntry{})
+// is constructed before the call and discarded on a hit, and
+// e.once.Do(func() { e.val = build() }) allocates the wrapping closure
+// as an argument even when Do's internal check makes it a no-op — the
+// exact closure-box anti-pattern file.go's memoEntry doc comment
+// describes fixing for File.Memo, which had the same gap: a Load-first
+// check before the throwaway LoadOrStore value must run first.
+func TestLoad_WarmPathAllocatesNothing(t *testing.T) {
+	var m sync.Map
+	build := func() any { return 42 }
+
+	// Warm the entry.
+	load(&m, "k", build)
+
+	allocs := testing.AllocsPerRun(200, func() {
+		load(&m, "k", build)
+	})
+	assert.Zero(t, allocs, "load's cache-hit path must not allocate")
+}

internal/rules/crossfilereferenceintegrity/layout_test.go

@@ -8,10 +8,15 @@ import (
 // TestStructLayout asserts the optimal size for the Rule struct.
 // Moving bool fields to the end (previously between larger fields, wasting
 // padding bytes) reduces size from 128 to 120 bytes and improves cache
-// utilisation across per-Check calls.
+// utilisation across per-Check calls. The cachedGlobSettingsErr memo
+// (globSettingsErr, globSettingsMu, globSettingsDone) added 24 bytes of
+// data; grouped with the two trailing bools, that 14-byte tail block
+// still rounds up to a 16-byte multiple of the struct's 8-byte
+// alignment, so the total grows from 120 to 144 with no extra padding
+// beyond that unavoidable 2-byte tail.
 func TestStructLayout(t *testing.T) {
 	got := unsafe.Sizeof(Rule{})
-	const want = uintptr(120)
+	const want = uintptr(144)
 	if got != want {
 		t.Errorf("unsafe.Sizeof(Rule{}) = %d; want %d (reorder fields to eliminate padding)",
 			got, want)