layout: Dramatically improve performance of layout queries and `requestAnimationFrame()`. #14442

pcwalton · 2016-12-02T20:57:25Z

See individual commits for details. This improves nytimes.com CPU usage and gets it within the ballpark of Firefox.

This change is


        layout: Replace `ConstructionResult::swap_out()` with

`ConstructionResult::get()`. It only actually swaps out in nonincremental mode (which isn't suitable for real world use), so the name is confusing.


        script: Detect when the page is using `requestAnimationFrame()` for

non-animation purposes and back off to a timer if so. After this patch, when the page calls `requestAnimationFrame()` too many times in the row without actually mutating the DOM, further calls to `rAF` will actually perform the moral equivalent of `setTimeout(16)`. This saves a lot of CPU time compared to actually waiting for the vertical blanking interval, because waiting for that requires us to draw the page. Reduces CPU usage drastically on nytimes.com, which has a perpetual `requestAnimationFrame()` loop that checks whether ads are in view.


        query: Stop searching the entire flow tree to resolve script-to-layout

queries. Instead of searching the entire tree to find the applicable fragment, we use the flow construction result of the nearest DOM node to find the flow we're looking for directly. We then recursively do this with ancestor DOM nodes in order to find the entire flow tree path, performing breadth first search to deal with anonymous flows in between. To fully eliminate the search, we will probably want some combination of (a) parent pointers in the flow tree and (b) rewritten script-to-layout queries that don't depend on visiting all ancestor flows of the query node. But this is a quick and dirty solution that dramatically improves the performance of layout queries. This is a large responsiveness increase on nytimes.com.


        layout_thread: Don't propagate dirty bits around the tree if no styles

changed and no flows were reconstructed. This saves a tree traversal. It also has the side effect of making incremental reflow bugs of the form "a dirty bit is never cleared" less likely to ruin the performance of layout queries.


        layout_thread: Assert that we removed the `REPAINT` flag from the root

after building display lists.


        layout_thread: Minor style cleanup.

highfive · 2016-12-02T20:57:29Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/document.rs, components/script/timers.rs, components/script_layout_interface/wrapper_traits.rs
@fitzgen: components/script/dom/document.rs, components/script/timers.rs, components/script_layout_interface/wrapper_traits.rs
@emilio: components/layout/construct.rs, components/layout/sequential.rs, components/layout/query.rs, components/layout/flow.rs

highfive · 2016-12-02T20:57:30Z

Warning

These commits modify layout and script code, but no tests are modified. Please consider adding a test!

fitzgen · 2016-12-02T21:18:37Z

Regarding ba274c9: Is an RAF that performs canvas draws but no dom mutations considered "spurious"? Is that desirable?

pcwalton · 2016-12-02T21:54:38Z

@fitzgen No, it shouldn't be considered spurious. Does it dirty the canvas? If not, I'm not sure what the best way to detect this is.

pcwalton · 2016-12-02T21:59:14Z

The canvas element is dirtied when you draw to it, so there should be no problems here.

emilio

r=me on the layout changes, they're great :)

I'm not sure we can fake animation frame callbacks using a timer per spec. IIRC we should run them at a very specific timing per the processing model, and we're already awful at it unfortunately.

I'd love to see this, and it may be the case that results are black-box identical in this case (though I think not because at least the last fake callback that modifies something and makes us go back to real animation frames runs out of sync).

I'd probably defer to @jdm or @Ms2ger to approve that bit, or request more proof of that being the case.

emilio · 2016-12-03T01:08:21Z

components/script/dom/document.rs

+    ///
+    /// A rAF request is considered spurious if nothing was actually reflowed.
+    spurious_animation_frames: Cell<u8>,
+


nit: Extra newline.

emilio · 2016-12-03T01:08:21Z

components/layout/sequential.rs

+    // Find the path from the root to the node.
+    //
+    // FIXME(pcwalton): This would be unnecessary if we had parent pointers in the flow tree. I
+    // think they're going to be inevitable.


Yeah, this is relatively expensive, though better that a full traversal I guess...

emilio · 2016-12-03T01:08:21Z

components/layout/sequential.rs

+        }
+
+        // Find the next child in the path.
+        let target_child = *path.last().expect("Path ended prematurely!");


I think here it's acceptable to just unwrap given the check above.

emilio · 2016-12-03T01:09:04Z

@bors-servo try

bors-servo · 2016-12-03T01:09:07Z

⌛ Trying commit 89895e3 with merge 6d97221...


        Auto merge of #14442 - pcwalton:raf-fallback, r=<try>

@emilio

layout: Dramatically improve performance of layout queries and `requestAnimationFrame()`. See individual commits for details. This improves nytimes.com CPU usage and gets it within the ballpark of Firefox. r? @emilio  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14442)

bors-servo · 2016-12-03T01:52:39Z

💔 Test failed - mac-rel-wpt2

pcwalton · 2016-12-03T07:39:32Z

@emilio I don't see anything in that list that mandates when animation frame callbacks run, only that they run at specific times relative to other events. We already don't try to do that, I don't think. Indeed it'd be impossible for the spec to mandate a specific time other than describing what a VBLANK is (and if it did, then every other browser on non-Windows platforms would fail it because they don't sync to VBLANK properly).

(On a side note, it's irritating that the spec technically forbids off main thread CSS animations, but I'm going to just break the spec because that's dumb.)

emilio · 2016-12-03T11:05:22Z

On Fri, Dec 02, 2016 at 11:39:39PM -0800, Patrick Walton wrote: We already don't try to do that, I don't think. Indeed it'd be impossible for the spec to mandate a specific time other than describing what a VBLANK is (and if it did, then every other browser on non-Windows platforms would fail it because they don't sync to VBLANK properly).

That's fair, that's why I lean towards merging this. But I think we eventually should try to conform to it, given there are pages that rely on it, and this will be more complexity to rewrite/remove when we want to do it.

bors-servo · 2016-12-07T03:28:09Z

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

jdm

This looks fine to me. Sorry that it was overlooked for so long!

jdm · 2017-02-02T15:27:19Z

components/script/dom/document.rs

+}
+
+impl FakeRequestAnimationFrameCallback {
+    #[allow(unrooted_must_root)]


This shouldn't be necessary.

nox · 2017-03-03T11:42:18Z

@jdm Want me to carry this? @pcwalton is busy with his Pathfinder adventures AFAIK.

jdm · 2017-03-03T18:19:16Z

That would be great!


        Auto merge of #15816 - servo:raf-timer, r=<try>

Improve performance of layout queries and requestAnimationFrame Supersedes #14442.


        Auto merge of #15816 - servo:raf-timer, r=<try>

Improve performance of layout queries and requestAnimationFrame Supersedes #14442.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15816)


        Auto merge of #15816 - servo:raf-timer, r=jdm,emilio

Improve performance of layout queries and requestAnimationFrame Part of #14442.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15816)

nox · 2017-03-05T13:17:52Z

I carried the first 2 commits in #15816. 👶👣

nox · 2017-03-12T11:02:14Z

@pcwalton Are you sure the commit "query: Stop searching the entire flow tree to resolve script-to-layout" works as intended?

nox · 2017-03-15T10:41:48Z

Lack of response, I'm closing this.

pcwalton added 6 commits Dec 2, 2016

layout: Replace `ConstructionResult::swap_out()` with

2a199a4

`ConstructionResult::get()`. It only actually swaps out in nonincremental mode (which isn't suitable for real world use), so the name is confusing.

layout_thread: Assert that we removed the `REPAINT` flag from the root

4139fd0

after building display lists.

layout_thread: Minor style cleanup.

Loading status checks…

89895e3

pcwalton added A-content/script A-layout/uncategorized I-perf-slow labels Dec 2, 2016

highfive assigned emilio Dec 2, 2016

highfive added the S-awaiting-review label Dec 2, 2016

emilio reviewed Dec 3, 2016

View changes

highfive added the S-tests-failed label Dec 3, 2016

highfive added the S-needs-rebase label Dec 7, 2016

jdm assigned jdm and unassigned emilio Jan 27, 2017

jdm requested changes Feb 2, 2017

View changes

components/script/dom/document.rs

}

impl FakeRequestAnimationFrameCallback {

#[allow(unrooted_must_root)]

This comment has been minimized.

Sign in to view

jdm Feb 2, 2017
Member

This shouldn't be necessary.

jdm removed the S-awaiting-review label Feb 2, 2017

jdm added the S-needs-code-changes label Feb 2, 2017

jdm mentioned this pull request Feb 2, 2017

nytimes.com is unusable #15248

Open

nox assigned nox and unassigned jdm Mar 3, 2017

nox mentioned this pull request Mar 3, 2017

Improve performance of layout queries and requestAnimationFrame #15816

Merged

bors-servo added a commit that referenced this pull request Mar 3, 2017

Auto merge of #15816 - servo:raf-timer, r=<try>

Loading status checks…

59bc904

Improve performance of layout queries and requestAnimationFrame Supersedes #14442.

nox closed this Mar 15, 2017

paulrouget mentioned this pull request Feb 18, 2020

Useless calls from the embedder #25305

Open

servo / servo

layout: Dramatically improve performance of layout queries and `requestAnimationFrame()`. #14442

layout: Dramatically improve performance of layout queries and `requestAnimationFrame()`. #14442

pcwalton commented Dec 2, 2016 •

edited by larsbergstrom

highfive commented Dec 2, 2016

highfive commented Dec 2, 2016

fitzgen commented Dec 2, 2016

pcwalton commented Dec 2, 2016

pcwalton commented Dec 2, 2016

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

emilio commented Dec 3, 2016

bors-servo commented Dec 3, 2016

bors-servo commented Dec 3, 2016

pcwalton commented Dec 3, 2016

emilio commented Dec 3, 2016

bors-servo commented Dec 7, 2016

jdm left a comment

This comment has been minimized.

nox commented Mar 3, 2017

jdm commented Mar 3, 2017

nox commented Mar 5, 2017

nox commented Mar 12, 2017

nox commented Mar 15, 2017

servo / servo

Join GitHub today

layout: Dramatically improve performance of layout queries and `requestAnimationFrame()`. #14442

layout: Dramatically improve performance of layout queries and `requestAnimationFrame()`. #14442

Conversation

pcwalton commented Dec 2, 2016 • edited by larsbergstrom

highfive commented Dec 2, 2016

highfive commented Dec 2, 2016

fitzgen commented Dec 2, 2016

pcwalton commented Dec 2, 2016

pcwalton commented Dec 2, 2016

emilio left a comment

This comment has been minimized.

emilio Dec 3, 2016

This comment has been minimized.

emilio Dec 3, 2016

This comment has been minimized.

emilio Dec 3, 2016

emilio commented Dec 3, 2016

bors-servo commented Dec 3, 2016

bors-servo commented Dec 3, 2016

pcwalton commented Dec 3, 2016

emilio commented Dec 3, 2016

bors-servo commented Dec 7, 2016

jdm left a comment

This comment has been minimized.

jdm Feb 2, 2017

nox commented Mar 3, 2017

jdm commented Mar 3, 2017

nox commented Mar 5, 2017

nox commented Mar 12, 2017

nox commented Mar 15, 2017

pcwalton commented Dec 2, 2016 •

edited by larsbergstrom