1416995 - Enable retained mode for the basic layer manager in fallback path

Status: RESOLVED FIXED

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

Description

[Ethan Lin[:ethlin]]

I notice that the basic layer manager in fallback path repaints everything, though the invalidate region is correct. We should enable the retained mode for the basic layer manager.

Comment 1

[Ethan Lin[:ethlin]]

Attachment: Bug 1416995 - Enable retained mode for the basic layer manager in WR fallback.

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

Comment 2

[Ethan Lin[:ethlin]]

I tested the patch on my local device.
Before the fix, the result of hixie-001.xml in talos-svgx is:

INFO -  PID 53076 | [#0] /hixie-001.xml  Cycles:25  Average:1663.16  Median:1665.00  stddev:120.40 (7.2%)  stddev-sans-first:122.99
INFO -  PID 53076 | Values: 1665.0  1555.0  1508.0  1536.0  1519.0  1595.0  1686.0  1507.0  1855.0  1622.0  1795.0  1868.0  1801.0  1696.0  1669.0  1636.0  1713.0  1607.0  1727.0  1783.0  1694.0  1888.0  1523.0  1645.0  1486.0

With the patch, the result is:
INFO -  PID 52133 | [#0] /hixie-001.xml  Cycles:25  Average:971.48  Median:801.00  stddev:97.50 (12.2%)  stddev-sans-first:93.55
INFO -  PID 52133 | Values: 1132.0  1000.0  958.0  1003.0  956.0  989.0  976.0  1018.0  1017.0  1169.0  1066.0  1007.0  953.0  972.0  986.0  950.0  1011.0  986.0  940.0  1062.0  976.0  826.0  774.0  759.0  801.0

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8928097 [details]
Bug 1416995 - Enable retained mode for the basic layer manager in WR fallback.

https://reviewboard.mozilla.org/r/199344/#review204494

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8928097 [details]
Bug 1416995 - Enable retained mode for the basic layer manager in WR fallback.

I'm unreviewing this after talking with mstange. I'd like to better understand what's going on with this enabled. Will it cause our DrawTargetRecording's to grow without bound?

Comment 5

[Ethan Lin[:ethlin]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> Comment on attachment 8928097 [details]
> Bug 1416995 - Enable retained mode for the basic layer manager in WR
> fallback.
> 
> I'm unreviewing this after talking with mstange. I'd like to better
> understand what's going on with this enabled. Will it cause our
> DrawTargetRecording's to grow without bound?

Ah...I think we should just do it for non-Blobimage. The SVG performance is still worse than non-WebRender. I will keep investigating.

Comment 6

[Ethan Lin[:ethlin]]

Attachment: Bug 1416995 - Update annotations for related reftests.

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

Comment 7

[Ethan Lin[:ethlin]]

Comment on attachment 8928097 [details]
Bug 1416995 - Enable retained mode for the basic layer manager in WR fallback.

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

Comment 8

[Ethan Lin[:ethlin]]

I think we still can land this for non-Blobimage case.
I updated the annotations based on this try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4b94281ccb208fb8cfb30fadc58317c1885d3b5.

Comment 9

[Ethan Lin[:ethlin]]

Originally I wanted to find another way to handle invalidation for both blobimage and non-blobimage. But it seems that the invalidation mechanism bundles painted layer and content client. So I want to use the original mechanism in basic layer manager, though we can only enable it for non-blobimage. With this fix, the result of svg in talos test is very close to original gecko's.

Comment 10

[Ethan Lin[:ethlin]]

Comment on attachment 8928097 [details]
Bug 1416995 - Enable retained mode for the basic layer manager in WR fallback.

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

Comment 11

[Ethan Lin[:ethlin]]

Comment on attachment 8928446 [details]
Bug 1416995 - Update annotations for related reftests.

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

Comment 12

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

Comment on attachment 8928097 [details]
Bug 1416995 - Enable retained mode for the basic layer manager in WR fallback.

https://reviewboard.mozilla.org/r/199344/#review208176

::: gfx/layers/wr/WebRenderCommandBuilder.cpp:345
(Diff revision 3)
>  }
>  
>  static bool
>  PaintByLayer(nsDisplayItem* aItem,
>               nsDisplayListBuilder* aDisplayListBuilder,
>               RefPtr<BasicLayerManager>& aManager,

Since you're not modifiying aManager inside this function anymore, can you change this to a const-ref?

const RefPtr<BasicLayerManager>& aManager

::: gfx/layers/wr/WebRenderCommandBuilder.cpp:406
(Diff revision 3)
>  PaintItemByDrawTarget(nsDisplayItem* aItem,
>                        gfx::DrawTarget* aDT,
>                        const LayerRect& aImageRect,
>                        const LayoutDevicePoint& aOffset,
>                        nsDisplayListBuilder* aDisplayListBuilder,
>                        RefPtr<BasicLayerManager>& aManager,

Ditto here

Comment 13

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

Comment on attachment 8928446 [details]
Bug 1416995 - Update annotations for related reftests.

https://reviewboard.mozilla.org/r/199688/#review208178

Comment 14

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> Comment on attachment 8928097 [details]
> Bug 1416995 - Enable retained mode for the basic layer manager in WR
> fallback.
> 
> https://reviewboard.mozilla.org/r/199344/#review208176
> 
> ::: gfx/layers/wr/WebRenderCommandBuilder.cpp:345
> (Diff revision 3)
> >  }
> >  
> >  static bool
> >  PaintByLayer(nsDisplayItem* aItem,
> >               nsDisplayListBuilder* aDisplayListBuilder,
> >               RefPtr<BasicLayerManager>& aManager,
> 
> Since you're not modifiying aManager inside this function anymore, can you
> change this to a const-ref?
> 
> const RefPtr<BasicLayerManager>& aManager
> 
> ::: gfx/layers/wr/WebRenderCommandBuilder.cpp:406
> (Diff revision 3)
> >  PaintItemByDrawTarget(nsDisplayItem* aItem,
> >                        gfx::DrawTarget* aDT,
> >                        const LayerRect& aImageRect,
> >                        const LayoutDevicePoint& aOffset,
> >                        nsDisplayListBuilder* aDisplayListBuilder,
> >                        RefPtr<BasicLayerManager>& aManager,
> 
> Ditto here

Okay, I will change it to const-ref.

Comment 15

[Ethan Lin[:ethlin]]

Comment on attachment 8928446 [details]
Bug 1416995 - Update annotations for related reftests.

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

Comment 16

[Ethan Lin[:ethlin]]

Comment on attachment 8928097 [details]
Bug 1416995 - Enable retained mode for the basic layer manager in WR fallback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199344/diff/3-4/

Comment 17

[Ethan Lin[:ethlin]]

Comment on attachment 8928446 [details]
Bug 1416995 - Update annotations for related reftests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199688/diff/3-4/

Comment 18

[Ethan Lin[:ethlin]]

Comment on attachment 8928097 [details]
Bug 1416995 - Enable retained mode for the basic layer manager in WR fallback.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199344/diff/4-5/

Comment 19

[Ethan Lin[:ethlin]]

Comment on attachment 8928446 [details]
Bug 1416995 - Update annotations for related reftests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/199688/diff/4-5/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92ac9fd6870d
Enable retained mode for the basic layer manager in WR fallback. r=jrmuizel,kats
https://hg.mozilla.org/integration/autoland/rev/9e8fb11cb40a
Update annotations for related reftests. r=kats

Comment 21

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/92ac9fd6870d
https://hg.mozilla.org/mozilla-central/rev/9e8fb11cb40a