Improve decisions in compositor over when to draw a frame. by glennw · Pull Request #17398 · servo/servo

glennw · 2017-06-19T02:16:20Z

This patch fixes a couple of issues in the compositor:

Remove the delayed composition code. Previously, this would schedule
a composite for 12ms in the future. This doesn't really make any sense
with WR. There's no point in doing a composite unless WR has provided
a new frame to be drawn. This fixes issues in several benchmarks where
we were doing multiple composite / renders per rAF, which is a waste
of CPU time. This does make the framerate slower in some cases (such
as a slow rAF callback) but it's more correct - otherwise we were just
compositing the same frame multiple times for no real benefit.
Inform the window of the current animation state of the compositor.
Specifically, if an animation (or rAF) is currently active, the
window system switches to use event polling, and does not block on
the OS-level event loop. In the case of active animation, we just
assume that we want to be running as the vsync interval and not
blocking. This means the compositor thread only sleeps on vsync
during animation, which reduces OS scheduling and results in much
smoother animation.

This change is

glennw · 2017-06-19T02:17:45Z

I think the above comments are all correct, and this certainly results in smoother results and "correct" frame rates based on rAF, from what I've tested is. Nonetheless, changing things like this is quite subtle, and I might be missing something - so this could do with a careful look over!

glennw · 2017-06-19T02:19:16Z

Thread profile before this change (showing multiple composites per rAF etc):

Thread profile after this change - the parallelism is what I would expect to see, given the threads involved:

glennw · 2017-06-19T02:20:24Z

cc @asajeffrey and @cbrewster since I think you've been working in this area and may have comments.

glennw · 2017-06-19T20:20:33Z

Fixed tidy error.

pcwalton · 2017-06-19T21:15:17Z

@bors-servo: r+

bors-servo · 2017-06-19T21:15:18Z

📌 Commit 36829da has been approved by pcwalton

bors-servo · 2017-06-19T21:40:42Z

⌛ Testing commit 36829da with merge d150e48...

Improve decisions in compositor over when to draw a frame. This patch fixes a couple of issues in the compositor: 1) Remove the delayed composition code. Previously, this would schedule a composite for 12ms in the future. This doesn't really make any sense with WR. There's no point in doing a composite unless WR has provided a new frame to be drawn. This fixes issues in several benchmarks where we were doing multiple composite / renders per rAF, which is a waste of CPU time. This *does* make the framerate slower in some cases (such as a slow rAF callback) but it's more correct - otherwise we were just compositing the same frame multiple times for no real benefit. 2) Inform the window of the current animation state of the compositor. Specifically, if an animation (or rAF) is currently active, the window system switches to use event polling, and does not block on the OS-level event loop. In the case of active animation, we just assume that we want to be running as the vsync interval and not blocking. This means the compositor thread only sleeps on vsync during animation, which reduces OS scheduling and results in much smoother animation.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17398)

bors-servo · 2017-06-19T22:30:20Z

💔 Test failed - linux-rel-wpt

jdm · 2017-06-19T22:43:48Z

{"status": "NOTRUN", "stack": null, "subtest": "Overall test", "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/canvas/buffer-offscreen-test.html", "line": 293680, "message": null, "expected": "PASS"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/canvas/buffer-offscreen-test.html", "line": 293685, "message": null, "expected": "OK"}
{"status": "NOTRUN", "stack": null, "subtest": "Overall test", "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/canvas/buffer-preserve-test.html", "line": 293797, "message": null, "expected": "PASS"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/canvas/buffer-preserve-test.html", "line": 293803, "message": null, "expected": "OK"}
{"status": "NOTRUN", "stack": null, "subtest": "Overall test", "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/canvas/framebuffer-bindings-unaffected-on-resize.html", "line": 294028, "message": null, "expected": "PASS"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/canvas/framebuffer-bindings-unaffected-on-resize.html", "line": 294039, "message": null, "expected": "OK"}
{"status": "NOTRUN", "stack": null, "subtest": "Overall test", "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/canvas/texture-bindings-unaffected-on-resize.html", "line": 294263, "message": null, "expected": "PASS"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/canvas/texture-bindings-unaffected-on-resize.html", "line": 294268, "message": null, "expected": "OK"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/extensions/oes-texture-float-with-canvas.html", "line": 296324, "message": null, "expected": "ERROR"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/extensions/oes-texture-half-float-with-canvas.html", "line": 296357, "message": null, "expected": "ERROR"}
{"status": "NOTRUN", "stack": null, "subtest": "Overall test", "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/extensions/webgl-compressed-texture-size-limit.html", "line": 296389, "message": null, "expected": "PASS"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/extensions/webgl-compressed-texture-size-limit.html", "line": 296391, "message": null, "expected": "OK"}
{"status": "NOTRUN", "stack": null, "subtest": "Overall test", "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/state/state-uneffected-after-compositing.html", "line": 325840, "message": null, "expected": "PASS"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/state/state-uneffected-after-compositing.html", "line": 325842, "message": null, "expected": "OK"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/textures/tex-image-and-sub-image-2d-with-canvas-rgb565.html", "line": 328225, "message": null, "expected": "ERROR"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/textures/tex-image-and-sub-image-2d-with-canvas-rgba4444.html", "line": 328235, "message": null, "expected": "ERROR"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/textures/tex-image-and-sub-image-2d-with-canvas-rgba5551.html", "line": 328717, "message": null, "expected": "ERROR"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/textures/tex-image-and-sub-image-2d-with-canvas.html", "line": 328853, "message": null, "expected": "ERROR"}
{"status": "NOTRUN", "stack": null, "subtest": "Overall test", "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/textures/tex-image-and-sub-image-2d-with-webgl-canvas-rgb565.html", "line": 329648, "message": null, "expected": "PASS"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/textures/tex-image-and-sub-image-2d-with-webgl-canvas-rgb565.html", "line": 329651, "message": null, "expected": "OK"}
{"status": "NOTRUN", "stack": null, "subtest": "Overall test", "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/textures/tex-image-and-sub-image-2d-with-webgl-canvas-rgba4444.html", "line": 329820, "message": null, "expected": "PASS"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/textures/tex-image-and-sub-image-2d-with-webgl-canvas-rgba4444.html", "line": 329823, "message": null, "expected": "OK"}
{"status": "NOTRUN", "stack": null, "subtest": "Overall test", "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/textures/tex-image-and-sub-image-2d-with-webgl-canvas-rgba5551.html", "line": 329841, "message": null, "expected": "PASS"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/textures/tex-image-and-sub-image-2d-with-webgl-canvas-rgba5551.html", "line": 329845, "message": null, "expected": "OK"}
{"status": "NOTRUN", "stack": null, "subtest": "Overall test", "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/textures/tex-image-and-sub-image-2d-with-webgl-canvas.html", "line": 329970, "message": null, "expected": "PASS"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/textures/tex-image-and-sub-image-2d-with-webgl-canvas.html", "line": 329973, "message": null, "expected": "OK"}
{"status": "NOTRUN", "stack": null, "subtest": "Overall test", "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/textures/texture-size-limit.html", "line": 330387, "message": null, "expected": "PASS"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/webgl/conformance-1.0.3/conformance/textures/texture-size-limit.html", "line": 330451, "message": null, "expected": "OK"}
{"status": "TIMEOUT", "stack": null, "subtest": "Concurrent requestAnimationFrame loops are deterministic", "action": "test_result", "test": "/_mozilla/mozilla/deterministic-raf.html", "line": 340405, "message": "Test timed out", "expected": "PASS"}
{"status": "TIMEOUT", "stack": null, "subtest": null, "action": "test_result", "test": "/_mozilla/mozilla/deterministic-raf.html", "line": 340406, "message": null, "expected": "OK"}

This patch fixes a couple of issues in the compositor: 1) Remove the delayed composition code. Previously, this would schedule a composite for 12ms in the future. This doesn't really make any sense with WR. There's no point in doing a composite unless WR has provided a new frame to be drawn. This fixes issues in several benchmarks where we were doing multiple composite / renders per rAF, which is a waste of CPU time. This *does* make the framerate slower in some cases (such as a slow rAF callback) but it's more correct - otherwise we were just compositing the same frame multiple times for no real benefit. 2) Inform the window of the current animation state of the compositor. Specifically, if an animation (or rAF) is currently active, the window system switches to use event polling, and does not block on the OS-level event loop. In the case of active animation, we just assume that we want to be running as the vsync interval and not blocking. This means the compositor thread only sleeps on vsync during animation, which reduces OS scheduling and results in much smoother animation.

yet considered spurious force a reflow / composite / animation tick cycle.

glennw · 2017-06-20T02:54:21Z

Removing the delayed compositor timer exposed a bug in how we handle rAF callbacks that don't mutate the DOM. Added a second commit to handle that.

r? @jdm

glennw · 2017-06-20T02:54:35Z

(or anyone else who knows the DOM code well enough to review that).

jdm · 2017-06-20T05:11:05Z

@bors-servo: r+

bors-servo · 2017-06-20T05:11:06Z

📌 Commit 6c9e305 has been approved by jdm

Improve decisions in compositor over when to draw a frame. This patch fixes a couple of issues in the compositor: 1) Remove the delayed composition code. Previously, this would schedule a composite for 12ms in the future. This doesn't really make any sense with WR. There's no point in doing a composite unless WR has provided a new frame to be drawn. This fixes issues in several benchmarks where we were doing multiple composite / renders per rAF, which is a waste of CPU time. This *does* make the framerate slower in some cases (such as a slow rAF callback) but it's more correct - otherwise we were just compositing the same frame multiple times for no real benefit. 2) Inform the window of the current animation state of the compositor. Specifically, if an animation (or rAF) is currently active, the window system switches to use event polling, and does not block on the OS-level event loop. In the case of active animation, we just assume that we want to be running as the vsync interval and not blocking. This means the compositor thread only sleeps on vsync during animation, which reduces OS scheduling and results in much smoother animation.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17398)

bors-servo · 2017-06-20T05:14:15Z

⌛ Testing commit 6c9e305 with merge 4530717...

bors-servo · 2017-06-20T06:12:18Z

💔 Test failed - linux-rel-wpt

glennw · 2017-06-20T08:41:27Z

@bors-servo r-

I'm seeing a few demos not animating correctly locally. I need to investigate more to see if it's related to this change.

This fixes another rAF bug, that is being exposed by the previous two commits. Previously, the fake timer callback would only be set if we were being called from a non-rAF event handler. Now, if we're in fake / spurious mode, unconditionally set the one shot timer. Otherwise, notify the compositor that animations are present if we're not currently in a rAF callback.

glennw · 2017-06-21T01:04:50Z

Added a follow up commit that addresses an issue I ran into.

r? @jdm

jdm · 2017-06-21T02:04:50Z

@bors-servo: r+

bors-servo · 2017-06-21T02:04:51Z

📌 Commit cbeb181 has been approved by jdm

bors-servo · 2017-06-21T06:13:32Z

⌛ Testing commit cbeb181 with merge 22ddd0f...

Improve decisions in compositor over when to draw a frame. This patch fixes a couple of issues in the compositor: 1) Remove the delayed composition code. Previously, this would schedule a composite for 12ms in the future. This doesn't really make any sense with WR. There's no point in doing a composite unless WR has provided a new frame to be drawn. This fixes issues in several benchmarks where we were doing multiple composite / renders per rAF, which is a waste of CPU time. This *does* make the framerate slower in some cases (such as a slow rAF callback) but it's more correct - otherwise we were just compositing the same frame multiple times for no real benefit. 2) Inform the window of the current animation state of the compositor. Specifically, if an animation (or rAF) is currently active, the window system switches to use event polling, and does not block on the OS-level event loop. In the case of active animation, we just assume that we want to be running as the vsync interval and not blocking. This means the compositor thread only sleeps on vsync during animation, which reduces OS scheduling and results in much smoother animation.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17398)

bors-servo · 2017-06-21T07:09:26Z

💔 Test failed - mac-rel-wpt4

jdm · 2017-06-21T13:33:15Z

@bors-servo: retry

servo-mac5 timed out on a bunch of builds but seems to be working fine now

bors-servo · 2017-06-21T14:47:54Z

⌛ Testing commit cbeb181 with merge 819a40b...

Improve decisions in compositor over when to draw a frame. This patch fixes a couple of issues in the compositor: 1) Remove the delayed composition code. Previously, this would schedule a composite for 12ms in the future. This doesn't really make any sense with WR. There's no point in doing a composite unless WR has provided a new frame to be drawn. This fixes issues in several benchmarks where we were doing multiple composite / renders per rAF, which is a waste of CPU time. This *does* make the framerate slower in some cases (such as a slow rAF callback) but it's more correct - otherwise we were just compositing the same frame multiple times for no real benefit. 2) Inform the window of the current animation state of the compositor. Specifically, if an animation (or rAF) is currently active, the window system switches to use event polling, and does not block on the OS-level event loop. In the case of active animation, we just assume that we want to be running as the vsync interval and not blocking. This means the compositor thread only sleeps on vsync during animation, which reduces OS scheduling and results in much smoother animation.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17398)

bors-servo · 2017-06-21T16:28:39Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 819a40b to master...

highfive assigned metajack Jun 19, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 19, 2017

highfive assigned pcwalton and unassigned metajack Jun 19, 2017

glennw force-pushed the opt-composite branch from 029d8f7 to 36829da Compare June 19, 2017 20:20

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 19, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jun 19, 2017

gw3583 added 2 commits June 20, 2017 08:47

Ensure that rAF callbacks which don't mutate the DOM, but aren't

6c9e305

yet considered spurious force a reflow / composite / animation tick cycle.

glennw force-pushed the opt-composite branch from 36829da to 6c9e305 Compare June 20, 2017 02:53

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jun 20, 2017

highfive assigned jdm and unassigned pcwalton Jun 20, 2017

highfive added the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jun 20, 2017

jdm mentioned this pull request Jun 20, 2017

Make requestAnimationFrame callback execution easier to reason about #17423

Closed

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jun 20, 2017

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jun 21, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jun 21, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jun 21, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Jun 21, 2017

bors-servo merged commit cbeb181 into servo:master Jun 21, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jun 21, 2017

bors-servo mentioned this pull request Jun 21, 2017

Remove compositor forwarding code and use dedicated mechanism #17269

Merged

5 tasks

Uh oh!

Conversation

glennw commented Jun 19, 2017 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glennw commented Jun 19, 2017

Uh oh!

glennw commented Jun 19, 2017

Uh oh!

glennw commented Jun 19, 2017

Uh oh!

glennw commented Jun 19, 2017

Uh oh!

pcwalton commented Jun 19, 2017

Uh oh!

bors-servo commented Jun 19, 2017

Uh oh!

bors-servo commented Jun 19, 2017

Uh oh!

bors-servo commented Jun 19, 2017

Uh oh!

jdm commented Jun 19, 2017

Uh oh!

glennw commented Jun 20, 2017

Uh oh!

glennw commented Jun 20, 2017

Uh oh!

jdm commented Jun 20, 2017

Uh oh!

bors-servo commented Jun 20, 2017

Uh oh!

bors-servo commented Jun 20, 2017

Uh oh!

bors-servo commented Jun 20, 2017

Uh oh!

glennw commented Jun 20, 2017

Uh oh!

glennw commented Jun 21, 2017

Uh oh!

jdm commented Jun 21, 2017

Uh oh!

bors-servo commented Jun 21, 2017

Uh oh!

bors-servo commented Jun 21, 2017

Uh oh!

bors-servo commented Jun 21, 2017

Uh oh!

jdm commented Jun 21, 2017

Uh oh!

bors-servo commented Jun 21, 2017

Uh oh!

bors-servo commented Jun 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

glennw commented Jun 19, 2017 •

edited by larsbergstrom

Loading