Closed Bug 1452845 Opened Last year Closed Last year

Ensure all tests pass with async scene building enabled

Categories

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

Other Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: kats, Assigned: kats)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

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
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:kats@mozilla.com) 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
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.
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
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 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=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 kgupta@mozilla.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
https://hg.mozilla.org/mozilla-central/rev/24e5bb07b1ee
https://hg.mozilla.org/mozilla-central/rev/ede2ec96fe1e
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
(In reply to Pulsebot from comment #24)
> Pushed by kgupta@mozilla.com:
> 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: 1471497
You need to log in before you can comment on or make changes to this bug.