Closed Bug 1434243 Opened 3 years ago Closed 2 years ago

Investigate unifying nsDisplayLayerEventRegions and nsDisplayCompositorHitTestInfo

Categories

(Core :: Web Painting, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: miko, Assigned: miko)

References

(Depends on 1 open bug, Blocks 2 open bugs, Regressed 2 open bugs)

Details

Attachments

(4 files)

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.
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 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
(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!
(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 :)
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 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
Attachment #8947883 - Flags: review?(bugmail) → 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.
Attachment #8947884 - Flags: review?(bugmail)
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.
Attachment #8948978 - Flags: review?(bugmail) → review+
Blocks: 1436409
Depends on: 1436415
No longer blocks: 1436409
Depends on: 1436409
Comment on attachment 8948977 [details]
Bug 1434243 - Part 3: Make nsDisplayListBuilder::ToReferenceFrame and nsDisplayListBuilder::FindReferenceFrame const

https://reviewboard.mozilla.org/r/218120/#review224396
Attachment #8948977 - Flags: review?(matt.woodrow) → review+
Attachment #8947884 - Flags: review?(bugmail)
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 on attachment 8947884 [details]
Bug 1434243 - Part 2: Add nsDisplayCompositorHitTestInfo support to FrameLayerBuilder

https://reviewboard.mozilla.org/r/217554/#review224674
Attachment #8947884 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8948978 [details]
Bug 1434243 - Part 4: Do not create unnecessary nsDisplayCompositorHitTestInfo items

https://reviewboard.mozilla.org/r/218122/#review224678
Attachment #8948978 - Flags: review?(matt.woodrow) → review+
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
Blocks: 1434846
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
Depends on: 1438527
Depends on: 1443518
Duplicate of this bug: 1426277
Depends on: 1534549
No longer depends on: 1534549
Regressions: 1534549
Regressions: 1648282
You need to log in before you can comment on or make changes to this bug.