perf(core): Prefetch git sort data to overlap with file search and collection#1467
perf(core): Prefetch git sort data to overlap with file search and collection#1467
Conversation
…llection Launch git --version + git log --name-only subprocesses early in the pipeline so that sortOutputFiles hits the in-memory cache instead of spawning ~15ms of subprocess overhead on the critical path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
⚡ Performance Benchmark
Details
Historyd6333ae perf(core): Prefetch git sort data to overlap with file search and collection
|
|
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:
📝 WalkthroughWalkthroughThis PR introduces a prefetch mechanism for file change count data used in sorting. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 #1467 +/- ##
=======================================
Coverage 87.18% 87.18%
=======================================
Files 117 117
Lines 4465 4473 +8
Branches 1032 1034 +2
=======================================
+ Hits 3893 3900 +7
- Misses 572 573 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code ReviewOverall: Looks good ✅ — Clean, minimal, and well-reasoned performance optimization. The fire-and-forget prefetch pattern is correct, and the change is architecturally consistent with the existing codebase. Feedback1. Silent error swallowing — consider adding a trace log (minor)const sortDataPromise = deps.prefetchSortData(config).catch(() => {});Since const sortDataPromise = deps.prefetchSortData(config).catch((error) => {
logger.trace('Prefetch sort data failed:', error);
});2. No unit tests for `prefetchSortData` (non-blocking)The new exported function has no test coverage in
3. `packager.ts` reaches ~263 lines, slightly over the 250-line guideline (pre-existing)This PR adds ~6 lines to an already borderline file. Not a blocker for this PR, but worth noting for future cleanup. No security concerns, no race conditions with the shared cache, and the concurrency approach correctly overlaps git subprocess I/O with file search/collection. 🤖 Generated with Claude Code |
Deploying repomix with
|
| Latest commit: |
f4a318c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://caa85cf3.repomix.pages.dev |
| Branch Preview URL: | https://perf-prefetch-sort-data.repomix.pages.dev |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/packager.ts (1)
81-84: Don’t suppress prefetch failures silently.Line 83 hides all failures; add a trace log so prefetch regressions remain observable without affecting runtime behavior.
Patch suggestion
import { logMemoryUsage, withMemoryLogging } from '../shared/memoryUtils.js'; +import { logger } from '../shared/logger.js'; @@ - const sortDataPromise = deps.prefetchSortData(config).catch(() => {}); + const sortDataPromise = deps.prefetchSortData(config).catch((error) => { + logger.trace('Failed to prefetch sort data; falling back to on-demand git fetch.', error); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/packager.ts` around lines 81 - 84, The current prefetch call assigns sortDataPromise = deps.prefetchSortData(config).catch(() => {}) which swallows all errors; change the catch to log the error at trace level while preserving the swallow (e.g., .catch(err => { logger.trace?.('deps.prefetchSortData failed', err); })) so regressions remain observable; locate the call to deps.prefetchSortData and replace the empty catch with a trace-level log using your module's logger (or console.trace if no logger is available) referencing sortDataPromise and config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/packager.ts`:
- Around line 81-84: The current prefetch call assigns sortDataPromise =
deps.prefetchSortData(config).catch(() => {}) which swallows all errors; change
the catch to log the error at trace level while preserving the swallow (e.g.,
.catch(err => { logger.trace?.('deps.prefetchSortData failed', err); })) so
regressions remain observable; locate the call to deps.prefetchSortData and
replace the empty catch with a trace-level log using your module's logger (or
console.trace if no logger is available) referencing sortDataPromise and config.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f8d7d9de-2f95-42ab-82f4-1cb0c0d98955
📒 Files selected for processing (2)
src/core/output/outputSort.tssrc/core/packager.ts
![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.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Code Review (Update)Overall: Looks good ✅ — Ready to merge. This is a clean, focused performance optimization with correct concurrency handling. What changed since last reviewThe trace logging suggestion from the previous review has been addressed in commit Reviewer consensusAll six review dimensions checked out:
Remaining feedback1. No direct unit tests for
|
1 participant
Summary
prefetchSortData()to launchgit --version+git log --name-onlysubprocesses early in the pipeline, so thatsortOutputFileshits the in-memory cache instead of spawning ~15ms of subprocess overhead on the critical pathsearchFilesandcollectFiles, hiding the git subprocess costTest plan
npm run lintpassesnpm run test— 1115/1115 tests passnode --run benchshows improvement compared to main🤖 Generated with Claude Code