1434243 - Investigate unifying nsDisplayLayerEventRegions and nsDisplayCompositorHitTestInfo

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement)

Description

[Miko Mynttinen]

Managing frame/rect pairs in nsDisplayLayerEventRegions is very slow, especially during display list merging phase. They also create subtle bugs related to frame/display item lifetime.
For these reasons, and to avoid duplicating code, it would be nice if we could use nsDisplayCompositorHitTestInfo to track these regions also outside of WebRender.

Because nsDisplayCompositorHitTestInfo are created for all frames, this will definitely add more display items to display lists, but faster merging might make up for this.

Depending on the performance, fixing bug 1425863 might be needed before landing.

Comment 1

[Miko Mynttinen]

Attachment: Bug 1434243 - Part 1: Ensure that AncestorHasApzAwareEventHandler is set when needed if APZ is enabled

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

Comment 2

[Miko Mynttinen]

Attachment: Bug 1434243 - Part 2: Add nsDisplayCompositorHitTestInfo support to FrameLayerBuilder

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

Comment 3

[Miko Mynttinen]

As expected, having a separate hit test item for every frame slows down FrameLayerBuilder a lot (since nsDisplayLayerEventRegions::AddFrame logic was moved there).
The total regression with Displaylist_mutate talos test is almost 40%, when compared to current nsDisplayLayerEventRegions solution.

Comment 4

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

Comment on attachment 8947884 [details]
Bug 1434243 - Part 2: Add nsDisplayCompositorHitTestInfo support to FrameLayerBuilder

https://reviewboard.mozilla.org/r/217554/#review223346

::: gfx/thebes/gfxPrefs.h:513
(Diff revision 1)
>    DECL_GFX_PREF(Once, "gfx.webrender.enabled",                 WebRenderEnabledDoNotUseDirectly, bool, false);
>    DECL_OVERRIDE_PREF(Live, "gfx.webrender.blob-images",        WebRenderBlobImages, gfxPrefs::WebRenderAll());
>    DECL_GFX_PREF(Live, "gfx.webrender.highlight-painted-layers",WebRenderHighlightPaintedLayers, bool, false);
>    DECL_GFX_PREF(Live, "gfx.webrender.hit-test",                WebRenderHitTest, bool, true);
>  
> +  DECL_GFX_PREF(Live, "gfx.webrender.hit-test-with-flb",       WebRenderHitTestWithFLB, bool, true);

Since we never actually use FLB when webrender is enabled, I'd prefer renaming this pref to something like "layout.hit-test-info.enabled" (analogous to layout.event-regions.enabled) rather than make it a gfx/webrender-related pref. Flipping this pref should not affect the webrender codepath at all.

::: layout/painting/nsDisplayList.cpp:5045
(Diff revision 1)
>        });
>  
>    // Insert a transparent rectangle with the hit-test info
>    aBuilder.SetHitTestInfo(scrollId, mHitTestInfo);
> -  wr::LayoutRect rect = aSc.ToRelativeLayoutRect(mArea);
> +
> +  const int32_t factor = mFrame->PresContext()->AppUnitsPerDevPixel();

Note that we intentionally avoid trying to hit the frame during CreateWebRenderCommands, see bug 1424637

Comment 5

[Miko Mynttinen]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Comment on attachment 8947884 [details]
> Bug 1434243 - Part 2: Add nsDisplayCompositorHitTestInfo support to
> FrameLayerBuilder
> 
> https://reviewboard.mozilla.org/r/217554/#review223346
> 
> ::: gfx/thebes/gfxPrefs.h:513
> (Diff revision 1)
> >    DECL_GFX_PREF(Once, "gfx.webrender.enabled",                 WebRenderEnabledDoNotUseDirectly, bool, false);
> >    DECL_OVERRIDE_PREF(Live, "gfx.webrender.blob-images",        WebRenderBlobImages, gfxPrefs::WebRenderAll());
> >    DECL_GFX_PREF(Live, "gfx.webrender.highlight-painted-layers",WebRenderHighlightPaintedLayers, bool, false);
> >    DECL_GFX_PREF(Live, "gfx.webrender.hit-test",                WebRenderHitTest, bool, true);
> >  
> > +  DECL_GFX_PREF(Live, "gfx.webrender.hit-test-with-flb",       WebRenderHitTestWithFLB, bool, true);
> 
> Since we never actually use FLB when webrender is enabled, I'd prefer
> renaming this pref to something like "layout.hit-test-info.enabled"
> (analogous to layout.event-regions.enabled) rather than make it a
> gfx/webrender-related pref. Flipping this pref should not affect the
> webrender codepath at all.
At the moment it shouldn't affect webrender codepath (unless I made a mistake). And you are right, having webrender in the name when the code does not interact with it is confusing.

> ::: layout/painting/nsDisplayList.cpp:5045
> (Diff revision 1)
> >        });
> >  
> >    // Insert a transparent rectangle with the hit-test info
> >    aBuilder.SetHitTestInfo(scrollId, mHitTestInfo);
> > -  wr::LayoutRect rect = aSc.ToRelativeLayoutRect(mArea);
> > +
> > +  const int32_t factor = mFrame->PresContext()->AppUnitsPerDevPixel();
> 
> Note that we intentionally avoid trying to hit the frame during
> CreateWebRenderCommands, see bug 1424637

I was actually thinking of this yesterday from a more general perspective.

Currently we have 34 calls to mFrame->PresContext()->AppUnitsPerDevPixel() in nsDisplayList.cpp alone. Each of these calls results in pretty lengthy call chain of frame->PresContext()->AppUnitsPerDevPixel()->mDeviceContext->AppUnitsPerDevPixel()->mAppUnitsPerDevPixel, which is definitely not helping the current cache problems during display list building. Sadly my initial local testing showed no performance gain from having AppUnitsPerDevPixel as a member in nsDisplayItem. It is probably still worth doing, even just for the sake of reducing copy-paste code.

I will fix these and request a proper review later, after I manage to get rid of huge performance regression. Thank you for the feedback!

Comment 6

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

(In reply to Miko Mynttinen [:miko] from comment #5)
> Sadly my
> initial local testing showed no performance gain from having
> AppUnitsPerDevPixel as a member in nsDisplayItem. It is probably still worth
> doing, even just for the sake of reducing copy-paste code.

Hm, interesting. Just as a FYI we also bug 1424968 open for improving this on the webrender side a bit more.

> I will fix these and request a proper review later, after I manage to get
> rid of huge performance regression.

Sounds good :)

Comment 7

[Miko Mynttinen]

Attachment: Bug 1434243 - Part 3: Make nsDisplayListBuilder::ToReferenceFrame and nsDisplayListBuilder::FindReferenceFrame const

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

Comment 8

[Miko Mynttinen]

Attachment: Bug 1434243 - Part 4: Do not create unnecessary nsDisplayCompositorHitTestInfo items

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

Comment 9

[Miko Mynttinen]

Comment on attachment 8947883 [details]
Bug 1434243 - Part 1: Ensure that AncestorHasApzAwareEventHandler is set when needed if APZ is enabled

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

Comment 10

[Miko Mynttinen]

Comment on attachment 8947884 [details]
Bug 1434243 - Part 2: Add nsDisplayCompositorHitTestInfo support to FrameLayerBuilder

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

Comment 11

[Miko Mynttinen]

I have added two new boolean preferences for toggling this feature on and off.
layout.simple-event-region-items: use nsDisplayCompositorHitTestInfo instead of nsDisplayLayerEventRegions
layout.less-event-region-items: only create a new nsDisplayCompositorHitTestInfo if the parent frame did not have one created with same flags/area

The initial results look very promising. All the reftests and mochitests pass, and displaylist_mutate shows roughly 5% performance improvement on my MBP.

#mach talos-test -a displaylist_mutate --setpref layout.simple-event-region-items=false --setpref layout.less-event-region-items=false --tpcycles 5
[#0] /displaylist_mutate.html  Cycles:25  Average:4569.56  Median:4620.46  stddev:220.71 (4.8%)  stddev-sans-first:207.38
Values: 4153.9  4257.3  4234.8  4270.5  4243.8  4300.2  4357.4  4716.0  4646.8  4582.3  4663.0  4906.2  4702.7  4612.1  4712.3  4562.0  4728.1  4919.2  4528.0  4837.1  4685.7  4674.3  4564.4  4620.5  4760.2

#mach talos-test -a displaylist_mutate --setpref layout.simple-event-region-items=true --setpref layout.less-event-region-items=false --tpcycles 5
[#0] /displaylist_mutate.html  Cycles:25  Average:6478.22  Median:6510.30  stddev:232.88 (3.6%)  stddev-sans-first:237.20
Values: 6393.1  6112.4  5965.8  6591.8  6798.3  6592.6  6145.2  6718.2  6537.2  6749.6  6514.0  6101.9  6757.8  6401.5  6353.7  6534.3  6637.6  6421.7  6277.7  6494.7  6721.4  6299.0  6819.0  6510.3  6506.3

#mach talos-test -a displaylist_mutate --setpref layout.simple-event-region-items=true --setpref layout.less-event-region-items=true --tpcycles 5
[#0] /displaylist_mutate.html  Cycles:25  Average:4332.70  Median:4377.72  stddev:135.50 (3.1%)  stddev-sans-first:114.59
Values: 3967.9  4068.9  4410.4  4418.4  4363.6  4078.7  4436.3  4422.8  4436.9  4377.7  4422.8  4212.2  4293.3  4343.8  4491.1  4152.3  4395.4  4297.6  4350.3  4488.5  4384.9  4370.4  4324.1  4381.0  4428.1

Comment 12

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

Comment on attachment 8947883 [details]
Bug 1434243 - Part 1: Ensure that AncestorHasApzAwareEventHandler is set when needed if APZ is enabled

https://reviewboard.mozilla.org/r/217552/#review224190

Comment 13

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

Comment on attachment 8947884 [details]
Bug 1434243 - Part 2: Add nsDisplayCompositorHitTestInfo support to FrameLayerBuilder

https://reviewboard.mozilla.org/r/217554/#review224192

::: layout/painting/FrameLayerBuilder.cpp:4598
(Diff revision 2)
>        if (itemType == DisplayItemType::TYPE_LAYER_EVENT_REGIONS) {
>          nsDisplayLayerEventRegions* eventRegions =
>              static_cast<nsDisplayLayerEventRegions*>(item);
>          paintedLayerData->AccumulateEventRegions(this, eventRegions);
> +      } else if (itemType == DisplayItemType::TYPE_COMPOSITOR_HITTEST_INFO) {
> +          nsDisplayCompositorHitTestInfo* hitTestInfo =

nit: indentation

::: layout/painting/nsDisplayList.cpp:1031
(Diff revision 2)
>        mIsBuilding(false),
>        mInInvalidSubtree(false)
>  {
>    MOZ_COUNT_CTOR(nsDisplayListBuilder);
>  
> -  mBuildCompositorHitTestInfo = gfxVars::UseWebRender()
> +  const bool useWR = gfxPrefs::WebRenderHitTest() && gfxVars::UseWebRender();

nit: would prefer this variable to be called useWRHitTest

::: layout/painting/nsDisplayList.cpp:5047
(Diff revision 2)
>        });
>  
>    // Insert a transparent rectangle with the hit-test info
>    aBuilder.SetHitTestInfo(scrollId, mHitTestInfo);
> -  wr::LayoutRect rect = aSc.ToRelativeLayoutRect(mArea);
> +
> +  const int32_t factor = mFrame->PresContext()->AppUnitsPerDevPixel();

Getting the appUnitsPerDevPixel here is undesirable for perf (as mentioned before) but I don't want to hold up this patchset just for that. We can instead expand the scope of bug 1424968 to make sure we're not fetching this any more than needed anywhere during WR display list construction, and deal with it there.

Comment 14

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

Comment on attachment 8948978 [details]
Bug 1434243 - Part 4: Do not create unnecessary nsDisplayCompositorHitTestInfo items

https://reviewboard.mozilla.org/r/218122/#review224198

LGTM

::: layout/generic/nsFrame.cpp:2760
(Diff revision 1)
>    }
>    return Nothing();
>  }
>  
> +static void
> +BuildCompositorHitTestInfoIfNeeded(nsDisplayListBuilder* aBuilder,

Given how many times this function is calling aBuilder->something, it seems like it would be cleaner as a method of nsDisplayListBuilder instead of a static function. That will allow you to inline ShouldBuildCompositorHitTestInfo and SetCompositorHitTestInfo and eliminate some redundancies, like checking mBuildCompositorHitTestInfo twice (once at the top of this function and once inside ShouldBuildCompositorHitTestInfo).

::: layout/generic/nsFrame.cpp
(Diff revision 1)
> -        set.BorderBackground()->AppendToBottom(
> -            new (aBuilder) nsDisplayCompositorHitTestInfo(aBuilder, this, info));

My memory on this is a bit hazy, but I think one of the reasons I put this code down here was because I didn't want to accidentally put it in a different spot in the BorderBackground() list than where the event regions item was getting inserted (i.e. in case there are other things that get inserted into that list between your new call sites and this old call site). However, the order in the list probably doesn't matter anyway. So feel free to ignore this comment, I'm just braindumping :)

::: layout/painting/nsDisplayList.h:732
(Diff revision 1)
> +    mCompositorHitTestInfo = aHitTestInfo;
> +  }
> +
> +  bool ShouldBuildCompositorHitTestInfo(const nsIFrame* aFrame,
> +                                        const mozilla::gfx::CompositorHitTestInfo& aInfo,
> +                                        const bool aBuildNew = false) const;

nit: I'd prefer not having the default value on the last arg since all the call sites provide a value explicitly anyway, and there's not that many call sites.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8948977 [details]
Bug 1434243 - Part 3: Make nsDisplayListBuilder::ToReferenceFrame and nsDisplayListBuilder::FindReferenceFrame const

https://reviewboard.mozilla.org/r/218120/#review224396

Comment 16

[Miko Mynttinen]

Comment on attachment 8947883 [details]
Bug 1434243 - Part 1: Ensure that AncestorHasApzAwareEventHandler is set when needed if APZ is enabled

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

Comment 17

[Miko Mynttinen]

Comment on attachment 8947884 [details]
Bug 1434243 - Part 2: Add nsDisplayCompositorHitTestInfo support to FrameLayerBuilder

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

Comment 18

[Miko Mynttinen]

Comment on attachment 8948977 [details]
Bug 1434243 - Part 3: Make nsDisplayListBuilder::ToReferenceFrame and nsDisplayListBuilder::FindReferenceFrame const

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

Comment 19

[Miko Mynttinen]

Comment on attachment 8948978 [details]
Bug 1434243 - Part 4: Do not create unnecessary nsDisplayCompositorHitTestInfo items

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

Comment 20

[Miko Mynttinen]

Comment on attachment 8948978 [details]
Bug 1434243 - Part 4: Do not create unnecessary nsDisplayCompositorHitTestInfo items

https://reviewboard.mozilla.org/r/218122/#review224198

> Given how many times this function is calling aBuilder->something, it seems like it would be cleaner as a method of nsDisplayListBuilder instead of a static function. That will allow you to inline ShouldBuildCompositorHitTestInfo and SetCompositorHitTestInfo and eliminate some redundancies, like checking mBuildCompositorHitTestInfo twice (once at the top of this function and once inside ShouldBuildCompositorHitTestInfo).

I moved this to nsDisplayListBuilder.

> My memory on this is a bit hazy, but I think one of the reasons I put this code down here was because I didn't want to accidentally put it in a different spot in the BorderBackground() list than where the event regions item was getting inserted (i.e. in case there are other things that get inserted into that list between your new call sites and this old call site). However, the order in the list probably doesn't matter anyway. So feel free to ignore this comment, I'm just braindumping :)

At this point set.BorderBackground() list should be empty.

> nit: I'd prefer not having the default value on the last arg since all the call sites provide a value explicitly anyway, and there's not that many call sites.

Fixed.

Comment 21

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8947884 [details]
Bug 1434243 - Part 2: Add nsDisplayCompositorHitTestInfo support to FrameLayerBuilder

https://reviewboard.mozilla.org/r/217554/#review224674

Comment 22

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8948978 [details]
Bug 1434243 - Part 4: Do not create unnecessary nsDisplayCompositorHitTestInfo items

https://reviewboard.mozilla.org/r/218122/#review224678

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/95d02e4566c4
Part 1: Ensure that AncestorHasApzAwareEventHandler is set when needed if APZ is enabled r=kats
https://hg.mozilla.org/integration/autoland/rev/b338bd13a9e9
Part 2: Add nsDisplayCompositorHitTestInfo support to FrameLayerBuilder r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/48d8cb573cb5
Part 3: Make nsDisplayListBuilder::ToReferenceFrame and nsDisplayListBuilder::FindReferenceFrame const r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/5417b73364be
Part 4: Do not create unnecessary nsDisplayCompositorHitTestInfo items r=kats,mattwoodrow

Comment 24

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/95d02e4566c4
https://hg.mozilla.org/mozilla-central/rev/b338bd13a9e9
https://hg.mozilla.org/mozilla-central/rev/48d8cb573cb5
https://hg.mozilla.org/mozilla-central/rev/5417b73364be

Comment 25

[Ionuț Goldan [:igoldan]]

Performance improvements noticed! \o/

== Change summary for alert #11471 (as of Fri, 09 Feb 2018 09:43:32 GMT) ==

Improvements:

  3%  tsvgx windows7-32 opt e10s stylo     470.02 -> 455.18
  3%  tart windows7-32 opt e10s stylo      4.85 -> 4.70
  3%  tsvgx linux64 opt e10s stylo         217.07 -> 210.67
  3%  tart linux64 pgo e10s stylo          2.09 -> 2.03
  3%  tart linux64 opt e10s stylo          2.30 -> 2.23
  3%  tsvgx linux64 pgo e10s stylo         211.30 -> 205.57

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11471