Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLI argument for setting max number of viewer threads #4931

Closed
nikolausWest opened this issue Jan 29, 2024 · 4 comments · Fixed by #5021
Closed

CLI argument for setting max number of viewer threads #4931

nikolausWest opened this issue Jan 29, 2024 · 4 comments · Fixed by #5021
Assignees
Labels
enhancement New feature or request 📉 performance Optimization, memory use, etc 🏎️ Quick Issue Can be fixed in a few hours or less 📺 re_viewer affects re_viewer itself
Milestone

Comments

@nikolausWest
Copy link
Member

Since we started parallelizing the viewer, it can end up hogging too much CPU such that it interferes with other running processes.

Add a CLI argument that sets the max number of threads used by the viewer.

@nikolausWest nikolausWest added enhancement New feature or request 📺 re_viewer affects re_viewer itself 📉 performance Optimization, memory use, etc 🏎️ Quick Issue Can be fixed in a few hours or less labels Jan 29, 2024
@nikolausWest nikolausWest added this to the 0.13 milestone Jan 29, 2024
@emilk
Copy link
Member

emilk commented Jan 29, 2024

This will also be useful when profiling, because it is hard to get an overview of 16 threads in puffin.

We should perhaps default to max(2, num_cores - 2) rayon worker threads, saving one core for the main thread, and one core for the rest of the system.

So perhaps the default of -j is -2 where non-positive numbers means "num cored minus this".

@Wumpf
Copy link
Member

Wumpf commented Jan 29, 2024

RAYON_NUM_THREADS seems to be picked up since we're using the default rayon configuration

@nikolausWest
Copy link
Member Author

I still think we should expose it as a CLI argument (and ideally an argument to rr.spawn as well) for the purpose of making it easy to find for our users. Users shouldn't need to know we use Rayon

@emilk
Copy link
Member

emilk commented Feb 1, 2024

This might be a good time for a re_parallel crate that handles the rayon initialization, so it is only initialized once.

Right now I think we don't initialize the thread pool explicitly when called from Python (meaning rayon threads are unnamed)

emilk added a commit that referenced this issue Feb 5, 2024
### What
* Closes #4931

You can now control the number of threads in the rayon thread pool using
`-j` or `--threads`. The default is `-2`, meaning two less threads than
the number of cores. This is to leave some breathing room for other
threads the viewer spawns, as well as the rest of the users system.

### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/5021/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/5021/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/5021/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/5021)
- [Docs
preview](https://rerun.io/preview/e2052e86d1395fc070e84bd8357ce56081959d87/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/e2052e86d1395fc070e84bd8357ce56081959d87/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request 📉 performance Optimization, memory use, etc 🏎️ Quick Issue Can be fixed in a few hours or less 📺 re_viewer affects re_viewer itself
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants