Skip to content

Plan 175: check-performance optimizations (−38% latency) - #331

Merged
jeduden merged 7 commits into
mainfrom
claude/trace-linting-performance-y5b0K
May 17, 2026
Merged

Plan 175: check-performance optimizations (−38% latency)#331
jeduden merged 7 commits into
mainfrom
claude/trace-linting-performance-y5b0K

Conversation

@jeduden

@jeduden jeduden commented May 17, 2026

Copy link
Copy Markdown
Owner

Implements performance optimizations from plan 175's trace analysis, achieving 38% latency reduction on the check gate (1677 → 1046 us/file, p95 1006 → 627 ms).

Summary

This branch lands six behaviour-preserving performance wins identified through profiling the single-core check path:

  1. Link reference caching — MDS053/MDS054 no longer re-parse the whole file; they read link reference definitions from the parse NewFile already ran via new lint.File.LinkReferences() method (~10% CPU savings)

  2. Parser pooling — The goldmark parser is taken from a sync.Pool instead of rebuilt per file (~5% allocations)

  3. AST walk memoizationCollectCodeBlockLines / CollectPIBlockLines are cached per File (eliminates ~20 redundant AST walks per file)

  4. Source context skipping — Per-diagnostic source-context strings are skipped when the caller discards them (the benchmark, machine output)

  5. Filesystem cachecrossfilereferenceintegrity memoizes per-link os.Stat / filepath.EvalSymlinks in package-level sync.Maps (Syscall6 was ~5.7% flat)

  6. Word countingmdtext.CountWords counts in an allocation-free rune scan instead of len(strings.Fields(...)) (~0.48 GB saved)

Key Changes

  • internal/lint/file.go: Added LinkReferences() method backed by sync.Once to expose goldmark's parsed link reference definitions without re-parsing. Captures parser.Context during NewFile and reuses it. Added codeBlockLines / piBlockLines caches with sync.Once guards.

  • internal/lint/file.go: Introduced parserPool (sync.Pool) and parseWithPooledParser() to reuse goldmark parser instances across calls, reducing per-file allocation overhead.

  • internal/lint/codeblocks.go: Wrapped CollectCodeBlockLines / CollectPIBlockLines with sync.Once memoization; moved implementation to private collectCodeBlockLines / collectPIBlockLines functions.

  • internal/rules/noundefinedreferencelabels/rule.go and internal/rules/nounusedlinkdefinitions/rule.go: Replaced manual re-parsing with calls to f.LinkReferences(), eliminating redundant parses.

  • internal/rules/crossfilereferenceintegrity/fscache.go (new): Added cachedStatExists() and cachedEvalSymlinks() backed by package-level sync.Map caches to memoize filesystem operations.

  • internal/mdtext/mdtext.go: Rewrote CountWords() to count in a single rune scan instead of allocating strings.Fields() slice.

  • internal/engine/check.go: Added skipSourceContext parameter to checkRules() to suppress populateSourceContext() when callers don't render source lines (benchmark, machine output).

  • internal/engine/runner.go: Added SkipSourceContext field to Runner to allow callers to opt out of source context population.

  • internal/engine/bench_test.go: Updated benchmark to set SkipSourceContext=true to measure rule execution without allocation overhead from unused source windows.

Testing

All caches are guarded by sync.Once, sync.Pool, or sync.Map to remain race-free under concurrent access (verified with -race). New tests cover:

  • TestNewFile_ConcurrentParseRaceFree — 32 goroutines parsing concurrently
  • TestNewFile_SharedFileConcurrentReaders — 16 goroutines reading one *File
  • TestLinkReferences_FromNewFileParse

https://claude.ai/code/session_018yuh89L418oRxz5V2QyyXg

claude added 6 commits May 17, 2026 18:44
Trace analysis of the single-core Run path (plan 175 profiler
loop) found three behaviour-preserving wins, all guarded for the
multi-goroutine check / LSP path:

- MDS053/MDS054 no longer re-parse the whole file just to read
  link reference definitions (~10% of check CPU). NewFile keeps
  the parse context it already produced; lint.File.LinkReferences
  exposes the defs (sync.Once, struct-literal fallback parses
  once via the pool).
- The goldmark parser is taken from a sync.Pool instead of
  rebuilt per file (~5% of allocations), mirroring the proven
  internal/index/build.go pattern.
- CollectCodeBlockLines / CollectPIBlockLines are memoized per
  File, collapsing ~20 redundant AST walks per file into one.

Equivalence is covered by the existing MDS053/MDS054 unit and
fixture suites plus the integration runner; new tests pin the
caches' identity and a -race concurrency surface.

https://claude.ai/code/session_018yuh89L418oRxz5V2QyyXg
populateSourceContext copied a []string window and string(line)
per diagnostic for every file — the single largest object count
on the check gate (~315 MB / 3.8M objects) and pure waste when
the caller never renders SourceLines. Add Runner.SkipSourceContext
(default false, so CLI text output is unchanged) and an internal
checkRules variant; the benchmark opts in since it discards the
Result. Public CheckRules signature and behaviour are unchanged.

https://claude.ai/code/session_018yuh89L418oRxz5V2QyyXg
Cross-file link integrity ran os.Stat + filepath.EvalSymlinks
once per link per linting file, re-issuing identical syscalls
across the workspace (Syscall6 ~5.7% flat of check CPU, plan
175 profiling). Add package-level sync.Map caches keyed by the
resolved path — not on the Rule struct, since each parallel
worker holds its own clone and ConfigureRule may re-clone per
file. Caches only the existence boolean / resolved path the
callers consume, so behaviour is unchanged. Process-lifetime,
with the gitignore cache's staleness caveat for a long-lived
LSP. Race-tested under concurrent access.

https://claude.ai/code/session_018yuh89L418oRxz5V2QyyXg
mdtext.CountWords returned len(strings.Fields(text)), allocating
a slice purely to take its length — ~0.48 GB over the 600-file
check gate, called per sentence per paragraph per file. Replace
with a single allocation-free rune scan that is exactly
len(strings.Fields(text)) (maximal non-unicode.IsSpace runs); an
equivalence table test pins it against the original definition
across tab/newline/CRLF/NBSP/ideographic/CJK inputs.

https://claude.ai/code/session_018yuh89L418oRxz5V2QyyXg
Copilot AI review requested due to automatic review settings May 17, 2026 19:02
@codecov

codecov Bot commented May 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.63%. Comparing base (4e63d6a) to head (691fb87).
⚠️ Report is 1 commits behind head on main.

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

☔ View full report in Codecov by Sentry.
📢 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Captures the deferred plan-175 pass-1 item: a safe skip of the
always-on dedupe map+slice (~253 MB over the 600-file gate)
needs a rule.RepoScoped marker applied to an audited set
(git-hook-sync/include/catalog/MDS027 emit cross-file
duplicates), guarded by an equivalence test and a regression
test, not a quick guard.

https://claude.ai/code/session_018yuh89L418oRxz5V2QyyXg
@jeduden
jeduden merged commit 59cfaf9 into main May 17, 2026
21 checks passed
jeduden pushed a commit that referenced this pull request May 17, 2026
Each new function introduced by the parallelization gets a focused
unit test at the base of the pyramid, complementing the existing
integration-level equivalence/verbose tests and the -race checks:

- filterIgnored: ignored-dropped + order-preserved, and the
  no-patterns path (both branches).
- cloneRules: independent instances (distinct pointers, identity
  preserved) and the empty input.
- markdownRulesFrom: no-config-path returns all; with a config path
  only IsConfigFileRule()==true rules are filtered (ConfigTarget
  true/false branches).
- logRulesTo: disabled-logger no-op, and enabled logs only rules
  present-and-enabled in the effective config.
- runFiles: sequential (workers<=1) output is identical to the
  parallel path, index->file mapping stable.
- lintFile: read-error returns errs; happy path returns diags.

filterIgnored/runFiles/cloneRules/logRulesTo/markdownRulesFrom now
100%; lintFile 94.1% (the lint.NewFileFromSource error return is the
codebase-standard, provably-unreachable constructor guard).

Rebased onto main (PR #331 single-core opts); parallelism composes
with the pooled parser + fscache race-clean. Combined 600-file gate
p95 ~238 ms on 4 cores (~3.4x vs the pre-work ~805 ms baseline).

https://claude.ai/code/session_012pboYfnxzyZQLHYE7VS4of
jeduden pushed a commit that referenced this pull request May 17, 2026
Each new function introduced by the parallelization gets a focused
unit test at the base of the pyramid, complementing the existing
integration-level equivalence/verbose tests and the -race checks:

- filterIgnored: ignored-dropped + order-preserved, and the
  no-patterns path (both branches).
- cloneRules: independent instances (distinct pointers, identity
  preserved) and the empty input.
- markdownRulesFrom: no-config-path returns all; with a config path
  only IsConfigFileRule()==true rules are filtered (ConfigTarget
  true/false branches).
- logRulesTo: disabled-logger no-op, and enabled logs only rules
  present-and-enabled in the effective config.
- runFiles: sequential (workers<=1) output is identical to the
  parallel path, index->file mapping stable.
- lintFile: read-error returns errs; happy path returns diags.

filterIgnored/runFiles/cloneRules/logRulesTo/markdownRulesFrom now
100%; lintFile 94.1% (the lint.NewFileFromSource error return is the
codebase-standard, provably-unreachable constructor guard).

Rebased onto main (PR #331 single-core opts); parallelism composes
with the pooled parser + fscache race-clean. Combined 600-file gate
p95 ~238 ms on 4 cores (~3.4x vs the pre-work ~805 ms baseline).

https://claude.ai/code/session_012pboYfnxzyZQLHYE7VS4of
jeduden added a commit that referenced this pull request May 17, 2026
* chore: gitignore Go test binaries and profile output

Profiling the check performance gate (`go test -bench -cpuprofile`)
leaves a `<pkg>.test` binary and `.prof` files in the worktree.
Ignore them so trace/profile runs don't dirty the tree.

https://claude.ai/code/session_012pboYfnxzyZQLHYE7VS4of

* perf: parallelize engine.Runner.Run across files

mdsmith check was single-threaded end to end: Runner.Run looped over
files on one goroutine, so adding cores did nothing (the profiler
showed GOMAXPROCS=1 and =8 within ~13% of each other) even though the
docs advertised core fan-out.

Fan files out across runtime.GOMAXPROCS workers (tunable via
Runner.Concurrency; 1 forces the old sequential path). Per-file work
now returns a fileOutcome instead of mutating the shared Result, and
results are merged in input order before dedupe/sort, so output is
identical to a sequential run regardless of scheduling.

All parallelism blockers addressed:
- shared Result writes: replaced with index-addressed per-file
  outcomes merged after the workers join (no lock on the hot path).
- gitignoreCache lazy-init data race: guarded with a mutex.
- stateful rule singletons (include's visited/chain, the directive
  engines): each worker clones its own rule set via the new
  rule.CloneInstance, an identity-preserving shallow copy. Unlike
  CloneRule, it does not reset a Configurable rule to
  zero+DefaultSettings, so the per-file effective-config Name() lookup
  still resolves.

goldmark's package-level parser singletons are read-only after init
(fresh ParseContext per Parse), so they remain shared safely.

600-file gate corpus, 4-core box: p95 ~805 ms -> ~307 ms (~2.6x);
scales 1->4 cores at ~2.95x where the old code was flat. Output
equivalence and the absence of data races are covered by new tests
run under -race.

https://claude.ai/code/session_012pboYfnxzyZQLHYE7VS4of

* perf: harden parallel lint — concurrent-safe logger, ordered -v, deterministic MDS033

Follow-up to the Runner.Run parallelization, closing the concurrency
sharp edges found in review:

- vlog.Logger.Printf is now mutex-guarded. It was an unsynchronized
  fmt.Fprintf to a shared writer; the new test reproduced dropped and
  torn lines plus a data race when many goroutines log at once
  (production W is os.Stderr so prod was race-free, but the type was a
  footgun for any buffer-backed writer).

- Verbose (-v) output is deterministic again. lintFile logs into a
  per-file buffer; Run flushes the buffers in input order during the
  single-threaded merge, so file/rule lines no longer interleave or
  reorder across workers. Buffers are only allocated when the logger
  is enabled, so the non-verbose hot path is unchanged (gate p95
  steady at ~314 ms on 4 cores).

- MDS033 directory-structure's "no allowed patterns" warning now
  anchors to the per-run RootDir instead of whichever file won the
  package-level sync.Once race, so the sorted diagnostics are
  identical across parallel runs. The emit-once behavior (and its
  test contract) is unchanged.

All touched packages pass `go test -race`; full suite, go vet,
golangci-lint (0 issues) and `mdsmith check .` (307/0) are clean.

https://claude.ai/code/session_012pboYfnxzyZQLHYE7VS4of

* test: cover CloneInstance value-type branch; drop unreachable ResolveWorkers guard

codecov/patch flagged uncovered new lines on PR #330:

- ResolveWorkers had `if w < 1 { w = 1 }`, which is unreachable: the
  n<=0 early return guarantees n>=1, GOMAXPROCS()>=1, and a positive
  concurrency stays positive, so w is always >=1 by that point. Per
  the repo's "no undrivable defensive branch" rule, remove it rather
  than test-stub it.

- CloneInstance's value-type fast path had no test. Add a value-
  receiver rule stub and assert the returned copy keeps identity.

The remaining patch miss is lintFile's parse-error return, which is
the pre-existing (API-required) handling of an error lint.NewFile /
lint.NewFileFromSource never actually returns; with the other misses
gone, patch coverage clears the auto target.

go test -race (engine/rule/log/directorystructure), full suite,
go vet, golangci-lint (0 issues) and mdsmith check (307/0) all clean.

https://claude.ai/code/session_012pboYfnxzyZQLHYE7VS4of

* test: dedicated unit tests for parallel-runner helpers (test pyramid)

Each new function introduced by the parallelization gets a focused
unit test at the base of the pyramid, complementing the existing
integration-level equivalence/verbose tests and the -race checks:

- filterIgnored: ignored-dropped + order-preserved, and the
  no-patterns path (both branches).
- cloneRules: independent instances (distinct pointers, identity
  preserved) and the empty input.
- markdownRulesFrom: no-config-path returns all; with a config path
  only IsConfigFileRule()==true rules are filtered (ConfigTarget
  true/false branches).
- logRulesTo: disabled-logger no-op, and enabled logs only rules
  present-and-enabled in the effective config.
- runFiles: sequential (workers<=1) output is identical to the
  parallel path, index->file mapping stable.
- lintFile: read-error returns errs; happy path returns diags.

filterIgnored/runFiles/cloneRules/logRulesTo/markdownRulesFrom now
100%; lintFile 94.1% (the lint.NewFileFromSource error return is the
codebase-standard, provably-unreachable constructor guard).

Rebased onto main (PR #331 single-core opts); parallelism composes
with the pooled parser + fscache race-clean. Combined 600-file gate
p95 ~238 ms on 4 cores (~3.4x vs the pre-work ~805 ms baseline).

https://claude.ai/code/session_012pboYfnxzyZQLHYE7VS4of

* plan: automate cross-tool benchmark on merge to main (#184)

Design for folding run.sh into a pinned, integrity-verified
`mdsmith-release bench` subcommand, running it on merge to main, and
publishing refreshed JSON+fragments to the orphan assets branch (the
demo.gif pattern) so the website serves current numbers. Decisions
locked from the four design forks; slices and the CI-only caveat
recorded. Lands on PR #330.

https://claude.ai/code/session_012pboYfnxzyZQLHYE7VS4of

* docs: shorten plan 184 Goal prose to satisfy MDS023

The Goal paragraph tripped the readability gate (index 15.3 > 14.0).
Split into short declarative sentences; refresh the PLAN.md catalog.

https://claude.ai/code/session_012pboYfnxzyZQLHYE7VS4of

---------

Co-authored-by: Claude <noreply@anthropic.com>
jeduden added a commit that referenced this pull request May 27, 2026
The substantive work landed via #328 (CI gate, benchmarks,
profiler, fragment-driven docs) and #331 (Pass-1 optimisations,
−38% latency). Task 11 (cheap-win loop on mdsmith-parity) is
out of cheap wins per its own Pass-2 verdict; the remaining
multiplexed-walk lever is tracked as separate scoped work. The
"CI check-bench / bench-fragments pass on this branch"
acceptance line is satisfied by the merged PRs' own CI runs.

https://claude.ai/code/session_01UDqPB3Jm2y3htZiyZT1ff4

Co-authored-by: Claude <noreply@anthropic.com>
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.

3 participants