perf(core): Skip unnecessary content scans in createRenderContext#1398
perf(core): Skip unnecessary content scans in createRenderContext#1398
Conversation
Skip two expensive full-content scans in createRenderContext when their results are unused by the current output format: - calculateFileLineCounts: scans all file contents to count newlines. Not referenced by any Handlebars template (XML, markdown, plain) or parsable output generator (XML, JSON). Only used by the skill path, so skip for regular output entirely. - calculateMarkdownDelimiter: scans all file contents for backtick sequences. Only used by the markdown template and skill generators. Skip for XML, plain, JSON, and parsable-XML output styles. Also optimized the implementations for when they do run: - calculateMarkdownDelimiter: replaced flatMap + intermediate array allocation with single-pass inline max tracking. - calculateFileLineCounts: replaced regex-based newline counting (which allocated arrays of all matches) with indexOf-based loop. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
⚡ Performance Benchmark
Details
History67c06ff refactor(core): Use character scan instead of regex in calculateMarkdownDelimiter
f890c50 perf(core): Skip unnecessary content scans in createRenderContext
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRefactored markdown delimiter calculation using single-pass regex and introduced conditional computation in render context generation. Added Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1398 +/- ##
==========================================
+ Coverage 87.40% 87.42% +0.01%
==========================================
Files 116 116
Lines 4392 4414 +22
Branches 1018 1024 +6
==========================================
+ Hits 3839 3859 +20
- Misses 553 555 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying repomix with
|
| Latest commit: |
940e1b0
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://88b467dd.repomix.pages.dev |
| Branch Preview URL: | https://perf-skip-unnecessary-conten.repomix.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Code Review - perf(core): Skip unnecessary content scans in createRenderContextOverall: Looks good. This is a clean, well-scoped performance optimization. The skip logic is correct for all current output paths, the algorithm rewrites are sound, and the PR description accurately reflects the changes. A few suggestions below. 1. Pre-existing bug exposed: fileLineCounts 0 treated as missing in skillStatistics.ts - In skillStatistics.ts, the fallback uses || which treats 0 as falsy. calculateFileLineCounts correctly returns 0 for empty files, but the || fallback overrides it with 1. This is pre-existing, not introduced by this PR, but now forceAll: true makes the 0 return value load-bearing. Consider changing || to ?? in a follow-up. 2. Add a comment near needsMarkdownDelimiter about future output styles - The condition hard-codes that only markdown needs the delimiter. If a new output style is added that uses markdownCodeBlockDelimiter, this condition must be updated. A brief comment pointing to the generateOutput switch would prevent a future oversight. 3. Consider adding tests for the skip logic and rewritten helpers - The new conditional paths and rewritten algorithms have no dedicated test coverage. The forceAll: true path (the only one calling the rewritten calculateFileLineCounts) is fully mocked in packSkill.test.ts, so the real countNewlines is never exercised by any test. 4. Minor: restore the removed comment about trailing-newline edge case - The original calculateFileLineCounts had a useful comment explaining the trailing-newline logic. The PR removes it, but the behavior is non-obvious. A brief comment would help future readers. No Security Issues - The markdown fence escape mechanism is never bypassed for any output path that renders code fences. The fallback triple-backtick for non-markdown styles is safe since those templates never reference the delimiter. Performance Assessment - The optimizations are meaningful: skipping calculateFileLineCounts saves O(total-bytes), skipping calculateMarkdownDelimiter saves a full regex pass for XML/JSON/plain, indexOf-based countNewlines reduces GC pressure, and the single-pass calculateMarkdownDelimiter eliminates flatMap intermediate array allocations. Reviewed with Claude |
…ownDelimiter Replace RegExp.exec loop with a simple character-by-character scan to avoid regex object and match object allocations entirely. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code Review (follow-up) — perf(core): Skip unnecessary content scans in createRenderContextOverall: Approve. Clean, well-scoped performance optimization. The skip logic is correct for all current output paths. Adding a few observations beyond the prior review. New findings1.
2. The 3. DetailsThe empty object 4. File size note Details
Previously noted (still relevant)The prior review's points about restoring the trailing-newline comment and the VerdictThe optimization is sound, the security implications are nil, and the algorithmic rewrites are correct. The benchmark data confirms real improvements. The suggestions above are all non-blocking improvements that could be addressed in follow-ups. Reviewed with Claude |
|
Closing — benchmark shows no measurable improvement (within noise). The added complexity (forceAll option, conditional logic) isn't justified for this level of gain. |
1 participant
Skip two expensive full-content scans in
createRenderContextwhen their results are unused by the current output format:calculateFileLineCounts: scans all file contents to count newlines. Not referenced by any Handlebars template (XML, markdown, plain) or parsable output generator (XML, JSON). Only used by the skill path, so skip for regular output entirely.calculateMarkdownDelimiter: scans all file contents for backtick sequences. Only used by the markdown template and skill generators. Skip for XML, plain, JSON, and parsable-XML output styles.Also optimized the implementations for when they do run:
calculateMarkdownDelimiter: replacedflatMap+ intermediate array allocation with single-pass inline max tracking.calculateFileLineCounts: replaced regex-based newline counting (which allocated arrays of all matches) withindexOf-based loop.Extracted from #1377. This is a pure performance optimization with no functional changes — output is identical for all formats.
Checklist
npm run testnpm run lint