Closed Bug 1452845 Opened 3 years ago Closed 3 years ago
Ensure all tests pass with async scene building enabled
59 bytes, text/x-review-board-request
59 bytes, text/x-review-board-request
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 . 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.  https://searchfox.org/mozilla-central/rev/b55e1a1cbcaee34878e133fbac20c4c2af6e11b5/gfx/layers/apz/src/APZCTreeManager.cpp#1976-1979
3 years ago
Priority: -- → P1
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.
(In reply to Kartikaya Gupta (email:firstname.lastname@example.org) 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  to fix this.  https://searchfox.org/mozilla-central/rev/4114ad2cfcbc511705c7865a4a34741812f9a2a9/gfx/layers/wr/WebRenderBridgeParent.cpp#811
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.
That try push didn't actually enable async scene building, this one does: https://treeherder.mozilla.org/#/jobs?repo=try&revision=49595ead4ecf90aea25c9006469d9a43acefefd9
Depends on: 1455302
Depends on: 1455387
Depends on: 1455715
Depends on: 1457448
Depends on: 1458259
Attachment #8972619 - Attachment is obsolete: true
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
(With just the WR update and ASB enabled, the try push is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e08515974664869e3eea9a0ffdde8ab2ad9f9113)
Depends on: 1460988
Most recent try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebcb6fec5534574a1f75278d39d0e63428b06ac0 still has some failures.
Depends on: 1417784
Depends on: 1464086
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.
No longer depends on: 1464086
Comment on attachment 8972619 [details] Bug 1452845 - Fix up snapshotting implementation with async-scene-building enabled. https://reviewboard.mozilla.org/r/241176/#review252724
Attachment #8972619 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8980411 [details] Bug 1452845 - Turn on async scene building by default. https://reviewboard.mozilla.org/r/246582/#review252726
Attachment #8980411 - Flags: review?(jmuizelaar) → review+
Was your one-week estimate just assuming you would run into a lot more trouble?
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.
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.
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.
Depends on: 1463313
Depends on: 1464908
Depends on: 1465078
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.
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.
Pushed by email@example.com: 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
(In reply to Pulsebot from comment #24) > Pushed by firstname.lastname@example.org: > 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 :)
I never even flagged you for review on this patch! I guess it's a good thing we're decommissioning MozReview...
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.
Depends on: 1467429
Depends on: 1465658
You need to log in before you can comment on or make changes to this bug.