1479075 - FlushRenderingAsync is wrong

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

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

The "async" version of FlushRendering just means that the IPC message is async. The compositor is still expected to flush any pending paints. However, we implement it totally differently on the WR side [1], in that we don't flush anything. So if we had previously gotten a new display list, it doesn't get necessarily get flushed and we might just re-composite whatever we were showing before.

This affects the tresize test with my patches for servo/webrender#2807, because as of bug 1476876 we are using the async FlushRendering - what happens is the UI process sends a new display list to the compositor and then calls FlushRendering. A lot of the time the new display list doesn't even get built into a scene before the FlushRendering runs and recomposites the old stuff. Then the display list ends up having to wait until the next vsync before it gets rendered, which inflates the tresize numbers.

[1] https://searchfox.org/mozilla-central/rev/d47c829065767b6f36d29303d650bffb7c4f94a3/gfx/layers/ipc/CompositorBridgeParent.cpp#619

Comment 1

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

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

Comment 2

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

The talos results are showing regressions in tpaint and sessionrestore_many_windows, which unsurprisingly are two of the tests that were helped by bug 1476876. As with bug 1471962 I think we need to take these regressions for correctness.

Comment 3

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

Attachment: Bug 1479075 - Ensure FlushRenderingAsync actually flushes the compositor.

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

Comment 4

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8995657 [details]
Bug 1479075 - Ensure FlushRenderingAsync actually flushes the compositor.

https://reviewboard.mozilla.org/r/260038/#review267118

Looks good.

Comment 5

[Sotaro Ikeda [:sotaro]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #0)
> The "async" version of FlushRendering just means that the IPC message is
> async. The compositor is still expected to flush any pending paints.
> However, we implement it totally differently on the WR side [1], in that we
> don't flush anything. So if we had previously gotten a new display list, it
> doesn't get necessarily get flushed and we might just re-composite whatever
> we were showing before.

From the above, FlushFramePresentation() in WebRenderBridgeParent::FlushRendering() during RecvFlushRenderingAsync() might not be necessary.

Comment 6

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

Yeah, that's a good point, we shouldn't need to wait for the presentation on this codepath. I'll update the patch.

Comment 7

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

Comment on attachment 8995657 [details]
Bug 1479075 - Ensure FlushRenderingAsync actually flushes the compositor.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2799f012459c
Ensure FlushRenderingAsync actually flushes the compositor. r=sotaro

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/2799f012459c