perf(security): Reduce security worker threads to minimize CPU contention#1369
perf(security): Reduce security worker threads to minimize CPU contention#1369
Conversation
|
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 change introduces configurable worker thread limits for task runners. The 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 |
⚡ Performance Benchmark
Details
History1da8bc5 perf(security): Reduce security worker threads to minimize CPU contention
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1369 +/- ##
=======================================
Coverage 86.96% 86.96%
=======================================
Files 116 116
Lines 4425 4426 +1
Branches 1025 1026 +1
=======================================
+ Hits 3848 3849 +1
Misses 577 577 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 maxThreads override for worker pools, specifically capping the security check worker pool to one thread to minimize CPU contention. The changes include updates to the WorkerOptions interface, logic in createWorkerPool to handle the override with a minimum value of 1, and new unit tests. Feedback suggests adjusting the minThreads calculation to ensure it never exceeds the maxThreads override, preventing potential errors in the Tinypool library.
| const { minThreads, maxThreads: autoMaxThreads } = getWorkerThreadCount(numOfTasks); | ||
| const maxThreads = maxThreadsOverride != null ? Math.max(1, maxThreadsOverride) : autoMaxThreads; |
There was a problem hiding this comment.
When maxThreadsOverride is used to set a low thread count (e.g., 1), there is a risk that minThreads (returned by getWorkerThreadCount) could exceed it, which would cause Tinypool to throw an error (as it requires minThreads <= maxThreads). Although minThreads is currently hardcoded to 1 in getWorkerThreadCount, ensuring this relationship makes the code more robust against future changes in the thread calculation logic.
| const { minThreads, maxThreads: autoMaxThreads } = getWorkerThreadCount(numOfTasks); | |
| const maxThreads = maxThreadsOverride != null ? Math.max(1, maxThreadsOverride) : autoMaxThreads; | |
| const { minThreads: autoMinThreads, maxThreads: autoMaxThreads } = getWorkerThreadCount(numOfTasks); | |
| const maxThreads = maxThreadsOverride != null ? Math.max(1, maxThreadsOverride) : autoMaxThreads; | |
| const minThreads = Math.min(autoMinThreads, maxThreads); |
Deploying repomix with
|
| Latest commit: |
1da63be
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://67429cb4.repomix.pages.dev |
| Branch Preview URL: | https://perf-reduce-security-worker.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)
tests/shared/processConcurrency.test.ts (1)
133-152: Good test coverage for the newmaxThreadsoption.The tests correctly verify:
- Override is respected when
maxThreads: 1is provided- Values below 1 are clamped (tested with
maxThreads: 0)Consider adding a test for negative values (e.g.,
maxThreads: -1) to ensure the clamping logic handles all edge cases, thoughMath.max(1, ...)should handle this correctly.🧪 Optional: Add test for negative maxThreads
+ it('should clamp negative maxThreads override to minimum of 1', () => { + createWorkerPool({ numOfTasks: 500, workerType: 'securityCheck', runtime: 'worker_threads', maxThreads: -5 }); + + expect(Tinypool).toHaveBeenCalledWith( + expect.objectContaining({ + maxThreads: 1, + }), + ); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/shared/processConcurrency.test.ts` around lines 133 - 152, Add a test case in the same suite that passes createWorkerPool({ numOfTasks: 500, workerType: 'securityCheck', runtime: 'worker_threads', maxThreads: -1 }) and assert that Tinypool was called with expect.objectContaining({ maxThreads: 1 }) to verify negative values are clamped; place it alongside the existing "should clamp maxThreads override to minimum of 1" test and reference the createWorkerPool helper and Tinypool mock.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/shared/processConcurrency.test.ts`:
- Around line 133-152: Add a test case in the same suite that passes
createWorkerPool({ numOfTasks: 500, workerType: 'securityCheck', runtime:
'worker_threads', maxThreads: -1 }) and assert that Tinypool was called with
expect.objectContaining({ maxThreads: 1 }) to verify negative values are
clamped; place it alongside the existing "should clamp maxThreads override to
minimum of 1" test and reference the createWorkerPool helper and Tinypool mock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d6ded7c0-7c5b-4758-9b30-4b53d3a32a5c
📒 Files selected for processing (3)
src/core/security/securityCheck.tssrc/shared/processConcurrency.tstests/shared/processConcurrency.test.ts
|
Closing: benchmark shows +71ms regression on main. The original optimization (ff96545) was measured on a branch where security overlapped with metrics workers, causing CPU contention. On main, security and metrics run sequentially, so reducing security threads just slows the security phase with no contention benefit. |
PR ReviewClean, well-scoped PR with good test coverage. A few observations: 1. Pipeline description is inaccurateThe PR description states "Security check runs concurrently with file processing" — but looking at The actual concurrency is between the security worker pool and the metrics warmup (pre-initialized at line 97-98), which is a single WASM-loading task on one thread. The contention story may still hold, but the description should be corrected for accuracy. 2. Defensive
|
…tion Add maxThreads option to WorkerOptions for explicit thread count control, and cap security check worker pool to 1 thread. Security runs concurrently with file processing and completes well before the metrics calculation phase. On a 4-core machine, the previous config spawned up to 4 security threads that competed for CPU cores with the metrics workers, inflating the metrics phase by ~140ms. With 1 thread, security still finishes before metrics while eliminating contention. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
yamadashy
force-pushed
the
perf/reduce-security-worker-threads
branch
from
1da8bc5 to
1da63be
Compare
PR Review (Updated)This PR is being closed per @yamadashy's comment — benchmark showed +71ms regression. Here's a final summary for the record. Why the approach didn't workThe PR description assumed security and metrics workers compete for CPU cores. However, looking at Code quality assessmentThe implementation itself was clean and well-designed:
One latent issue worth notingIf No further action needed given the closure. 🤖 Generated with Claude Code |
|
Closing: the optimization is specific to CPU-constrained environments (4 cores). On machines with 8+ cores, security and metrics workers run without contention, and reducing security threads purely slows the security phase (+54ms on a high-core-count machine). Not a universal improvement. |
1 participant
Summary
maxThreadsoption toWorkerOptionsinterface, allowing callers to explicitly cap worker thread countBackground
Security check runs concurrently with file processing in the pipeline and completes well before metrics calculation begins. Previously, the security worker pool spawned threads based on task count (up to CPU core count), competing for CPU cores with the later metrics workers. On a 4-core machine, 4 security + 4 metrics threads caused ~140ms of CPU contention that inflated the metrics phase.
With 1 security thread, security still finishes before metrics while eliminating contention entirely.
Design
Rather than indirectly controlling thread count by capping
numOfTasks(which depends on the internalTASKS_PER_THREADconstant), this PR adds an explicitmaxThreadsoverride toWorkerOptions. This makes the intent clear and is resilient to future changes in the thread calculation logic.Checklist
npm run testnpm run lint