perf: apply high-performance-go guidelines across 5 hot paths (3-round review) - #688
Merged
Conversation
Audited the codebase against docs/development/high-performance-go.md
and fixed the five highest-impact violations identified by scanning
internal/ and rules/.
1. rename: map[int]bool → map[int]struct{} (guideline: map[K]struct{}
for sets). ValidRefDefBodyLines and contentBlockLines tracked
presence-only with bool values; struct{} removes the 1-byte value
overhead and makes the set semantics explicit. Updated callers in
lsp/rename.go and rename_test.go.
2. export: map[int]bool → map[int]struct{} for stripLines/bodyLines
(same guideline). stripDirectives, overlapsAny, and emitLines all
operated on presence-only maps; changed all three sites.
3. secreview/render: fmt.Sprintf(":%d") → strconv.Itoa (guideline:
strconv over fmt.Sprintf). strconv.Itoa is ~3× faster than
fmt.Sprintf for integer formatting because it skips reflection and
format-string parsing.
4. index/locate: eliminate string(lines[i]) per loop iteration in
enclosingListKey (guideline: stay in []byte). FindSubmatch([]byte)
replaces FindStringSubmatch(string(line)), removing one heap
allocation per scanned line in the upward-scan loop. Only the
matched key group is converted to string at the return point.
5. concisenessscoring: eliminate message += fmt.Sprintf pattern
(guideline: strings.Builder over +). A single conditional
fmt.Sprintf replaces the initial Sprintf followed by a
string-concatenation assignment, removing one heap allocation per
diagnostic when verbose cues are present.
All tests pass. mdsmith check . reports 0 failures.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015QYub2iGP6CeeEFSfjV3Wv
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:
|
…rage Three test improvements from code-review round 1: - rule_test.go: replace vacuous assert.NotEmpty (which strings.Contains already implies non-empty) with a meaningful assert.Contains for "e.g.," so the assertion can actually go red if the format string loses its example section - rule_test.go: add TestCheck_NoCuesMessage to exercise the `if examples == ""` branch — verboseParagraph always produces cues so the base-message-only path was uncovered; fixes the Codecov patch check failure (40% → 100% on that branch) - locate_test.go: rename TestEnclosingListKey_NoStringAllocPerLine to TestEnclosingListKey_FindsParentKey; the test only asserts the return value, not allocation behaviour, so the old name was misleading Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QYub2iGP6CeeEFSfjV3Wv
Two follow-on fixes from code review round 2: - locate.go: piArgRE's `\s*$` suffix already strips trailing whitespace from capture group 2, so `bytes.TrimSpace(m[2])` was always a no-op; replace with plain `len(m[2]) == 0`. - concisenessscoring/rule_test.go: TestCheck_NoCuesMessage previously used a hardcoded MinScore of 0.50, which would break if the embedded model drifts. Rewrite to probe NewScorer() at runtime, assert len(Cues)==0 (skip if the model now sees cues), and set MinScore = scored.Conciseness + 0.10 so the threshold is always just above the actual score. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QYub2iGP6CeeEFSfjV3Wv
…seness rule Two fixes from code-review round 3: - rule_test.go: TestCheck_MessageNoConcatenationWhenExamplesPresent guarded the assert.Contains(msg, "e.g.,") behind `if strings.Contains(msg, "reduce verbose cues")`, making it a no-op in the exact regression it was meant to catch (message drops cue text → outer if is false → inner assert never runs). verboseParagraph() always produces cues, so assert both strings unconditionally. Remove the now-unused "strings" import. - rule.go: Replace the if/else with two identical fmt.Sprintf call sites (same three arguments, same format prefix) with a single call that takes a conditional cuesSuffix string. The base format string now lives in one place so it can't diverge between the cue and no-cue paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015QYub2iGP6CeeEFSfjV3Wv
jeduden
changed the title
jeduden
marked this pull request as ready for review
June 23, 2026 20:54
jeduden
added
queue
Owner
Author
|
🟢 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. |
Owner
Author
|
🔵 Merge Queue — CI running Merged into batch branch Next: No action needed — you'll be notified when CI completes. |
jeduden
removed
the
queue:active
Owner
Author
|
✅ Merge Queue — merged This PR landed on Next: Done — nothing more to do here. |
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
Performance audit against
docs/development/high-performance-go.md, fixing 5 violations across core packages. Three rounds ofcode-review --fixat high severity were applied before merging.Fixes (commit
7c9dfc7)internal/export/export.gomap[int]boolsets for line trackingmap[int]struct{}— zero-size value, same semanticsinternal/index/locate.goenclosingListKeyFindStringSubmatch(string(lines[i]))— onestring()copy per scanned lineFindSubmatch(lines[i])— regex operates directly on[]byteinternal/lsp/rename.go/internal/rename/rename.goValidRefDefBodyLinesreturnedmap[int]bool; lookup usedmap[bool]zero-valuemap[int]struct{}; lookups use_, okidiominternal/secreview/render.golocStrfmt.Sprintf(":%d", n)— reflection overhead for integer formatting":" + strconv.Itoa(n)— ~3× faster, no reflectioninternal/rules/concisenessscoring/rule.gomessage += fmt.Sprintf(...)— intermediate string allocation on verbose pathfmt.Sprintfvia if/else — no extra allocationCode-review round 1 fixes (commit
917eafa)rule_test.goTestCheck_MessageNoConcatenationWhenExamplesPresent: replaced vacuousassert.NotEmptywithassert.Contains(msg, "e.g.,")— the original guard could never faillocate_test.go: renamedTestEnclosingListKey_NoStringAllocPerLine→TestEnclosingListKey_FindsParentKey(old name implied allocation testing the test didn't do)rule_test.go: addedTestCheck_NoCuesMessageto cover theexamples == ""branch, which was missing coverage (Codecov patch gate was failing at 82%)Code-review round 2 fixes (commit
12f6a79)locate.goenclosingListKey: removed redundantbytes.TrimSpace(m[2])— piArgRE's\s*$suffix structurally guarantees group 2 has no trailing whitespace; replaced withlen(m[2]) == 0rule_test.goTestCheck_NoCuesMessage: replaced hardcodedMinScore: 0.50(fragile against model drift) with a runtime scorer probe — callsNewScorer(), assertslen(Cues) == 0, and setsMinScore = scored.Conciseness + 0.10Code-review round 3 fixes (commit
3a45453)rule_test.goTestCheck_MessageNoConcatenationWhenExamplesPresent: removed conditional guardif strings.Contains(msg, "reduce verbose cues")— the innerassert.Contains(msg, "e.g.,")was dead code in the exact regression it was meant to catch (if implementation drops cue text, the outerifis false and the inner assert never runs);verboseParagraph()always produces cues so both asserts are now unconditionalrule.go: unified the twofmt.Sprintfcall sites (sharing the same base format and three repeated arguments) into a single call with a conditionalcuesSuffixstring — base format string now lives in one placeTest plan
go test ./...code-review --fixat high severity — no surviving CONFIRMED findings after round 3go vet ./...clean🤖 Generated with Claude Code
https://claude.ai/code/session_015QYub2iGP6CeeEFSfjV3Wv