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

Dynamically switch to sequential layout if only a small amount of reflow needs to happen #10110

Closed
pcwalton opened this issue Mar 22, 2016 · 25 comments

Comments

@pcwalton
Copy link
Contributor

@pcwalton pcwalton commented Mar 22, 2016

Parallel layout overhead is overkill if only a small number of nodes need to be reflowed. In the extreme case, if one chain of nodes from parent to child needs to be reflowed, then parallel layout cannot help at all since it's sequentialized, and all of the traversal code is pure overhead!

The easiest way to solve this, I think, would be to have an atomic counter of the number of nodes that were discovered to be dirty (incremental.rs can compute this, perhaps) and have the layout thread consult that number. If below a threshold (8? wild guess) then parallel layout will not be spawned. Note that we don't want this to regress performance on pages that have a small number of nodes but lots of reflow to do: consider four paragraphs of 10,000 words each, for instance, which has a small number of nodes but is the ideal case for parallel layout. So we'll probably need a couple of heuristics here and tune appropriately.

cc @paulrouget

@pcwalton pcwalton changed the title Dynamically switch to sequential layout if only a small number of nodes needed to be reflowed Dynamically switch to sequential layout if only a small amount of reflow needs to happen Mar 22, 2016
@larsbergstrom
Copy link
Contributor

@larsbergstrom larsbergstrom commented Mar 22, 2016

@rzambre has been doing a bunch of predictive work here (for more than just the size of the DOM tree, but also metrics around its width at each height) to predict whether parallelism is a payoff.

So far, we've definitely seen that this is a per-architecture decision, but he has a framework and some testing that is gathering a lot of info here. Integrating this into Servo is the planned topic of his RA/internship this summer!

@pcwalton
Copy link
Contributor Author

@pcwalton pcwalton commented Mar 22, 2016

Nice! Paul's work isolating performance issues in browser.html has shown that there should be a real benefit from this area. :)

@larsbergstrom
Copy link
Contributor

@larsbergstrom larsbergstrom commented Mar 22, 2016

One of the things I'm most excited about here is also just making those gobs of tiny iframes (tracker icons, just a navpane, etc.) only take one thread. We might get CNN.com under 250 threads in one swoop! :-)

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Mar 22, 2016

Is that a thing that could be done by June?

@larsbergstrom
Copy link
Contributor

@larsbergstrom larsbergstrom commented Mar 22, 2016

I'll talk to @rzambre. We could probably get some very simple heuristics in place pretty quickly based on some of the numbers he's gathered around cutoff points for this metric.

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Mar 23, 2016

I'll talk to @rzambre. We could probably get some very simple heuristics in place pretty quickly based on some of the numbers he's gathered around cutoff points for this metric.

That would be awesome! I'd like to see how much this could improve browserhtml performance.

@rzambre
Copy link
Contributor

@rzambre rzambre commented Mar 31, 2016

I am quite confident about the influence of the following DOM-tree features on the amount of parallel work available: DOM-size (total number of DOM nodes), average width of the DOM tree, number of leaves in the DOM tree.

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented Apr 5, 2016

I am quite confident about the influence of the following DOM-tree features on the amount of parallel work available: DOM-size (total number of DOM nodes), average width of the DOM tree, number of leaves in the DOM tree.

@rzambre are you working on this?

@rzambre
Copy link
Contributor

@rzambre rzambre commented Apr 5, 2016

@paulrouget Yes! Our group here at UC Irvine has been working on creating a predictive classification model for the parallel styling/layout performance of Servo using DOM tree characteristics as the predictive features. Using 8 DOM tree characteristics, we have some simple models that achieve an accuracy of up to 80%. Those were based on performance numbers on a laptop platform. We are now looking at doing the same with an embedded/mobile platform. We are expecting to see higher distinctions between performance numbers for different kinds of DOMs. But as far as incorporating these results into Servo, we haven't done any of that yet. We'd like to see higher accuracies from our embedded system data set. Also, we'd like to dig a little deeper into the inner working of Servo's parallel styling/layout to possibly capture new predictive features. But we can definitely begin incorporating heuristics that we feel confident about!

@rzambre
Copy link
Contributor

@rzambre rzambre commented Apr 28, 2016

After going through our labeled classification data, here are some raw cut-off numbers for parallelization of overall layout (styling + primary layout passes).

If we are looking at only one feature to decide whether or not to parallelize, these would be the threshold values:

  • DOM-size: parallelize if greater than 750
  • Number of leaves: parallelize if greater than 385
  • Average tree width: parallelize if greater than 45

If we are looking at a combination of features then I would suggest the following:

Parallelize only if (DOM-size > 600 AND num-of-leaves > 300 AND avg-tree-width > 45). Otherwise, perform overall layout sequentially.

@paulrouget
Copy link
Contributor

@paulrouget paulrouget commented May 2, 2016

Parallelize only if (DOM-size > 600 AND num-of-leaves > 300 AND avg-tree-width > 45). Otherwise, perform overall layout sequentially.

Is there an easy way to test if my page fits in there?

@rzambre
Copy link
Contributor

@rzambre rzambre commented May 4, 2016

We'll have to collect these numbers for each page that is being loaded. I'm thinking DOM-size and num-of-leaves can be easily computed by incrementing counters when the HTML parser works on new/incremental HTML content. avg-tree-width could be trickier to collect since we might have to wait until the whole DOM tree of the page is constructed; the width at each height-level of the tree needs to be collected.
I had used a BFS traversal on the DOM trees built from saved HTML files to collect these numbers on the pages that I was working with.

@metajack
Copy link
Contributor

@metajack metajack commented May 10, 2016

You can test how this affects browser.html by running with -y 1. I'm not sure why this is a P2 bhtml bug though. It seems like this would have minimal impact.

Note that we can start layout before the whole DOM is loaded. Specifically we start layout after N milliseconds where N is some small constant like 12 or so. So whatever heuristics we want will be applied to whatever DOM we get in that timeframe, although as we get more we can obviously run layout with different numbers of threads on subsequent passes.

A first pass implementation of this could just wait until the DOM is fully realized, do the calculation and just make further layouts single threaded. Then in later improvements we can start trying to make this decision earlier.

@avadacatavra avadacatavra self-assigned this Jun 7, 2016
@metajack
Copy link
Contributor

@metajack metajack commented Jun 7, 2016

Diane and I discussed this. I think the first step will be to implement the @rzambre's DOM-size metric first, which should be easy to calculate earlier in the pipeline.

@metajack metajack added the C-assigned label Jun 7, 2016
@avadacatavra
Copy link
Contributor

@avadacatavra avadacatavra commented Jun 8, 2016

I'm thinking that I'll stick a counter in the ServoHTMLParser and keep track when create_element is called.

Is that reasonable? Or is there a better place to stick the counting?

@larsbergstrom
Copy link
Contributor

@larsbergstrom larsbergstrom commented Jun 8, 2016

@dd0x68 That seems reasonable to me. @jdm, will that capture all of the DOM element creations and be available by the time we want to perform layout?

@jdm
Copy link
Member

@jdm jdm commented Jun 8, 2016

Yes and yes, depending on where the count is stored.

@avadacatavra
Copy link
Contributor

@avadacatavra avadacatavra commented Jun 8, 2016

@jdm That's what I'm trying to figure out now

@metajack
Copy link
Contributor

@metajack metajack commented Jun 8, 2016

Do we want to update the count on element removal as well? Or just assume that once we switch to parallel mode for a layout we stay in parallel mode?

@rzambre @larsbergstrom it occurs to me that relayout will be some subset of the flow tree. I assume your results were just for full layout passes? Maybe later we need to be smarter and make this based on dirty flow tree nodes.

@avadacatavra
Copy link
Contributor

@avadacatavra avadacatavra commented Jun 8, 2016

@metajack From what I see, the parallel/sequential determination is made when the LayoutThread is created. I guess to go to sequential we would just terminate the workers and set parallel_traversal to None. It seems like we would end up either losing state or redoing work in that case.

Currently, I'm just assuming that once we switch to parallel mode, we stay in parallel mode.

@metajack
Copy link
Contributor

@metajack metajack commented Jun 8, 2016

Yes, that is how it works now and I think we should constrain the first pass at this.

In the future, it may make sense to make this more dynamic. Baby steps :)

@jdm
Copy link
Member

@jdm jdm commented Jun 8, 2016

The LayoutThread is created before the page content is available, so the decision will need to be made later than that.

notriddle added a commit that referenced this issue Aug 18, 2016
notriddle added a commit that referenced this issue Nov 6, 2016
bors-servo added a commit that referenced this issue Nov 6, 2016
added dom obj counting to decide sequential/parallel layout (#10110)

This is a rebased version of #11713
---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #10110 (github issue number if applicable).
- [X] There are no tests for these changes because it's an optimization with no visible behavioral changes

<!-- 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/12862)

<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Nov 6, 2016
added dom obj counting to decide sequential/parallel layout (#10110)

This is a rebased version of #11713
---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #10110 (github issue number if applicable).
- [X] There are no tests for these changes because it's an optimization with no visible behavioral changes

<!-- 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/12862)

<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Nov 7, 2016
added dom obj counting to decide sequential/parallel layout (#10110)

This is a rebased version of #11713
---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #10110 (github issue number if applicable).
- [X] There are no tests for these changes because it's an optimization with no visible behavioral changes

<!-- 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/12862)

<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Nov 7, 2016
added dom obj counting to decide sequential/parallel layout (#10110)

This is a rebased version of #11713
---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #10110 (github issue number if applicable).
- [X] There are no tests for these changes because it's an optimization with no visible behavioral changes

<!-- 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/12862)

<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Nov 7, 2016
added dom obj counting to decide sequential/parallel layout (#10110)

This is a rebased version of #11713
---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #10110 (github issue number if applicable).
- [X] There are no tests for these changes because it's an optimization with no visible behavioral changes

<!-- 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/12862)

<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Nov 14, 2016
added dom obj counting to decide sequential/parallel layout (#10110)

This is a rebased version of #11713

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #10110 (github issue number if applicable).
- [X] There are no tests for these changes because it's an optimization with no visible behavioral changes

<!-- 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/12862)

<!-- Reviewable:end -->
notriddle added a commit that referenced this issue Nov 14, 2016
notriddle added a commit that referenced this issue Nov 14, 2016
notriddle added a commit that referenced this issue Nov 14, 2016
notriddle added a commit that referenced this issue Nov 15, 2016
notriddle added a commit that referenced this issue Nov 15, 2016
notriddle added a commit that referenced this issue Nov 15, 2016
bors-servo added a commit that referenced this issue Nov 15, 2016
added dom obj counting to decide sequential/parallel layout (#10110)

This is a rebased version of #11713

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #10110 (github issue number if applicable).
- [X] There are no tests for these changes because it's an optimization with no visible behavioral changes

<!-- 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/12862)

<!-- Reviewable:end -->
notriddle added a commit that referenced this issue Nov 15, 2016
notriddle added a commit that referenced this issue Nov 16, 2016
bors-servo added a commit that referenced this issue Nov 16, 2016
added dom obj counting to decide sequential/parallel layout (#10110)

This is a rebased version of #11713

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #10110 (github issue number if applicable).
- [X] There are no tests for these changes because it's an optimization with no visible behavioral changes

<!-- 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/12862)

<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Nov 18, 2016
added dom obj counting to decide sequential/parallel layout (#10110)

This is a rebased version of #11713

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #10110 (github issue number if applicable).
- [X] There are no tests for these changes because it's an optimization with no visible behavioral changes

<!-- 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/12862)

<!-- Reviewable:end -->
notriddle added a commit that referenced this issue Nov 28, 2016
bors-servo added a commit that referenced this issue Nov 28, 2016
added dom obj counting to decide sequential/parallel layout (#10110)

This is a rebased version of #11713

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #10110 (github issue number if applicable).
- [X] There are no tests for these changes because it's an optimization with no visible behavioral changes

<!-- 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/12862)

<!-- Reviewable:end -->
notriddle added a commit that referenced this issue Dec 5, 2016
bors-servo added a commit that referenced this issue Dec 5, 2016
added dom obj counting to decide sequential/parallel layout (#10110)

This is a rebased version of #11713

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #10110 (github issue number if applicable).
- [X] There are no tests for these changes because it's an optimization with no visible behavioral changes

<!-- 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/12862)

<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Dec 6, 2016
added dom obj counting to decide sequential/parallel layout (#10110)

This is a rebased version of #11713

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #10110 (github issue number if applicable).
- [X] There are no tests for these changes because it's an optimization with no visible behavioral changes

<!-- 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/12862)

<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Dec 7, 2016
added dom obj counting to decide sequential/parallel layout (#10110)

This is a rebased version of #11713

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #10110 (github issue number if applicable).
- [X] There are no tests for these changes because it's an optimization with no visible behavioral changes

<!-- 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/12862)

<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Dec 7, 2016
added dom obj counting to decide sequential/parallel layout (#10110)

This is a rebased version of #11713

---
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes fix #10110 (github issue number if applicable).
- [X] There are no tests for these changes because it's an optimization with no visible behavioral changes

<!-- 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/12862)

<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

7 participants
You can’t perform that action at this time.