Closed Bug 1416995 Opened 2 years ago Closed 2 years ago

Enable retained mode for the basic layer manager in fallback path

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: ethlin, Assigned: ethlin)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [wr-mvp])

Attachments

(2 files)

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.
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
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [wr-mvp]
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
Attachment #8928097 - Flags: review?(jmuizelaar) → review+
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?
Attachment #8928097 - Flags: review+ → review?(jmuizelaar)
(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.
Attachment #8928097 - Flags: review?(jmuizelaar)
Attachment #8928097 - Flags: review?(jmuizelaar)
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.
Attachment #8928097 - Flags: review?(jmuizelaar)
Attachment #8928446 - Flags: review?(jmuizelaar)
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.
Attachment #8928097 - Flags: review?(jmuizelaar)
Attachment #8928446 - Flags: review?(jmuizelaar)
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
Attachment #8928097 - Flags: review?(bugmail) → review+
Comment on attachment 8928446 [details]
Bug 1416995 - Update annotations for related reftests.

https://reviewboard.mozilla.org/r/199688/#review208178
Attachment #8928446 - Flags: review?(bugmail) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Pushed by ethlin@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/92ac9fd6870d
https://hg.mozilla.org/mozilla-central/rev/9e8fb11cb40a
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.