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

Render in parallel #2609

Merged
merged 4 commits into from
Jun 20, 2014
Merged

Render in parallel #2609

merged 4 commits into from
Jun 20, 2014

Conversation

brson
Copy link
Contributor

@brson brson commented Jun 6, 2014

This round-robins tile rendering across cores. I put this together pretty quickly at the end of the work week, but it's probably close to ready. The speedups aren't as good as you might expect (perf-rainbow is decent), but it is generally faster, these test cases aren't all that rendering-intense, and there's probably some unknown synchronization happening deeper in the stack.

Some numbers (in ms) on my 4 core machine:

CPU mode, en.wikipedia.org/wiki/Rust

before: RenderingCategory: 108.3725 102.9995 2.7500 280.4812 91
after: RenderingCategory: 85.5513 87.8209 3.6918 149.5089 121

GPU mode, en.wikipedia.org/wiki/Rust

before: RenderingCategory: 383.4462 308.6807 151.7053 840.7829 22
after: RenderingCategory: 338.4899 360.0376 90.4928 586.7247 54

CPU mode, lipsum.html

before: RenderingCategory: 43.8702 41.5389 0.4079 119.9315 165
after: RenderingCategory: 51.1924 48.0576 0.3047 123.5846 113

GPU mode, lipsum.html

before: RenderingCategory: 374.0989 396.5835 88.6352 629.7278 30
after: RenderingCategory: 222.2546 229.1403 58.1653 442.7781 46

CPU, perf-rainbow.html

before: RenderingCategory: 54.5183 56.3723 29.4359 93.4502 22
after: RenderingCategory: 25.5713 22.3374 2.6982 93.4121 65

@hoppipolla-critic-bot
Copy link

Critic review: https://critic.hoppipolla.co.uk/r/1761

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.

@highfive
Copy link

highfive commented Jun 6, 2014

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

// Rust types, not C types
#[allow(ctypes)]
extern {
fn rust_get_num_cpus() -> uint;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a layout_threads option in gfx::opts for this; I would suggest just using the same value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually found an n_render_threads value in that same struct that I wired up in a follow up. Do you think I should combine them or leave the separate knobs?

@brson
Copy link
Contributor Author

brson commented Jun 7, 2014

The most recent push of this branch will definitely not pass tests quite yet.

@brson
Copy link
Contributor Author

brson commented Jun 9, 2014

Passes make check on linux now.

@pcwalton
Copy link
Contributor

So $10 says what's happening is that the glyph cache is per-thread and much of the time is spent filling it up, reducing the parallel gains. I can profile tomorrow to check to see if that's the case.

@brson
Copy link
Contributor Author

brson commented Jun 19, 2014

Rebased.

pcwalton added a commit that referenced this pull request Jun 20, 2014
@pcwalton pcwalton merged commit 850bd28 into servo:master Jun 20, 2014
larsbergstrom added a commit to larsbergstrom/servo that referenced this pull request Jun 20, 2014
This reverts commit 850bd28, reversing
changes made to 5b0feac.
brson added a commit that referenced this pull request Jun 20, 2014
Revert "Merge pull request #2609 from brson/parallel-render"
pcwalton added a commit to pcwalton/servo that referenced this pull request Jun 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants