Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upUse rayon's thread pool. #1202
Merged
Use rayon's thread pool. #1202
Conversation
|
r? @kvark |
|
I am being told that it would be best to wait for servo/servo#16303 to be merged before merging this one in order to avoid having 0.6 in servo and 0.7 in webrender. |
webrender/src/renderer.rs
Outdated
| @@ -2215,7 +2217,7 @@ pub struct RendererOptions { | |||
| pub enable_batcher: bool, | |||
| pub render_target_debug: bool, | |||
| pub max_texture_size: Option<u32>, | |||
| pub workers: Option<Arc<Mutex<ThreadPool>>>, | |||
| pub workers: Option<Arc<Mutex<rayon::ThreadPool>>>, | |||
This comment has been minimized.
This comment has been minimized.
emilio
May 4, 2017
Member
Do you need the mutex now? It seems the spawn_async function takes an immutable ThreadPool, so I don't see why off-hand.
|
Thanks @emilio, it's quite a lot nicer without that mutex around the thread pool! |
|
Looks great, there is a couple of notes. |
webrender/src/renderer.rs
Outdated
| @@ -1046,9 +1046,11 @@ impl Renderer { | |||
| let render_target_debug = options.render_target_debug; | |||
| let payload_tx_for_backend = payload_tx.clone(); | |||
| let recorder = options.recorder; | |||
| // TODO(gw): Use a heuristic to select best # of worker threads. | |||
| let worker_config = rayon::Configuration::new().thread_name(|idx|{ format!("WebRender:Worker#{}", idx) }) | |||
| .num_threads(4); | |||
This comment has been minimized.
This comment has been minimized.
kvark
May 4, 2017
Member
let's not even call num_threads?
If num_threads is 0, or you do not call this function, then the Rayon runtime will select the number of threads automatically. At present, this is based on the RAYON_RS_NUM_CPUS (if set), or the number of logical CPUs (otherwise).
webrender/src/renderer.rs
Outdated
| @@ -2215,7 +2217,7 @@ pub struct RendererOptions { | |||
| pub enable_batcher: bool, | |||
| pub render_target_debug: bool, | |||
| pub max_texture_size: Option<u32>, | |||
| pub workers: Option<Arc<Mutex<ThreadPool>>>, | |||
| pub workers: Option<Arc<rayon::ThreadPool>>, | |||
This comment has been minimized.
This comment has been minimized.
kvark
May 4, 2017
Member
while I use this style in some of the personal project, I believe the convention in WR is to use all the types you need
glennw
changed the title
|
@nical I marked this as [DO NOT MERGE] based on your comment #1202 (comment) - feel free to update the title when this is ready to go :) |
|
|
stshine
commented
May 9, 2017
|
servo/servo#16303 has landed. |
nical
changed the title
|
@bors-servo r+ |
|
|
bors-servo
added a commit
that referenced
this pull request
May 9, 2017
Use rayon's thread pool. cf. issue #1178. Rayon's thread pool implementation is a lot better than the one provided by the threadpool crate, and comes with a lot of very useful additional features. The spawn_async functionality which we need is currently behind the unstable feature flag. Note that stylo is also using rayon with the unstable feature flag and is bumping to version 0.7 in servo/servo#16303. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1202) <!-- Reviewable:end -->
|
|
bors-servo
added a commit
to servo/servo
that referenced
this pull request
May 14, 2017
WR update: preserve-3d support <!-- Please describe your changes on the following line: --> This is WR update to servo/webrender@d335555 having new features: - limited "preserve-3d" support (servo/webrender#1169, servo/webrender#1208) - rayon thread pool (servo/webrender#1202) - further border rendering improvements Edit: the references to bincode serialization and border styles are removed from here, since they are already integrated into Servo. Edit2: this is alternative/similar to #16801, based on @mrobinson code (see first commit). --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). Related to #9087 Note that I'm unlocking a few tests as well as changing some related to `preserve-3d`. The changes come from common sense and comparison to Chromium. I'm ready to discuss them on a individual basis. <!-- Either: --> - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____ There is still an investigation to do with regards to the differences of preserve3d logic between Blink and Gecko. <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16775) <!-- Reviewable:end -->
bors-servo
added a commit
that referenced
this pull request
May 17, 2017
Let rayon automatically choose the number of threads in the ThreadPool. This is a tiny followup from PR #1202 that I wanted to submit in isolation so that we can easily check its effect on performance (if any) and roll it back if needed. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1259) <!-- Reviewable:end -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
nical commentedMay 4, 2017
•
edited by larsbergstrom
cf. issue #1178.
Rayon's thread pool implementation is a lot better than the one provided by the threadpool crate, and comes with a lot of very useful additional features. The spawn_async functionality which we need is currently behind the unstable feature flag. Note that stylo is also using rayon with the unstable feature flag and is bumping to version 0.7 in servo/servo#16303.
This change is