1452845 - Ensure all tests pass with async scene building enabled

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Kartikaya Gupta (email:[e-mail])]

At least one that we need to do (as noted by Botond in bug 1449982 comment 75) is to fix the comment/assertion in UpdateZoomConstraints at [1]. With async scene building enabled, the content process can send a UpdateZoomConstraints message over PAPZCTreeManager, which arrives in APZCTreeManagerParent on the compositor thread, which then calls APZCTreeManager::UpdateZoomConstraints on the compositor thread. Since the compositor thread != the updater thread when async scene building is enabled, it means that codepath can be taken in the GPU process as well, not just in the UI process. So the assertion is not valid any more, and the comment needs updating.

[1] https://searchfox.org/mozilla-central/rev/b55e1a1cbcaee34878e133fbac20c4c2af6e11b5/gfx/layers/apz/src/APZCTreeManager.cpp#1976-1979

Comment 1

[Kartikaya Gupta (email:[e-mail])]

A recent try push with it enabled has much orange:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5317587b3b96f40aad73bb55b42441eec995547a

There's a lot of reftest wonkiness, presumably because the async scene build screws with "sync composites". And a bunch of assertion failures from code that I added which I'll need to investigate.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #1)
> There's a lot of reftest wonkiness, presumably because the async scene build
> screws with "sync composites".

Most likely we'll need to flush any inflight async scene builds before [1] to fix this.

[1] https://searchfox.org/mozilla-central/rev/4114ad2cfcbc511705c7865a4a34741812f9a2a9/gfx/layers/wr/WebRenderBridgeParent.cpp#811

Comment 3

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5c20f06c65463694362230425f872e4803a8b6b

This should hopefully fix the assertion failures and leaks (I was seeing leaks locally). Reftests are still going to be busted.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

That try push didn't actually enable async scene building, this one does: https://treeherder.mozilla.org/#/jobs?repo=try&revision=49595ead4ecf90aea25c9006469d9a43acefefd9

Comment 5

[Kartikaya Gupta (email:[e-mail])]

With more fixes: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d93667e47e271bd9e4ed34509403447ea5799afc

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1452845 - Fix up snapshotting implementation with async-scene-building enabled.

Review commit: https://reviewboard.mozilla.org/r/241176/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241176/

Comment 7

[Botond Ballo [:botond]]

Comment on attachment 8972619 [details]
Bug 1452845 - Fix up snapshotting implementation with async-scene-building enabled.

https://reviewboard.mozilla.org/r/241176/#review247052

Patch is fine, but wrong bug number?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Uhh whoops!

Comment 9

[Kartikaya Gupta (email:[e-mail])]

The WR update in bug 1459935 fixes a bunch of async-scene-building crashes. The patches from bug 1457466 expose some other issues that I have WIPs for. Try push with everything so far is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=84543c4a590c4dd5d363aa7c92c26f3e07ec1848

Comment 10

[Kartikaya Gupta (email:[e-mail])]

(With just the WR update and ASB enabled, the try push is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e08515974664869e3eea9a0ffdde8ab2ad9f9113)

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Most recent try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebcb6fec5534574a1f75278d39d0e63428b06ac0 still has some failures.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Finally I have something I think is correct, and looks green (so far): https://treeherder.mozilla.org/#/jobs?repo=try&revision=abe896785893f58feab65ddb65d2701c6052144b

Will clean it up and post patches.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0da49e5875cfb2540cc9c8998bfa1229ed680aa2

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1452845 - Turn on async scene building by default.

Review commit: https://reviewboard.mozilla.org/r/246582/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246582/

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8972619 [details]
Bug 1452845 - Fix up snapshotting implementation with async-scene-building enabled.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/241176/diff/1-2/

Comment 16

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8972619 [details]
Bug 1452845 - Fix up snapshotting implementation with async-scene-building enabled.

https://reviewboard.mozilla.org/r/241176/#review252724

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8980411 [details]
Bug 1452845 - Turn on async scene building by default.

https://reviewboard.mozilla.org/r/246582/#review252726

Comment 18

[Aria Beingessner [:Gankra]]

Was your one-week estimate just assuming you would run into a lot more trouble?

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Yeah, my one-week estimate was on the assumption that I would actually need to fix bug 1464086. Turns out we don't need to in order to turn this on. That being said it looks like there's still some orange that might be related on the try push. Either I didn't fix all the problems or I screwed up something during patch cleanup.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Here's the most recent try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1b7fc2e95e9becec455279591bd28c8eccf5f525

This includes the patches from bug 1417784. There's a few issues turning up in this push that aren't pre-existing intermittents. One is the R3 failures in webgl-color-test, which I suspect will be fixed by bug 1463313. Then there's the M2 test_fullscreen-api-race.html failure which I need to investigate. And there's a solitary M5 test_mousecapture.xhtml failure which might just be random but that I'm not convinced about yet.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1161ec64b207597ddb6890bf427d4d40bb8ba1da has the patches from bug 1463313 and it does seem to address the webgl-color-test failures.

Comment 22

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ea2f1d628127efd04348ff4810b245d8b48a3be6 has all the inflight stuff. Looking decent so far. I'll look into the tp5o_webext timeout in g5.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

After retriggering g5 a bunch it didn't seem to persist. I also did a logging push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=1bff67aa7d566a327422975dfee517cbf4213455 and it was all green. So I'll assume it's good for now, and deal with it if it turns up as a high-frequency after landing these patches.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24e5bb07b1ee
Fix up snapshotting implementation with async-scene-building enabled. r=botond,jrmuizel
https://hg.mozilla.org/integration/autoland/rev/ede2ec96fe1e
Turn on async scene building by default. r=jrmuizel

Comment 25

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/24e5bb07b1ee
https://hg.mozilla.org/mozilla-central/rev/ede2ec96fe1e

Comment 26

[Botond Ballo [:botond]]

(In reply to Pulsebot from comment #24)
> Pushed by [e-mail]:
> https://hg.mozilla.org/integration/autoland/rev/24e5bb07b1ee
> Fix up snapshotting implementation with async-scene-building enabled.
> r=botond,jrmuizel

I don't recall reviewing this :)

Comment 27

[Kartikaya Gupta (email:[e-mail])]

I never even flagged you for review on this patch! I guess it's a good thing we're decommissioning MozReview...

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Oh, I see what happened, it's from comment 6 wherein I accidentally submitted the patch from bug 1458259 to this bug. MozReview decided to carry the r+ to the new and totally different patch.