perf(core): Reduce worker thread contention for faster pipeline execution#1409
perf(core): Reduce worker thread contention for faster pipeline execution#1409
Conversation
…tion Add maxWorkerThreads option to WorkerOptions for explicit thread count capping, then use it to reduce CPU contention when metrics and security worker pools run concurrently during the pipeline overlap phase. - Metrics pool: capped at (processConcurrency - 1) - Security pool: capped at floor(processConcurrency / 2) On a 4-core machine this reduces concurrent threads from 8 (4+4) to 5 (3+2), avoiding context-switching overhead during gpt-tokenizer warmup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
⚡ Performance Benchmark
Details
Historyf49a5d0 fix(core): Add early return for empty input and restore numOfTasks comment
46a0a7b perf(core): Remove metrics worker cap to avoid regression on low-core machines
aff7aa9 refactor(core): Cap security workers at 2 instead of floor(cores/2)
2a87942 perf(core): Reduce worker thread contention for faster pipeline execution
|
|
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:
📝 WalkthroughWalkthroughThe PR introduces a worker thread concurrency capping mechanism by adding an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Metrics/<br/>Security Module
participant PC as ProcessConcurrency<br/>Module
participant TR as TaskRunner/<br/>initTaskRunner
participant WP as createWorkerPool
participant WT as getWorkerThreadCount
Caller->>PC: getProcessConcurrency()
PC-->>Caller: CPU count
Caller->>Caller: compute max cap<br/>(e.g., CPUs - 1)
Caller->>TR: initTaskRunner({<br/>maxWorkerThreads: cap<br/>})
TR->>WP: pass maxWorkerThreads
WP->>WT: getWorkerThreadCount(<br/>numOfTasks,<br/>maxWorkerThreads)
WT->>WT: effectiveConcurrency =<br/>min(detected, cap)
WT->>WT: maxThreads =<br/>min(effectiveConcurrency,<br/>task-derived limit)
WT-->>WP: { minThreads, maxThreads }
WP->>WP: configure tinypool<br/>with capped maxThreads
WP-->>TR: WorkerPool instance
TR-->>Caller: TaskRunner ready
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1409 +/- ##
==========================================
+ Coverage 87.40% 87.42% +0.01%
==========================================
Files 116 116
Lines 4392 4397 +5
Branches 1018 1020 +2
==========================================
+ Hits 3839 3844 +5
Misses 553 553 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Deploying repomix with
|
| Latest commit: |
7de6923
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3af6c4d1.repomix.pages.dev |
| Branch Preview URL: | https://perf-reduce-worker-thread-co.repomix.pages.dev |
![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/security/securityCheck.ts (1)
74-87: Skip worker pool initialization when there are no items.A fast return when
totalItems === 0avoids unnecessary worker setup/teardown on empty inputs.♻️ Suggested change
const allItems = [...fileItems, ...gitDiffItems, ...gitLogItems]; const totalItems = allItems.length; + + if (totalItems === 0) { + return []; + } // Cap security workers at half the available CPU cores to reduce contention with the // metrics worker pool that runs concurrently. The security check uses coarse-grained // batches (BATCH_SIZE=50), so fewer workers still provide sufficient parallelism. const maxSecurityWorkers = Math.max(1, Math.floor(getProcessConcurrency() / 2));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/security/securityCheck.ts` around lines 74 - 87, Check totalItems (computed from allItems) before initializing the worker pool and return early when totalItems === 0 to avoid unnecessary init/teardown; specifically, add a fast-path that when totalItems is 0 skips calling deps.initTaskRunner (the current code that creates taskRunner using maxSecurityWorkers and workerType 'securityCheck') and returns the appropriate empty result (or resolves the surrounding function) instead of initializing the worker pool and continuing.
🤖 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/security/securityCheck.ts`:
- Around line 74-87: Check totalItems (computed from allItems) before
initializing the worker pool and return early when totalItems === 0 to avoid
unnecessary init/teardown; specifically, add a fast-path that when totalItems is
0 skips calling deps.initTaskRunner (the current code that creates taskRunner
using maxSecurityWorkers and workerType 'securityCheck') and returns the
appropriate empty result (or resolves the surrounding function) instead of
initializing the worker pool and continuing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 26dd26d4-3910-464e-b0e7-d226ad223484
📒 Files selected for processing (5)
src/core/metrics/calculateMetrics.tssrc/core/security/securityCheck.tssrc/shared/processConcurrency.tstests/core/security/securityCheck.test.tstests/shared/processConcurrency.test.ts
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to limit the number of worker threads for metrics calculation and security checks to reduce CPU contention during concurrent execution. It updates the getWorkerThreadCount utility to support an optional thread cap and adjusts the task runners in calculateMetrics.ts and securityCheck.ts accordingly. Unit tests have been added to verify the capping logic. I have no feedback to provide.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Security check uses coarse-grained batches (50 files/batch), so 2 workers provide sufficient parallelism even for large repos. Fixing the cap at 2 avoids over-allocation on high-core machines (e.g., 8 threads on 16 cores) while keeping the formula simple and predictable. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Claude Code Review — PR #1409Overall: Clean, well-motivated PR with solid benchmarks on Ubuntu/Windows. The Noteworthy Findings1. macOS benchmark regression (+7.7%) warrants investigation The benchmark shows Ubuntu -9.6% and Windows -7.2% improvements, but macOS regressed from 0.87s to 0.94s. On a 2-core macOS CI runner, 2. Removed comment about The original code had an important comment explaining why // numOfTasks uses totalItems (not batches.length) to avoid under-sizing the pool.3.
Additional observations (non-blocking)
Bot Comment EvaluationNo AI bot inline comments required priority judgment — CodeRabbit's review was skipped (auto-incremental disabled), and Gemini/Devin found no issues. CodeRabbit did leave a nitpick suggesting an early return for Reviewed with Claude Code |
… machines CI benchmarks showed macOS (3 vCPU) regressed by ~6-7% with the processConcurrency-1 cap, while Ubuntu/Windows (4 vCPU) improved. On 3 cores, capping metrics from 3→2 workers is a 33% parallelism reduction that outweighs contention savings. The main improvement comes from capping security at 2 workers, which is sufficient since the security pool uses coarse batches (50 files) and finishes quickly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…mment Address PR review feedback: - Add early return when totalItems === 0 to skip worker pool init - Restore comment explaining why numOfTasks uses totalItems not batches.length Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Thanks for the thorough review! 1. macOS regression — Addressed in 46a0a7b by removing the metrics worker cap ( 2. Removed 3. Also added early return for |
Move getProcessConcurrency from a direct module import to the deps parameter for consistency with initTaskRunner. This makes it easier to test with different concurrency values without module-level mocking. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Addressed the remaining feedback:
|
1 participant
Add
maxWorkerThreadsoption toWorkerOptionsfor explicit thread count capping, then use it to reduce CPU contention when metrics and security worker pools run concurrently during the pipeline overlap phase.Changes
processConcurrency.ts: Add optionalmaxWorkerThreadstoWorkerOptions.getWorkerThreadCountandcreateWorkerPoolrespect this cap when provided.calculateMetrics.ts: Cap metrics workers atprocessConcurrency - 1increateMetricsTaskRunner. Warmup thread count now matches the actual pool size.securityCheck.ts: Cap security workers atfloor(processConcurrency / 2). Security check uses coarse-grained batches (50 files), so fewer workers still provide sufficient parallelism.Rationale
During the pipeline overlap phase, the metrics pool (gpt-tokenizer warmup) and security pool run concurrently. On a 4-core machine, this previously created 8 threads (4+4) competing for 4 cores, causing significant context-switching overhead. With the caps, total concurrent threads drop to 5 (3+2), reducing contention while maintaining throughput.
The implementation uses an explicit
maxWorkerThreadsparameter rather than manipulatingnumOfTasks, keeping the API clean and ensuring warmup thread count stays in sync with the actual pool size.Checklist
npm run testnpm run lint