Merge PR #767: perf: audit against high-performance-go.md, fix 3 confirmed hot paths

Author: merge-queue-bot

internal/rules/build/alloc_test.go

@@ -0,0 +1,83 @@
+package build
+
+import "testing"
+
+// hasReservedDeviceNameBudget pins docs/development/high-performance-go.md's
+// "compile regexes at package scope" sibling pattern for case folding: gate
+// the strings.ToUpper allocation behind a cheap length check instead of
+// converting every path segment. reservedDeviceNames only holds 3-4 byte
+// entries (CON, PRN, COM1..9, LPT1..9), so a segment outside that length
+// range can never match and must not pay for the case-fold copy. Measured
+// baseline on a 3-path, 7-segment fixture: 10 allocs/op before the length
+// gate, 5 after (the two segments that do fall in the 3-4 byte range still
+// allocate, which is correct — only the always-allocate-on-every-segment
+// behavior regressed).
+const hasReservedDeviceNameAllocBudget = 5
+
+// hasReservedDeviceNameFixturePaths mirrors a small set of real repo-style
+// paths: a mix of segment lengths, most outside the reserved-name range.
+var hasReservedDeviceNameFixturePaths = []string{
+	"docs/readme.md",
+	"internal/rules/foo.go",
+	"scripts/build.sh",
+}
+
+// TestHasReservedDeviceName_AllocBudget pins the allocation regression gate
+// under a normal `go test` run (not only `-bench`), matching the project's
+// paragraphstructure.TestCheckAllocBudget convention.
+func TestHasReservedDeviceName_AllocBudget(t *testing.T) {
+	if testing.Short() {
+		t.Skip("alloc gate skipped in -short mode")
+	}
+
+	allocs := testing.AllocsPerRun(200, func() {
+		for _, p := range hasReservedDeviceNameFixturePaths {
+			_ = hasReservedDeviceName(p)
+		}
+	})
+	t.Logf("hasReservedDeviceName allocs/op over %d paths = %.1f (budget = %d)",
+		len(hasReservedDeviceNameFixturePaths), allocs, hasReservedDeviceNameAllocBudget)
+	if allocs > float64(hasReservedDeviceNameAllocBudget) {
+		t.Fatalf("hasReservedDeviceName allocs/op = %.1f, budget = %d; "+
+			"the length gate before strings.ToUpper may have regressed",
+			allocs, hasReservedDeviceNameAllocBudget)
+	}
+}
+
+// TestHasReservedDeviceName_Correctness pins that the length gate does not
+// change which paths are flagged: reserved names at every valid length
+// (3 and 4 bytes) still match, in any case, and non-reserved segments of
+// any length (including 3-4 byte look-alikes) do not.
+func TestHasReservedDeviceName_Correctness(t *testing.T) {
+	cases := []struct {
+		path string
+		want bool
+	}{
+		{"CON", true},
+		{"con", true},
+		{"dir/NUL.txt", true},
+		{"COM1.log", true},
+		{"com9", true},
+		{"LPT9", true},
+		{"CONSOLE.md", false},
+		{"docs/readme.md", false},
+		{"foo", false},
+		{"bar/baz.md", false},
+	}
+	for _, c := range cases {
+		if got := hasReservedDeviceName(c.path); got != c.want {
+			t.Errorf("hasReservedDeviceName(%q) = %v, want %v", c.path, got, c.want)
+		}
+	}
+}
+
+// BenchmarkHasReservedDeviceName reports allocs/op alongside ns/op so a
+// regression shows up in `go test -bench` output too.
+func BenchmarkHasReservedDeviceName(b *testing.B) {
+	b.ReportAllocs()
+	for i := 0; i < b.N; i++ {
+		for _, p := range hasReservedDeviceNameFixturePaths {
+			_ = hasReservedDeviceName(p)
+		}
+	}
+}

internal/rules/build/rule.go

@@ -393,6 +393,12 @@ func hasReservedDeviceName(p string) bool {
 		if dot := strings.IndexByte(seg, '.'); dot >= 0 {
 			seg = seg[:dot]
 		}
+		// Every reserved name is 3-4 bytes; gate the ToUpper allocation
+		// behind a length check so an ordinary (usually longer) path
+		// segment never pays for the case-fold copy.
+		if len(seg) < 3 || len(seg) > 4 {
+			continue
+		}
 		if setutil.Contains(reservedDeviceNames, strings.ToUpper(seg)) {
 			return true
 		}

internal/rules/githooksync/rule.go

@@ -28,6 +28,12 @@ const hookMaxReadBytes int64 = 1024 * 1024
 // hoisted to avoid re-converting the marker constant to []byte on every call.
 var preMergeMarkerBytes = []byte(githooks.PreMergeCommitMarker)
 
+// resolveHooksDir is a package variable so tests can substitute a
+// counting stub and assert driftParts resolves the hooks directory
+// exactly once per cache miss, instead of once for peekHookSource and
+// again for preMergeCommitHookDrift (each a `git rev-parse` subprocess).
+var resolveHooksDir = githooks.ResolveHooksDir
+
 func init() {
 	rule.Register(&Rule{})
 }
@@ -181,9 +187,12 @@ const (
 )
 
 // peekHookSource reports the current state of the pre-merge-commit
-// hook without parsing its contents.
-func peekHookSource(repoRoot string) hookSource {
-	hookPath := filepath.Join(githooks.ResolveHooksDir(repoRoot), "pre-merge-commit")
+// hook without parsing its contents. hooksDir is the repo's resolved
+// hooks directory (githooks.ResolveHooksDir), passed in so a caller
+// that also needs it (driftParts) resolves it once rather than
+// spawning a second `git rev-parse` for the same repo.
+func peekHookSource(hooksDir string) hookSource {
+	hookPath := filepath.Join(hooksDir, "pre-merge-commit")
 	data, err := bytelimit.ReadFileLimited(hookPath, hookMaxReadBytes)
 	if err != nil {
 		if os.IsNotExist(err) {
@@ -213,7 +222,8 @@ func (r *Rule) driftParts(repoRoot string) []string {
 	driftMu.Unlock()
 
 	hasDriver := githooks.HasMdsmithMergeDriver(repoRoot)
-	hookState := peekHookSource(repoRoot)
+	hooksDir := resolveHooksDir(repoRoot)
+	hookState := peekHookSource(hooksDir)
 	// Early-exit when the user has not opted in (no driver) and the hook
 	// is not mdsmith-managed. hookSourceUnreadable (file too large, bad
 	// perms) is included in the early-exit: we cannot verify the hook's
@@ -231,7 +241,7 @@ func (r *Rule) driftParts(repoRoot string) []string {
 	if msg := r.mergeDriverDrift(repoRoot, hasDriver, expectedGlobs); msg != "" {
 		parts = append(parts, msg)
 	}
-	if msg := r.preMergeCommitHookDrift(repoRoot); msg != "" {
+	if msg := r.preMergeCommitHookDrift(hooksDir); msg != "" {
 		parts = append(parts, msg)
 	}
 
@@ -301,9 +311,12 @@ func describeGlobs(patterns []string) string {
 // hook content. Returns an empty string if no hook is installed, the
 // hook is not mdsmith-managed, or the content matches. A non-ENOENT
 // read error is surfaced rather than silently passing so permission
-// or IO failures cannot mask real drift.
-func (r *Rule) preMergeCommitHookDrift(repoRoot string) string {
-	hookPath := filepath.Join(githooks.ResolveHooksDir(repoRoot), "pre-merge-commit")
+// or IO failures cannot mask real drift. hooksDir is the repo's
+// resolved hooks directory, shared with peekHookSource by driftParts
+// so the two checks spawn only one `git rev-parse` per repo instead
+// of one each.
+func (r *Rule) preMergeCommitHookDrift(hooksDir string) string {
+	hookPath := filepath.Join(hooksDir, "pre-merge-commit")
 	data, err := bytelimit.ReadFileLimited(hookPath, hookMaxReadBytes)
 	if err != nil {
 		if os.IsNotExist(err) {

internal/rules/githooksync/rule_test.go

@@ -1071,3 +1071,32 @@ func TestRule_Check_OversizedHookNoDriverSilent(t *testing.T) {
 	assert.Empty(t, diags,
 		"no driver registered: oversized third-party hook must not produce a diagnostic")
 }
+
+// TestDriftParts_ResolvesHooksDirOnce pins that a driftParts cache miss
+// spawns `git rev-parse --git-path hooks` exactly once, shared between
+// peekHookSource and preMergeCommitHookDrift, instead of once for each.
+func TestDriftParts_ResolvesHooksDirOnce(t *testing.T) {
+	dir := t.TempDir()
+	initRepoWithDriver(t, dir)
+	installCanonicalHook(t, dir)
+	require.NoError(t, os.WriteFile(filepath.Join(dir, ".gitattributes"),
+		[]byte(canonicalManagedBlock()), 0o644))
+
+	driftMu.Lock()
+	delete(driftCache, dir)
+	driftMu.Unlock()
+
+	calls := 0
+	orig := resolveHooksDir
+	resolveHooksDir = func(repoRoot string) string {
+		calls++
+		return orig(repoRoot)
+	}
+	defer func() { resolveHooksDir = orig }()
+
+	r := &Rule{}
+	parts := r.driftParts(dir)
+	assert.Empty(t, parts, "canonical repo must report no drift")
+	assert.Equal(t, 1, calls,
+		"driftParts must resolve the hooks directory once, not once per drift check")
+}

internal/rules/noreferencestyle/alloc_test.go

@@ -155,14 +155,19 @@ func TestMayContainFootnote(t *testing.T) {
 	assert.True(t, mayContainFootnote([]byte("[^note]: a definition.\n")))
 }
 
-// TestCheckFootnotes_NoNeedle_SkipsBothRegexPasses benchmarks
-// checkFootnotes on prose with no footnote syntax to demonstrate the
-// gate's real effect: without it, footnoteRefRE and footnoteDefRE each
-// run a full FindAllSubmatchIndex over the whole file on every Check
-// call, unconditionally, even though this rule (MDS043) is opt-in and
-// so only runs for workspaces that enabled it. b.Fatalf pins a budget
-// so a future regression that removes the gate is caught in CI rather
-// than by a human re-running benchstat.
+// footnoteCheckBudgetNs pins the gate's real effect: without it,
+// footnoteRefRE and footnoteDefRE each run a full FindAllSubmatchIndex
+// over the whole file on every Check call. Gated: ~1us (one
+// bytes.Contains scan). Ungated: ~580us (two full regex passes over
+// ~28KB). The budget keeps roughly the same ~15-20x headroom over the
+// gated baseline that BenchmarkCheckCorpusSmall/Large use (see
+// internal/engine/bench_test.go), well above measurement noise, while
+// staying two orders of magnitude below the ungated cost.
+const footnoteCheckBudgetNs = 50_000
+
+// BenchmarkCheckFootnotes_NoNeedle exercises checkFootnotes on prose with
+// no footnote syntax; benchstat-friendly (no assertion), consumed by
+// TestCheckFootnotes_NoNeedleBudget below for the enforced gate.
 func BenchmarkCheckFootnotes_NoNeedle(b *testing.B) {
 	var src []byte
 	for i := 0; i < 200; i++ {
@@ -175,18 +180,36 @@ func BenchmarkCheckFootnotes_NoNeedle(b *testing.B) {
 	require.NoError(b, err)
 	r := &Rule{}
 
-	b.ResetTimer()
 	for i := 0; i < b.N; i++ {
 		r.checkFootnotes(f)
 	}
-	perOp := float64(b.Elapsed().Nanoseconds()) / float64(b.N)
-	// Gated: ~1µs (one bytes.Contains scan). Ungated: ~580µs (two full
-	// regex passes over ~28KB). 50µs stays far above measurement noise
-	// while catching a dropped gate by two orders of magnitude.
-	const budgetNsPerOp = 50_000
-	if perOp > budgetNsPerOp {
-		b.Fatalf("checkFootnotes on a no-footnote file: %.0f ns/op, budget = %d; "+
+}
+
+// TestCheckFootnotes_NoNeedleBudget pins the ns/op regression gate under
+// a normal `go test` run. CI's check-bench/markdown-bench jobs only run
+// `-bench` against internal/engine, pkg/markdown, internal/lsp, and
+// cue/cuelite (see .github/workflows/ci.yml) — a plain
+// BenchmarkCheckFootnotes_NoNeedle with an inline b.Fatalf would never
+// execute in CI and the assertion would be dead code. testing.Benchmark
+// runs the benchmark function programmatically so the assertion lands
+// in a Test that `go test ./...` (and therefore CI) actually runs,
+// matching paragraphstructure.TestCheckAllocBudget's rationale for its
+// own Benchmark/Test pair.
+func TestCheckFootnotes_NoNeedleBudget(t *testing.T) {
+	if testing.Short() {
+		t.Skip("perf gate skipped in -short mode")
+	}
+	if raceEnabled {
+		t.Skip("perf gate skipped under -race; the race detector's " +
+			"instrumentation overhead perturbs the ns/op measurement")
+	}
+	result := testing.Benchmark(BenchmarkCheckFootnotes_NoNeedle)
+	perOp := float64(result.NsPerOp())
+	t.Logf("checkFootnotes on a no-footnote file = %.0f ns/op (budget = %d)",
+		perOp, footnoteCheckBudgetNs)
+	if perOp > footnoteCheckBudgetNs {
+		t.Fatalf("checkFootnotes on a no-footnote file: %.0f ns/op, budget = %d; "+
 			"the mayContainFootnote gate may have been removed or bypassed",
-			perOp, budgetNsPerOp)
+			perOp, footnoteCheckBudgetNs)
 	}
 }

internal/rules/noreferencestyle/race_off_test.go

@@ -0,0 +1,5 @@
+//go:build !race
+
+package noreferencestyle
+
+const raceEnabled = false

internal/rules/noreferencestyle/race_on_test.go

@@ -0,0 +1,5 @@
+//go:build race
+
+package noreferencestyle
+
+const raceEnabled = true