Use rayon to drive parallel layout and styling. #13641

emilio · 2016-10-07T17:36:36Z

The current work queue had a really annoying constraint: The size of the node had to be the size of the work unit data.

This makes it impractical for the new restyling model where we plan to pass down a bunch of data.

Rayon by default makes you wait for the result of the work unit, which makes it impractical for the current model (it's mostly sequential).

I added an API to rayon that allows us to push work to the queue without waiting (https://github.com/nikomatsakis/rayon/pull/103).

This still needs some work (for example, we're loosing the memory reporting functionality), but I wanted feedback on this.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2016-10-07T17:36:39Z

Heads up! This PR modifies the following files:

@bholley: components/style/Cargo.toml, components/style/lib.rs, components/style/parallel.rs, components/style/workqueue.rs, components/style/thread_state.rs

highfive · 2016-10-07T17:36:40Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify style and layout code, but no tests are modified. Please consider adding a test!

emilio · 2016-10-07T17:37:19Z

@pcwalton, could you take a look at this and see if it makes sense to you?

Also, cc @bholley, @SimonSapin, and @Manishearth.

larsbergstrom · 2016-10-07T17:45:21Z

components/style/workqueue.rs

-                                }
-                            }
-
-                            if i > SPINS_UNTIL_BACKOFF {


I'd like to see benchmarks of Rayon against our threadpool, as a lot of the backoff stuff was pretty highly tuned for the work item sizes of style and layout. These sorts of things are really easy to kill performance on small-sized work items if you move to locks/condvars/signals/etc.

Fair enough, I'll try to get some numbers.

After spending a few minutes studying Servo profiler dumps, I can't really draw a strong conclusion about this. Sometimes slower, sometimes faster, but always on the same order...

larsbergstrom · 2016-10-07T17:45:21Z

components/layout/parallel.rs


                if self.should_record_thread_ids() {
-                    flow::mut_base(flow).thread_id = proxy.worker_index();
+                    // flow::mut_base(flow).thread_id = proxy.worker_index();


Do we no longer use this?

That was for the layout tinting debug mode, I'm not sure it still works now.

aneeshusa · 2016-10-07T18:18:07Z

FYI, I think I only check for all-uppercase WIP/TODO in homu: servo/homu@1187df0. Might want to file an issue for having it work with lowercase as well.

pcwalton · 2016-10-08T01:58:59Z

I've been wanting to do this for quite a while! This is great! It's always nice when we can move over to community solutions.

I second wanting to get solid numbers on this. If you can profile a few pages and make sure our parallel gains are the same as before (check ARM too), then I'm cool with it.

bors-servo · 2016-10-13T07:44:03Z

☔ The latest upstream changes (presumably #13722) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo · 2016-10-20T21:39:58Z

☔ The latest upstream changes (presumably #13843) made this pull request unmergeable. Please resolve the merge conflicts.

emilio · 2016-10-24T13:30:19Z

The status of this is the following: I've benchmarked rayon and I think this is going to be good enough, and maybe better with low workloads given the rayon API executes the first call in the current thread and moves subsequent calls to worker threads.

Also, there's no unsafe code now there, only the Node::from_unsafe calls that I eventually plan to eliminate after this lands.

pcwalton · 2016-10-24T23:51:48Z

@emilio What numbers are we talking about? Within 10% or...?

emilio · 2016-10-25T15:40:48Z

I'd say the difference is way lower, less than 5% in my previous
profiles (and on some of them rayon even appeared faster in the
profile), but I need to profile again with the new Rayon, and
potentially with https://github.com/nikomatsakis/rayon/pull/122.

bholley · 2016-10-25T17:08:49Z

Are these synthetic benchmarks? Would be good to know what the impact is on stylo (i.e. redoing the measurements in http://people.mozilla.org/~bholley/stylo-london-2016/wikipedia-annotated.png and http://people.mozilla.org/~bholley/stylo-london-2016/spec-annotated.png

emilio · 2016-10-25T19:21:47Z

Nope, they're done basically browsing with servo, trusting a bit the profiler and a bit more perf. I'll try to do more accurate benchmarking.

pcwalton · 2016-10-25T21:06:35Z

If it's under 5% that's more than good enough for me. We may well make up way more than that with parallel display list construction, which rayon could reenable.

bholley · 2016-10-25T21:11:24Z

I'm generally optimistic here, but would like to see the numbers on stylo.

bors-servo · 2016-10-26T17:27:47Z

☔ The latest upstream changes (presumably #13913) made this pull request unmergeable. Please resolve the merge conflicts.

metajack · 2016-10-27T15:29:40Z

It seems likely we can get back this performance by optimizing Rayon. I agree with Patrick that this change seems worthwhile even at a minor performance cost.


        Auto merge of #532 - emilio:rayon, r=nox

Update rayon. I need this for servo/servo#13641  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/532)

bors-servo · 2016-11-13T00:25:19Z

💔 Test failed - linux-dev

emilio · 2016-11-13T10:56:52Z

@bors-servo try

bors-servo · 2016-11-13T10:56:55Z

⌛ Trying commit b7971d6 with merge 38e97d2...


        Auto merge of #13641 - emilio:rayon-style, r=<try>

Use rayon to drive parallel layout and styling.  The current work queue had a really annoying constraint: The size of the node had to be the size of the work unit data. This makes it impractical for the new restyling model where we plan to pass down a bunch of data. Rayon by default makes you wait for the result of the work unit, which makes it impractical for the current model (it's mostly sequential). I added an API to rayon that allows us to push work to the queue without waiting (https://github.com/nikomatsakis/rayon/pull/103). This still needs some work (for example, we're loosing the memory reporting functionality), but I wanted feedback on this. ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13641)

bors-servo · 2016-11-13T12:32:36Z

💔 Test failed - linux-rel-css

bors-servo · 2016-11-13T21:13:19Z

☔ The latest upstream changes (presumably #14187) made this pull request unmergeable. Please resolve the merge conflicts.


        style: Use rayon instead of our custom work queue.


        mark submit-entity-body.html as needing a long timeout.

After discussion with jdm.

emilio · 2016-11-14T20:26:57Z

@bors-servo r=pcwalton

There we go.

bors-servo · 2016-11-14T20:26:57Z

📌 Commit 58bd039 has been approved by pcwalton

bors-servo · 2016-11-14T20:47:22Z

⌛ Testing commit 58bd039 with merge 9467fbe...


        Auto merge of #13641 - emilio:rayon-style, r=pcwalton

Use rayon to drive parallel layout and styling.  The current work queue had a really annoying constraint: The size of the node had to be the size of the work unit data. This makes it impractical for the new restyling model where we plan to pass down a bunch of data. Rayon by default makes you wait for the result of the work unit, which makes it impractical for the current model (it's mostly sequential). I added an API to rayon that allows us to push work to the queue without waiting (https://github.com/nikomatsakis/rayon/pull/103). This still needs some work (for example, we're loosing the memory reporting functionality), but I wanted feedback on this. ---  - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13641)

bors-servo · 2016-11-14T22:11:28Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

highfive assigned larsbergstrom Oct 7, 2016

highfive added the S-awaiting-review label Oct 7, 2016

larsbergstrom reviewed Oct 7, 2016

View changes

highfive added the S-needs-rebase label Oct 13, 2016

nikomatsakis mentioned this pull request Oct 15, 2016

introduce explicit scope API; cleanup unwinding rayon-rs/rayon#71

Merged

5 of 5 tasks complete

emilio force-pushed the emilio:rayon-style branch 2 times, most recently from 6e393f2 to 83bc2f9 Oct 20, 2016

highfive added the S-needs-rebase label Oct 20, 2016

emilio force-pushed the emilio:rayon-style branch 2 times, most recently from 317d116 to 4c4a387 Oct 22, 2016

highfive added the S-needs-rebase label Oct 26, 2016

emilio mentioned this pull request Nov 5, 2016

Update rayon. servo/webrender#532

Merged

emilio force-pushed the emilio:rayon-style branch from 4c4a387 to ea06174 Nov 5, 2016

highfive added the S-tests-failed label Nov 13, 2016

emilio force-pushed the emilio:rayon-style branch from 0e7af1c to b7971d6 Nov 13, 2016

highfive removed the S-tests-failed label Nov 13, 2016

highfive added the S-tests-failed label Nov 13, 2016

highfive added S-needs-rebase and removed S-tests-failed labels Nov 13, 2016

emilio added 2 commits Oct 7, 2016

style: Use rayon instead of our custom work queue.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

73917cc

mark submit-entity-body.html as needing a long timeout.

Unverified

This user has not uploaded their public key yet.

GPG key ID: 056B727BB9C1027C Learn about signing commits

Loading status checks…

58bd039

After discussion with jdm.

emilio force-pushed the emilio:rayon-style branch from 8948a5c to 58bd039 Nov 14, 2016

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Nov 14, 2016

bors-servo merged commit 58bd039 into servo:master Nov 14, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Nov 14, 2016

emilio deleted the emilio:rayon-style branch Nov 15, 2016

servo / servo

Use rayon to drive parallel layout and styling. #13641

Use rayon to drive parallel layout and styling. #13641

emilio commented Oct 7, 2016 •

edited by larsbergstrom

highfive commented Oct 7, 2016

highfive commented Oct 7, 2016

emilio commented Oct 7, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa commented Oct 7, 2016

pcwalton commented Oct 8, 2016

bors-servo commented Oct 13, 2016

bors-servo commented Oct 20, 2016

emilio commented Oct 24, 2016

pcwalton commented Oct 24, 2016

emilio commented Oct 25, 2016

bholley commented Oct 25, 2016

emilio commented Oct 25, 2016

pcwalton commented Oct 25, 2016

bholley commented Oct 25, 2016

bors-servo commented Oct 26, 2016

metajack commented Oct 27, 2016

bors-servo commented Nov 13, 2016

emilio commented Nov 13, 2016

bors-servo commented Nov 13, 2016

bors-servo commented Nov 13, 2016

bors-servo commented Nov 13, 2016

emilio commented Nov 14, 2016

bors-servo commented Nov 14, 2016

bors-servo commented Nov 14, 2016

bors-servo commented Nov 14, 2016

servo / servo

Join GitHub today

Use rayon to drive parallel layout and styling. #13641

Use rayon to drive parallel layout and styling. #13641

Conversation

emilio commented Oct 7, 2016 • edited by larsbergstrom

highfive commented Oct 7, 2016

highfive commented Oct 7, 2016

emilio commented Oct 7, 2016

This comment has been minimized.

larsbergstrom Oct 7, 2016

This comment has been minimized.

emilio Oct 7, 2016

This comment has been minimized.

emilio Oct 7, 2016

This comment has been minimized.

larsbergstrom Oct 7, 2016

This comment has been minimized.

emilio Oct 7, 2016

aneeshusa commented Oct 7, 2016

pcwalton commented Oct 8, 2016

bors-servo commented Oct 13, 2016

bors-servo commented Oct 20, 2016

emilio commented Oct 24, 2016

pcwalton commented Oct 24, 2016

emilio commented Oct 25, 2016

bholley commented Oct 25, 2016

emilio commented Oct 25, 2016

pcwalton commented Oct 25, 2016

bholley commented Oct 25, 2016

bors-servo commented Oct 26, 2016

metajack commented Oct 27, 2016

bors-servo commented Nov 13, 2016

emilio commented Nov 13, 2016

bors-servo commented Nov 13, 2016

bors-servo commented Nov 13, 2016

bors-servo commented Nov 13, 2016

emilio commented Nov 14, 2016

bors-servo commented Nov 14, 2016

bors-servo commented Nov 14, 2016

bors-servo commented Nov 14, 2016

emilio commented Oct 7, 2016 •

edited by larsbergstrom