-
Notifications
You must be signed in to change notification settings - Fork 659
refactor(rome_cli): refactor the threading of parallel traversal to increase occupancy #3577
Conversation
…ncrease occupancy
✅ Deploy Preview for docs-rometools canceled.
|
@MichaReiser Hopefully this should improve the wall time benchmarks a bit for the multi-threaded case |
duration = Some(traverse_inputs( | ||
let duration = thread::scope(|s| { | ||
thread::Builder::new() | ||
.name(String::from("rome::console")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
static INIT_ONCE: Once = Once::new(); | ||
INIT_ONCE.call_once(|| { | ||
rayon::ThreadPoolBuilder::new() | ||
.thread_name(|index| format!("rome::worker_{index}")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make sure to respect the RAYON_NUM_THREADS
env variable? Or I have to find a new way to limit the threads in my benchmarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the .num_threads()
method is never called on the ThreadPoolBuilder
it the size of the thread pool should be the default value of either the RAYON_NUM_THREADS
variable or num_cpus
* upstream/main: fix(ci): fix the release workflows for the stable release (rome#3583) Fix logo container margin Fix mobile docs regressions perf: End-to-end Linter and Formatter benchmarks (rome#3570) doc: VS Code extension (rome#3579) refactor(rome_cli): refactor the threading of parallel traversal to increase occupancy (rome#3577) [docs] Add navigation dropdown for docs (rome#3578) doc(rome_cli): Document `--files-max-size` option perf(rome_js_semantic): Use FX Hash function (rome#3565) fix(rome_js_analyzer): `noInvalidConstructorSuper` false positive for class expressions (rome#3561) Clean up mobile navigation doc(website): Run `cargo lintdoc` (rome#3567) doc: Fix install command Fix mobile code blocks Fix dark mode logo Update links Implement new website (rome#3556)
Summary
This PR changes how threads are spawned during parallel traversal operations: it merges the console and report tasks into a single dedicated thread running outside of the Rayon thread pool, so that all threads in the pool are now available to process files.
I've also made the initial setup of input tasks run on the main thread (to avoid allocating an extraneous thread for this), and replaced some redundant cloning operations of the traversal state with shared references (this is made possible by using the scoped thread API of the standard library from Rust 1.63)
Test Plan
This is purely an internal change, it should have no noticeable side effect other than possibly improving traversal performance on multi-core systems, and the existing test suite of the CLI should ensure this is actually the case.