Skip to content

perf: replace map[string]bool sets with map[string]struct{} across hot paths - #679

Merged
jeduden merged 3 commits into
mainfrom
claude/kind-darwin-r97wed
Jun 22, 2026
Merged

perf: replace map[string]bool sets with map[string]struct{} across hot paths#679
jeduden merged 3 commits into
mainfrom
claude/kind-darwin-r97wed

Conversation

@jeduden

@jeduden jeduden commented Jun 21, 2026

Copy link
Copy Markdown
Owner

Summary

Performance audit against docs/development/high-performance-go.md, which states: "map[K]struct{} for sets — zero-byte value type." Five hot-path locations and two related cleanup sites used map[string]bool as presence-only sets, paying one wasted byte per entry and one wasted bool comparison per read.

Fixes (5 hot-path locations + 2 follow-ons)

1. internal/rules/catalog/rule.go — include-graph traversal (hottest path)
fileIncludesTarget / fileIncludesTargetAbs and their recursive workers scanIncludesForTarget / scanIncludesForTargetAbs carried visited map[string]bool through every per-file catalog directive evaluation. Changed to map[string]struct{}. DFS delete(visited, resolved) backtracking preserved.

2. internal/lint/runcache.go — LSP invalidation cycle guard
InvalidateinvalidateinvalidateDependents passed a map[string]bool cycle guard on every LSP file-edit event. Changed to map[string]struct{}.

3. internal/config/kind_extends.go + validate.go — extends chain walkers
Three chain-walking functions (KindExtendsChain, extendsChainSchemas, validateKindExtends) used map[string]bool for cycle detection. Changed to map[string]struct{}. KindExtendsChain also fixed to return nil instead of []string{} on empty result (project nil-not-empty convention). Two intermediate chain := []string{} accumulators changed to var chain []string (defers first allocation to first append).

4. internal/config/merge.go + provenance.go — per-file kind resolution
EffectiveKinds, resolveEffectiveKinds, resolveKindsWithSources, and allRuleNames de-dup sets changed from map[string]bool to map[string]struct{}.

5. internal/lint/files.go — CLI file deduplication
ResolveFilesWithOpts seen-set changed to map[string]struct{}.

6. pkg/mdsmith/workspace.go — memFS directory enumeration
memFS.dirEntries seen-set changed. Also fixed a short-circuit evaluation bug in the conversion: the if init; cond form if _, ok := seen[name]; name != "" && !ok ran the map probe before the guard (init always executes in Go's if init; cond form). Restored guard-first order with nested if blocks.

TDD

  • Red test: TestKindExtendsChain_EmptyNameReturnsNil (fails against old []string{} return; passes after var out []string)
  • Red state: Updating catalog test call sites to map[string]struct{} before updating function signatures produces type-mismatch compile errors; fixing signatures restores green
  • Workspace test: TestMemFSDirEntriesIgnoresEmptySegment added to document and pin the empty-name-segment guard in dirEntries

Code review passes

Three --max code review passes were run:

  • Pass 1: Found confirmed bug (short-circuit inversion in workspace.go) → fixed in 9b0fa79
  • Pass 2: No new confirmed correctness bugs
  • Pass 3: Found chain := []string{} allocation (confirmed), allRuleNames not converted (plausible), missing workspace test (confirmed TDD gap) → fixed in ef818c0

Test plan

  • go test ./... — all packages green
  • go build ./... — clean build
  • Verify internal/integration/alloc_budget_test.go passes (allocation budget gate)
  • Run go run ./cmd/mdsmith check . to confirm markdown lints clean

🤖 Generated with Claude Code

https://claude.ai/code/session_01QjbjBieiWMBKURdKVNnPvy


Generated by Claude Code

@codecov

codecov Bot commented Jun 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.62%. Comparing base (ff7ab4d) to head (6d89652).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files
Components Coverage Δ
Go 98.61% <100.00%> (+<0.01%) ⬆️
TypeScript 99.54% <ø> (ø)

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jeduden
jeduden marked this pull request as ready for review June 22, 2026 11:35
@jeduden jeduden added queue Add to a PR to enqueue it queue:active Applied automatically when a PR is in an active batch and removed queue Add to a PR to enqueue it labels Jun 22, 2026
@jeduden

jeduden commented Jun 22, 2026

Copy link
Copy Markdown
Owner Author

🟢 Merge Queue — picked up

This PR is in the queue and will be batched with other queue-labelled PRs.

Next: No action needed — you'll get another comment when CI starts on the batch. View merge queue run.

@jeduden jeduden added the queue:failed Applied automatically when CI fails or merge conflict occurs label Jun 22, 2026
@jeduden

jeduden commented Jun 22, 2026

Copy link
Copy Markdown
Owner Author

⚠️ Merge Queue — merge conflict

This PR could not be merged into the batch branch without conflicts with main or another queued PR.

Next: Rebase onto or merge main into your branch, resolve conflicts, push, then re-add the queue label.

@jeduden jeduden removed the queue:active Applied automatically when a PR is in an active batch label Jun 22, 2026
claude added 3 commits June 22, 2026 11:42
…y returns

Five hot-path locations used map[string]bool as a visited/dedup set,
storing only true values and paying one wasted byte per entry. Per the
high-performance Go guidelines (docs/development/high-performance-go.md),
pure sets must use map[string]struct{} — zero-byte value type.

- catalog/rule.go: scanIncludesForTarget + scanIncludesForTargetAbs
  visited sets (called per-file per catalog directive, hot check path)
- lint/runcache.go: Invalidate/invalidate/invalidateDependents cycle guard
  (called on every LSP file edit)
- config/kind_extends.go + validate.go: validateKindExtends, extendsChainSchemas,
  KindExtendsChain visited sets; KindExtendsChain also fixed to return nil
  instead of []string{} for empty results (project return-nil convention)
- config/merge.go + provenance.go: EffectiveKinds, resolveEffectiveKinds,
  resolveKindsWithSources dedup sets (called per-file during engine run)
- pkg/mdsmith/workspace.go: memFS.dirEntries dedup set (workspace walk)
- lint/files.go: ResolveFilesWithOpts dedup set (CLI file resolution)

Red: TestKindExtendsChain_EmptyNameReturnsNil (wrong return type) +
     type-mismatch compile errors in catalog tests (wrong visited type).
Green: all tests pass after signature + call-site updates.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QjbjBieiWMBKURdKVNnPvy
…check

The previous commit changed `if name != "" && !seen[name]` to
`if _, ok := seen[name]; name != "" && !ok`. In Go's `if init; cond`
form the init statement always executes before the condition, so the
map probe ran unconditionally — even when `name == ""` — reversing
the original short-circuit semantics.

Restore the guard-first order by nesting the membership check inside
an explicit `if name != ""` block, matching the original evaluation
order and preventing any wasted probe on empty segment paths.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QjbjBieiWMBKURdKVNnPvy
Third code-review pass found three issues:

1. validate.go and kind_extends.go: `chain := []string{}` allocates an
   empty backing array on every call. `var chain []string` (nil slice)
   defers the first allocation to the initial append, saving one heap
   allocation on every call where the loop never appends (kinds without
   an extends: chain in validateKindExtends, or when the extends chain
   has no schemas in extendsChainSchemas).

2. provenance.go: `allRuleNames` was the last `map[string]bool` presence
   set in the file not converted by the previous commit. Changed to
   `map[string]struct{}` for consistency with the surrounding code.

3. workspace_test.go: Add TestMemFSDirEntriesIgnoresEmptySegment to
   document and pin the empty-name-segment guard in memFS.dirEntries.
   A key with a double-slash segment (e.g. "a//b.md") produces an empty
   first component after stripping the directory prefix; the nested-if
   guard introduced in 9b0fa79 must check `name != ""` before the
   seen-map probe. The test constructs memFS directly (bypassing
   NewMemWorkspace path.Clean) to exercise this path.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QjbjBieiWMBKURdKVNnPvy
@jeduden
jeduden force-pushed the claude/kind-darwin-r97wed branch from ef818c0 to 6d89652 Compare June 22, 2026 11:44
@jeduden jeduden added queue Add to a PR to enqueue it and removed queue:failed Applied automatically when CI fails or merge conflict occurs labels Jun 22, 2026 — with Claude
@jeduden jeduden added queue:active Applied automatically when a PR is in an active batch and removed queue Add to a PR to enqueue it labels Jun 22, 2026
@jeduden

jeduden commented Jun 22, 2026

Copy link
Copy Markdown
Owner Author

🟢 Merge Queue — picked up

This PR is in the queue and will be batched with other queue-labelled PRs.

Next: No action needed — you'll get another comment when CI starts on the batch. View merge queue run.

@jeduden

jeduden commented Jun 22, 2026

Copy link
Copy Markdown
Owner Author

🔵 Merge Queue — CI running

Merged into batch branch merge-queue/batch-679-1782128941. View CI run.

Next: No action needed — you'll be notified when CI completes.

@jeduden jeduden removed the queue:active Applied automatically when a PR is in an active batch label Jun 22, 2026
@jeduden
jeduden merged commit cbd42a3 into main Jun 22, 2026
32 checks passed
@jeduden

jeduden commented Jun 22, 2026

Copy link
Copy Markdown
Owner Author

Merge Queue — merged

This PR landed on main via commit cbd42a3. CI run that validated the merge.

Next: Done — nothing more to do here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants