Miko Mynttinen Assignee
	Description • 7 years ago

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.

	Miko Mynttinen Assignee
	Comment 3 • 7 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 4 • 7 years ago
mozreview-review

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

	Miko Mynttinen Assignee
	Comment 5 • 7 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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!

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 6 • 7 years ago

(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 :)

	Miko Mynttinen Assignee
	Comment 11 • 7 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 12 • 7 years ago
mozreview-review

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 13 • 7 years ago
mozreview-review

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com)
	Comment 14 • 7 years ago
mozreview-review

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.

	Matt Woodrow (:mattwoodrow)
	Comment 15 • 7 years ago
mozreview-review

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

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

	Miko Mynttinen Assignee
	Comment 20 • 7 years ago
mozreview-review-reply

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.

	Matt Woodrow (:mattwoodrow)
	Comment 21 • 7 years ago
mozreview-review

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

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

	Matt Woodrow (:mattwoodrow)
	Comment 22 • 7 years ago
mozreview-review

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

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

	Pulsebot
	Comment 23 • 7 years ago

Pushed by mikokm@gmail.com:
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

	Natalia Csoregi [:nataliaCs]
	Comment 24 • 7 years ago
bugherder

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

	Ionuț Goldan [:igoldan]
	Comment 25 • 7 years ago

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