Closed Bug 1440177 Opened 6 years ago Closed 6 years ago

Improve FrameLayerBuilder performance with inactive layers

Categories

(Core :: Web Painting, enhancement, P2)

Product:
Core
Component:
Web Painting
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

Bug 1440177 - Part 1: Don't call GetLayerState from BuildContainerLayerFor as it recurses into child display items to find the answer.
59 bytes, text/x-review-board-request
jnicol
: review+
Details
Bug 1440177 - Part 2: Combine PaintedLayerItemsEntry and PaintedDisplayItemLayerUserData into a single struct.
59 bytes, text/x-review-board-request
jnicol
: review+
Details
Bug 1440177 - Part 3: Preallocate a small number of PaintedLayerData objects and only resize the mPaintedLayerDataStack once.
59 bytes, text/x-review-board-request
jnicol
: review+
Details
Bug 1440177 - Part 4: Avoid expensive hashtable lookups in PaintedLayerDataTree when we're in an inactive layer and want all items in the same layer.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1440177 - Part 5: Don't call GetDisplayItemDataForManager in AddPaintedDisplayItem since we already have it passed in as a parameter.
59 bytes, text/x-review-board-request
jnicol
: review+
Details
Bug 1440177 - Part 6: Don't dereference display items during AddPaintedDisplayItem for the LAYER_NONE case.
59 bytes, text/x-review-board-request
jnicol
: review+
Details
Bug 1440177 - Part 7: Don't allocate new clips when flattening nsDisplayOpacity.
59 bytes, text/x-review-board-request
mstange
: review+
Details 
Bug 1439807 adds a new talos test that covers inactive layer performance, and it shows a lot of places where we can be more efficient.

There's still plenty more that can be done, but this bug will have fixes for some of the easier low hanging fruit.
Comment on attachment 8952920 [details]
Bug 1440177 - Part 7: Don't allocate new clips when flattening nsDisplayOpacity.

https://reviewboard.mozilla.org/r/222192/#review228472
Attachment #8952920 - Flags: review?(mstange) → review+
Comment on attachment 8952914 [details]
Bug 1440177 - Part 1: Don't call GetLayerState from BuildContainerLayerFor as it recurses into child display items to find the answer.

https://reviewboard.mozilla.org/r/222180/#review228610
Attachment #8952914 - Flags: review?(jnicol) → review+
Comment on attachment 8952916 [details]
Bug 1440177 - Part 3: Preallocate a small number of PaintedLayerData objects and only resize the mPaintedLayerDataStack once.

https://reviewboard.mozilla.org/r/222184/#review228612
Attachment #8952916 - Flags: review?(jnicol) → review+
Comment on attachment 8952918 [details]
Bug 1440177 - Part 5: Don't call GetDisplayItemDataForManager in AddPaintedDisplayItem since we already have it passed in as a parameter.

https://reviewboard.mozilla.org/r/222188/#review228614
Attachment #8952918 - Flags: review?(jnicol) → review+
Comment on attachment 8952919 [details]
Bug 1440177 - Part 6: Don't dereference display items during AddPaintedDisplayItem for the LAYER_NONE case.

https://reviewboard.mozilla.org/r/222190/#review229094

::: layout/painting/FrameLayerBuilder.cpp:4827
(Diff revision 1)
>    bool hasClip = false;
> -  if (aLayerState != LAYER_NONE) {
> -    if (aData) {
> -      tempManager = aData->mInactiveManager;
> +  if (aItem.mLayerState != LAYER_NONE) {
> +    if (aItem.mDisplayItemData) {
> +      tempManager = aItem.mDisplayItemData->mInactiveManager;
>  
>        // We need to grab these before calling AddLayerDisplayItem because it will overwrite them.

Update the comment now this function doesn't exist
Attachment #8952919 - Flags: review?(jnicol) → review+
Comment on attachment 8952915 [details]
Bug 1440177 - Part 2: Combine PaintedLayerItemsEntry and PaintedDisplayItemLayerUserData into a single struct.

https://reviewboard.mozilla.org/r/222182/#review229568

::: layout/painting/FrameLayerBuilder.h:703
(Diff revision 1)
> -
> -    enum { ALLOW_MEMMOVE = true };
> -  };
> -
>    /**
>     * Get the PaintedLayerItemsEntry object associated with aLayer in this

Comment needs updated

::: layout/painting/FrameLayerBuilder.h:751
(Diff revision 1)
>    /**
>     * The display list builder being used.
>     */
>    nsDisplayListBuilder*               mDisplayListBuilder;
>    /**
>     * A map from PaintedLayers to the list of display items (plus

Comment needs updated

::: layout/painting/FrameLayerBuilder.cpp:1368
(Diff revision 1)
>     * relative to the container reference frame
>     * aRoundedRectClipCount is used when building mask layers for PaintedLayers,
>     * SetupMaskLayer will build a mask layer for only the first
>     * aRoundedRectClipCount rounded rects in aClip
>     */
> -  void SetupMaskLayer(Layer *aLayer, const DisplayItemClip& aClip,
> +  uint32_t SetupMaskLayer(Layer *aLayer, const DisplayItemClip& aClip,

Add a comment documenting the return value

::: layout/painting/FrameLayerBuilder.cpp:1484
(Diff revision 1)
>      mXScale(1.f), mYScale(1.f),
>      mAppUnitsPerDevPixel(0),
>      mTranslation(0, 0),
>      mAnimatedGeometryRootPosition(0, 0),
> -    mLastItemCount(0) {}
> +    mLastItemCount(0),
> +    mLastCommonClipCount(0),

Ensure mContainerLayerFrame is initialized

::: layout/painting/FrameLayerBuilder.cpp:1554
(Diff revision 1)
>    size_t mLastItemCount;
>  
> +  // The translation set on this PaintedLayer before we started updating the
> +  // layer tree.
> +  nsIntPoint mLastPaintOffset;
> +  uint32_t mLastCommonClipCount;

Might be tidier to move this next to mMaskClipCount?

::: layout/painting/FrameLayerBuilder.cpp:1556
(Diff revision 1)
> +  // The translation set on this PaintedLayer before we started updating the
> +  // layer tree.
> +  nsIntPoint mLastPaintOffset;
> +  uint32_t mLastCommonClipCount;
> +
> +  nsTArray<AssignedDisplayItem> mItems;

Can we have a comment with what these are and the lifetime they are valid

::: layout/painting/FrameLayerBuilder.cpp:4986
(Diff revision 1)
>  }
>  
>  nsIntPoint
>  FrameLayerBuilder::GetLastPaintOffset(PaintedLayer* aLayer)
>  {
> -  PaintedLayerItemsEntry* entry = mPaintedLayerItems.PutEntry(aLayer);
> +  PaintedDisplayItemLayerUserData* layerData =

Should we use GetPaintedDisplayItemLayerUserData here?
Attachment #8952915 - Flags: review?(jnicol) → review+
Blocks: 1440522
Comment on attachment 8952917 [details]
Bug 1440177 - Part 4: Avoid expensive hashtable lookups in PaintedLayerDataTree when we're in an inactive layer and want all items in the same layer.

https://reviewboard.mozilla.org/r/222186/#review230042

Would be nice to have some assertions about equal AGRs when mForInactiveLayer is true.

::: layout/painting/FrameLayerBuilder.cpp:2753
(Diff revision 1)
> +  if (aTree.IsForInactiveLayer()) {
> +    mHasClip = false;
> +  } else {
> -  mHasClip = mTree.IsClippedWithRespectToParentAnimatedGeometryRoot(mAnimatedGeometryRoot, &mClipRect);
> +    mHasClip = mTree.IsClippedWithRespectToParentAnimatedGeometryRoot(mAnimatedGeometryRoot, &mClipRect);

Would it work to move the check into IsClippedWithRespectToParentAnimatedGeometryRoot?
Attachment #8952917 - Flags: review?(mstange) → review+
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ce353b632a31
Part 1: Don't call GetLayerState from BuildContainerLayerFor as it recurses into child display items to find the answer. r=jnicol
https://hg.mozilla.org/integration/autoland/rev/e6808a7dda3e
Part 2: Combine PaintedLayerItemsEntry and PaintedDisplayItemLayerUserData into a single struct. r=jnicol
https://hg.mozilla.org/integration/autoland/rev/f92987184172
Part 3: Preallocate a small number of PaintedLayerData objects and only resize the mPaintedLayerDataStack once. r=jnicol
https://hg.mozilla.org/integration/autoland/rev/b7e46c4c7dc1
Part 4: Avoid expensive hashtable lookups in PaintedLayerDataTree when we're in an inactive layer and want all items in the same layer. r=mstange
https://hg.mozilla.org/integration/autoland/rev/d22462ac7ee1
Part 5: Don't call GetDisplayItemDataForManager in AddPaintedDisplayItem since we already have it passed in as a parameter. r=jnicol
https://hg.mozilla.org/integration/autoland/rev/d63c5a6c13ae
Part 6: Don't dereference display items during AddPaintedDisplayItem for the LAYER_NONE case. r=jnicol
https://hg.mozilla.org/integration/autoland/rev/f359ad5c4fc3
Part 7: Don't allocate new clips when flattening nsDisplayOpacity. r=mstange
Backout by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aeb71b8d8ca3
Backed out 7 changesets for crashtest (on 1359658-1.html) and reftest failures
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b1ba6c67264b
Part 1: Don't call GetLayerState from BuildContainerLayerFor as it recurses into child display items to find the answer. r=jnicol
https://hg.mozilla.org/integration/autoland/rev/0840994846bf
Part 2: Combine PaintedLayerItemsEntry and PaintedDisplayItemLayerUserData into a single struct. r=jnicol
https://hg.mozilla.org/integration/autoland/rev/717a696b17e0
Part 3: Preallocate a small number of PaintedLayerData objects and only resize the mPaintedLayerDataStack once. r=jnicol
https://hg.mozilla.org/integration/autoland/rev/49b285030492
Part 4: Avoid expensive hashtable lookups in PaintedLayerDataTree when we're in an inactive layer and want all items in the same layer. r=mstange
https://hg.mozilla.org/integration/autoland/rev/928770efc9a3
Part 5: Don't call GetDisplayItemDataForManager in AddPaintedDisplayItem since we already have it passed in as a parameter. r=jnicol
https://hg.mozilla.org/integration/autoland/rev/a77d06b2cf03
Part 6: Don't dereference display items during AddPaintedDisplayItem for the LAYER_NONE case. r=jnicol
https://hg.mozilla.org/integration/autoland/rev/d83a1820b2f2
Part 7: Don't allocate new clips when flattening nsDisplayOpacity. r=mstange
Backout by aiakab@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13adabb75562
Backed out 7 changesets for build bustages on a CLOSED TREE
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d4b4e8e9d04
Part 1: Don't call GetLayerState from BuildContainerLayerFor as it recurses into child display items to find the answer. r=jnicol
https://hg.mozilla.org/integration/autoland/rev/ef972081de69
Part 2: Combine PaintedLayerItemsEntry and PaintedDisplayItemLayerUserData into a single struct. r=jnicol
https://hg.mozilla.org/integration/autoland/rev/7b7026149e56
Part 3: Preallocate a small number of PaintedLayerData objects and only resize the mPaintedLayerDataStack once. r=jnicol
https://hg.mozilla.org/integration/autoland/rev/992b8e378397
Part 4: Avoid expensive hashtable lookups in PaintedLayerDataTree when we're in an inactive layer and want all items in the same layer. r=mstange
https://hg.mozilla.org/integration/autoland/rev/4c62cca5f3ec
Part 5: Don't call GetDisplayItemDataForManager in AddPaintedDisplayItem since we already have it passed in as a parameter. r=jnicol
https://hg.mozilla.org/integration/autoland/rev/58025bcb77cb
Part 6: Don't dereference display items during AddPaintedDisplayItem for the LAYER_NONE case. r=jnicol
https://hg.mozilla.org/integration/autoland/rev/114d99e1c41c
Part 7: Don't allocate new clips when flattening nsDisplayOpacity. r=mstange
Big perf improvements on all desktop platforms!

== Change summary for alert #11889 (as of Thu, 01 Mar 2018 20:53:34 GMT) ==

Improvements:

 12%  displaylist_mutate windows10-64 opt e10s stylo     7,724.86 -> 6,824.32
 11%  displaylist_mutate windows7-32 opt e10s stylo      10,261.45 -> 9,114.52
  9%  displaylist_mutate windows7-32 pgo e10s stylo      7,120.81 -> 6,487.78
  8%  displaylist_mutate linux64 opt e10s stylo          3,421.80 -> 3,140.61
  8%  displaylist_mutate linux64 pgo e10s stylo          2,994.01 -> 2,764.58
  7%  displaylist_mutate windows10-64 pgo e10s stylo     6,547.81 -> 6,086.81
  6%  displaylist_mutate osx-10-10 opt e10s stylo        7,732.30 -> 7,255.42

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11889
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: