1471962 - Flush scene builder when as part of flushing rendering

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P2)

Description

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

We get sync FlushRendering calls during window resize, and the expectation is that when that call finishes the rendering will have updated to the new window size. However currently WebRenderBridgeParent doesn't flush the scene builder, it has a TODO about that. If we don't flush the scene builder we could just end up re-rendering the old scene which isn't at the new window size.

This also addresses the tresize regression I was seeing with the latest version of my patches for servo/webrender#2807.

Comment 1

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

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

Comment 2

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

Well. This helps tresize on linux, but hurts tpaint, ts_paint, ts_paint_heavy, ts_paint_webext on windows. By a lot.

Comment 3

[Markus Stange [:mstange]]

Could this be because those tests have expensive content scenes? That would make this an instance of the problem mentioned in bug 1464086 comment 1.

Comment 4

[Markus Stange [:mstange]]

Oh, I see now that you've filed bug 1471562 about that problem.

Comment 5

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

It's also not clear to me under what circumstances we do a sync FlushRendering call. It surprised me that we're doing that so much in talos tests like tpaint, but we must be if making that slower causes regressions in these tests. I suspect that fixing the extra render described at [1] will help a bunch in solving the regression.

[1] https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/gfx/layers/wr/WebRenderBridgeParent.cpp#885

Comment 6

[Markus Stange [:mstange]]

I haven't checked the code but the cases that I'm aware of where we flush rendering are:
 - window opening
 - window focus changes
 - window resizes

Comment 7

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

Yeah, it looks likes both tpaint and ts_paint spawn new windows, and so the fact that we're getting a rendering flush seems correct.

In the case of ts_paint, it records from process startup to the end of the first paint (at [1]), so that includes a sync rendering flush, but does NOT include the extra render that I mentioned in comment 5. So removing that render wouldn't help. Without the patch on this bug, the test is effectively measuring the wrong thing, because it hasn't included the display list that got sent over, because the flush doesn't actually flush the scene build. So the regression on ts_paint is a result of measuring things more correctly. I did also observe that the time from process startup to the first parent-process MozAfterPaint is reduced with the patch applied, because the MozAfterPaint is only fired when the epochs line up, and that happens sooner with the explicit scene building flush than without. So if the test were recording the time from process startup to first UI process MozAfterPaint it would register an improvement rather than a regression with this patch.

tpaint is a little different; it opens a child window which reports back on the first content MozAfterPaint and it records the time taken for that. However, from my investigation, it looks like the thing that is triggering the MozAfterPaint is the timer at [2] rather than actually painting that content. So that's totally wrong.

[1] https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/view/nsViewManager.cpp#361
[2] https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/layout/base/nsPresContext.cpp#3303

Comment 8

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

Oho! So it looks like with layers, we send a DidComposite for content-process layers even if those layers are not attached to the parent process layers. i.e. say we create a new tab, and we create the associated RenderFrameParent in the UI process. That RenderFrameParent doesn't actually get painted right away; some code (I presume the front-end) keeps it deactivated somehow until it knows that the content process layers have been painted via a MozAfterPaint, and then it activates the RenderFrameParent.

With WR, that MozAfterPaint doesn't happen because, well, the display list wasn't rendered and so the PipelineInfo doesn't contain the epoch for the content process pipeline. After 100ms that timer expires, fires the MozAfterPaint, and then that allows the front-end to activate the RenderFrameParent. If I increase the timer from 100ms to 1000ms the exact same thing happens - it takes 1000ms before the MozAfterPaint fires and unblocks the painting.

So I think for consistency with layers we need to make it so that even if a content process display list isn't attached to the parent process we need to send a MozAfterPaint for it.

Comment 9

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

The patches I have for bug 1473352 help somewhat with the tpaint regression, but it's still around 60% on windows (was >90% without those patches, in comment 1). It's still around 60-70% on ts_paint*.

Try push with a hacky version of the changes needed for bug 1473352, plus the FlushRendering change: https://treeherder.mozilla.org/#/jobs?repo=try&revision=456477f60f9b35e9e087f37100d4724f64a33b53

Comment 10

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

I've been trying to improve the talos numbers on these tests so that we can land the patch for this bug without making the regression worse. However I haven't been very successful. Most of the time seems to be spent in binding the shaders for the new window, as that happens on-demand for the window's first composite. I tried to make it happen a bit earlier as part of an "async setup" step that is triggered on the renderer thread after we create the renderer but before the first composite. It didn't really help since it still takes long enough that it pushes the composite back.

Comment 11

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

Attachment: Bug 1471962 - Flush scene builds when flushing rendering.

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

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8992672 [details]
Bug 1471962 - Flush scene builds when flushing rendering.

https://reviewboard.mozilla.org/r/257528/#review264414

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83724fe36744
Flush scene builds when flushing rendering. r=jrmuizel

Comment 14

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/83724fe36744

Comment 15

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

Moving dependencies to the talos regression bug.