refactor(linter): always explicitly initialize Rayon thread pool#13122
Conversation
github-actions
bot
added
A-linter
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Rayon thread pool initialization to ensure thread count consistency across the linter's execution. The change guarantees that rayon::current_num_threads() will always return the same value, which is critical for future multi-threaded JS plugin support.
Key changes:
- Always explicitly initialize the Rayon thread pool, even when using default thread count
- Add fallback handling when CPU core count detection fails with user notification
- Move thread pool initialization logic from conditional to unconditional execution
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/oxlint/src/command/lint.rs | Refactored thread pool initialization to always run with explicit thread count and added error handling for CPU detection failure |
| crates/oxc_linter/src/service/runtime.rs | Added unconditional thread pool initialization as a safety measure in the Runtime constructor |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
CodSpeed Instrumentation Performance ReportMerging #13122 will not alter performanceComparing Summary
Footnotes |
Similar to #13122. Unconditionally initialize Rayon global thread pool in coverage runner. The guarantees provided by explicitly initializing the thread pool are going to be critical for the linter. Coverage currently doesn't cover linter, so it doesn't matter, but this doesn't hurt to do, and gives us greater safety if coverage does involve the linter at a later date.
overlookmotel
force-pushed
the
08-15-refactor_linter_always_explicitly_initialize_rayon_thread_pool
branch
from
2257eb4 to
7ddeb68
Compare
|
The warning that could not determine how many available threads is printed with @camc314 Is this OK? Or should logging be going through some reporter abstraction? |
|
Discussed with Boshen offline. He thinks this PR is fine. Going to merge it now as I have more to stack on top, and Cam can check it when he's next at his desk. |
Merge activity
|
) Previously we only explicitly initialized the global Rayon thread pool if `--threads` option is specified. Instead, do it unconditionally, and always specify thread count. The purpose is to obtain a guarantee that `rayon::current_num_threads()` will always return the same number during the rest of the linter's work. This invariant is not relied upon currently, but will become critical for soundness once we run JS plugins on multiple threads. Also, if `std::thread::available_parallelism()` is not able to obtain CPU core count, log a warning that the linter will run single-threaded. Previously Rayon would use only a single thread, but would make that decision silently. I'm not sure in what circumstances `available_parallelism()` can return `Err`, but if it does, the user will probably want to know about it.
graphite-app
bot
force-pushed
the
08-15-refactor_linter_always_explicitly_initialize_rayon_thread_pool
branch
from
7ddeb68 to
9f924f6
Compare
graphite-app
bot
deleted the
08-15-refactor_linter_always_explicitly_initialize_rayon_thread_pool
branch
| } else if let Ok(thread_count) = std::thread::available_parallelism() { | ||
| thread_count.get() | ||
| } else { | ||
| eprintln!( | ||
| "Unable to determine available thread count. Defaulting to 1.\nConsider specifying the number of threads explicitly with `--threads` option." | ||
| ); | ||
| 1 | ||
| }; |
There was a problem hiding this comment.
this is going to be such a cold path, i think it's fine. for ref, rayon defaults to 1x thread if it can't call std::thread::available_parrellism
let default = || {
thread::available_parallelism()
.map(|n| n.get())
.unwrap_or(1)
};
There was a problem hiding this comment.
Thanks for reviewing. Yes, that's the point of logging a warning. Rayon would default to 1 if it can't determine available parallelism, so we're not changing that behavior. But what we are changing is to warn the user that this is happening, as it's probably not what they want.
I have no idea in what circumstances std::thread::available_parallelism would return Err anyway. Maybe it's possible in some VMs? 🤷
There was a problem hiding this comment.
/// This function will, but is not limited to, return errors in the following
/// cases:
///
/// - If the amount of parallelism is not known for the target platform.
/// - If the program lacks permission to query the amount of parallelism made
/// available to it.
stupid edge cases i think.
stderr is the right solution here, as it means it won't break people using stdout and parsing the json ect
|
LGTM 👍 |
Previously we only explicitly initialized the global Rayon thread pool if
--threadsoption is specified.Instead, do it unconditionally, and always specify thread count. The purpose is to obtain a guarantee that
rayon::current_num_threads()will always return the same number during the rest of the linter's work.This invariant is not relied upon currently, but will become critical for soundness once we run JS plugins on multiple threads.
Also, if
std::thread::available_parallelism()is not able to obtain CPU core count, log a warning that the linter will run single-threaded. Previously Rayon would use only a single thread, but would make that decision silently. I'm not sure in what circumstancesavailable_parallelism()can returnErr, but if it does, the user will probably want to know about it.