Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
![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 an automatic subprocess job control feature for the ptrace backend, allowing dynamic control of subprocess parallelism based on system resource pressure (CPU and memory stalls via Linux PSI, with fallbacks). It adds a new --job-control auto CLI option, a corresponding configuration parameter, and the core AutoJobController logic along with extensive tests. A critical race condition was identified in the thread registration logic of the job control wakeup state, which could cause the wakeup thread to block indefinitely if a job is queued before registration completes. A code suggestion has been provided to resolve this issue.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| pub(super) fn register_worker(&self, worker: Thread) { | ||
| assert!( | ||
| self.worker.set(worker).is_ok(), | ||
| "job-control wakeup worker was registered twice" | ||
| ); | ||
| } |
There was a problem hiding this comment.
There is a race condition between the tracer thread registering the worker thread and the worker thread parking itself.
- The
jc-wakeupthread is spawned and starts running. It checkshas_waiting_jobs(), which isfalse, so it prepares to park. - Before the tracer thread calls
register_worker, a job is paused, callingset_waiting_jobs(true). set_waiting_jobs(true)sees thatself.worker.get()isNone(sinceregister_workerhasn't run yet), so it does not callunpark().- The tracer thread then calls
register_worker(thread). - The
jc-wakeupthread callsstd::thread::park()and blocks forever, because theunpark()signal was lost.
To fix this, register_worker should check if there are already waiting jobs, and if so, immediately unpark the worker.
| pub(super) fn register_worker(&self, worker: Thread) { | |
| assert!( | |
| self.worker.set(worker).is_ok(), | |
| "job-control wakeup worker was registered twice" | |
| ); | |
| } | |
| pub(super) fn register_worker(&self, worker: Thread) { | |
| assert!( | |
| self.worker.set(worker).is_ok(), | |
| "job-control wakeup worker was registered twice" | |
| ); | |
| if self.has_waiting_jobs() { | |
| if let Some(worker) = self.worker.get() { | |
| worker.unpark(); | |
| } | |
| } | |
| } |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #298 +/- ##
==========================================
+ Coverage 82.25% 82.61% +0.35%
==========================================
Files 84 85 +1
Lines 21026 21873 +847
==========================================
+ Hits 17295 18070 +775
- Misses 3731 3803 +72 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
PoC for #32.
tracexec can be used to control parallelism of sub-processes to help avoiding pressure stalls or even OOM.
I think I will become willing to merge this if it could exceed the native performance of
make -j$(nproc)in a kernel build.Currently it is slightly slower than it.