Whenever we paint a PaintedLayer, we run RecomputeVisibilityForItems across the full set, even though it frequently doesn't change much between paints. We should be able to figure out which items intersect ones that might have changed, and leave the others as-is.

Comment on attachment 8974606 [details] Bug 1460491 - Part 1: Split nsDisplayItem::mVisibleRect into two members, one for each stated purpose. Gets rid of the save/restore since we no longer modify the building rect. https://reviewboard.mozilla.org/r/242976/#review248842 ::: gfx/layers/wr/WebRenderCommandBuilder.cpp:1609 (Diff revision 1) > paintBounds = aItem->GetClippedBounds(aDisplayListBuilder); > } > > // nsDisplayItem::Paint() may refer the variables that come from ComputeVisibility(). > // So we should call ComputeVisibility() before painting. e.g.: nsDisplayBoxShadowInner > // uses mVisibleRegion in Paint() and mVisibleRegion is computed in Comment should say mPaintRect ::: layout/painting/FrameLayerBuilder.cpp:4473 (Diff revision 1) > } > ((nsRect&)mAccumulatedChildBounds).UnionRect(mAccumulatedChildBounds, bounds); > #endif > > nsIntRect itemVisibleRect = itemDrawRect; > // We haven't computed visibility at this point, so item->GetVisibleRect() Comment should say mBuildingRect ::: layout/painting/nsDisplayList.h:2166 (Diff revision 1) > , mClip(aOther.mClip) > , mActiveScrolledRoot(aOther.mActiveScrolledRoot) > , mReferenceFrame(aOther.mReferenceFrame) > , mAnimatedGeometryRoot(aOther.mAnimatedGeometryRoot) > , mToReferenceFrame(aOther.mToReferenceFrame) > - , mVisibleRect(aOther.mVisibleRect) > + , mBuildingRect(aOther.mBuildingRect) I'm not sure in what cases display items are copied, but do we not need to initialize mPaintRect (to either mBuildingRect or aOther.mPaintRect)? ::: layout/painting/nsDisplayList.h:2646 (Diff revision 1) > * a different coordinate system to this item. > */ > virtual RetainedDisplayList* GetChildren() const { return nullptr; } > > /** > * Returns the visible rect. Update this comment ::: layout/painting/nsDisplayList.h:2660 (Diff revision 1) > + void SetPaintRect(const nsRect& aPaintRect) { > + mPaintRect = aPaintRect; > } > > /** > * Returns the visible rect for the children, relative to their and this one ::: layout/painting/nsDisplayList.h:2863 (Diff revision 1) > // Result of ToReferenceFrame(mFrame), if mFrame is non-null > nsPoint mToReferenceFrame; > RefPtr<mozilla::DisplayItemData> mDisplayItemData; > + > +private: > // This is the rectangle that needs to be painted. Let's comment each of the members seperately explaining what they do ::: layout/painting/nsDisplayList.h:3163 (Diff revision 1) > * of aASR, then the clipped bounds with respect to aASR will be the clip of > * that item for aASR, because the item can move anywhere inside that clip. > * If there is an item in this list which is not bounded with respect to > * aASR (i.e. which does not have "finite bounds" with respect to aASR), > * then this method trigger an assertion failure. > * The optional aVisibleRect out argument can be set to non-null if the comment needs updated

Comment on attachment 8974606 [details] Bug 1460491 - Part 1: Split nsDisplayItem::mVisibleRect into two members, one for each stated purpose. Gets rid of the save/restore since we no longer modify the building rect. https://reviewboard.mozilla.org/r/242976/#review248976 Looks good, just some comments needing updating

Comment on attachment 8974607 [details] Bug 1460491 - Part 2: Only recompute visibility for items if they are newly added to this layer, or intersect one that changed. https://reviewboard.mozilla.org/r/242978/#review249568 ::: layout/painting/FrameLayerBuilder.cpp:1700 (Diff revision 1) > // RecomputeVisibilityForItems if it is known in advance that a larger > // region will be painted during a transaction than in a single call to > // DrawPaintedLayer, for example when progressive paint is enabled. > nsIntRegion mVisibilityComputedRegion; > > + nsRect mRecomputeVisibilityRect; mRecomputeVisibilityRect and mVisibilityComputedRegion now have very similar names but are for different purposes. A comment on mRecomputeVisibilityRect would make things more clear. Perhaps even renaming them - maybe mValidVisibilityComputedRegion and mPreviousRecomputeVisibilityRect? ::: layout/painting/nsDisplayList.h:2657 (Diff revision 1) > void SetBuildingRect(const nsRect& aBuildingRect) > { > mPaintRect = mBuildingRect = aBuildingRect; > } > > void SetPaintRect(const nsRect& aPaintRect) { Can we change mPaintRect to a Maybe instead of having an additional variable to say whether it's valid?

(In reply to Jamie Nicol [:jnicol] from comment #5) > > Can we change mPaintRect to a Maybe instead of having an additional variable > to say whether it's valid? The goal was to do this (and to not set mPaintRect from SetBuildingRect), so that we'd assert if we ever tried to read the paint rect without having called RecomputeVisibility. Unfortunately we fail that assertion due to bug 1459070, so we have to default initialize mPaintRect, and not have assertions until that's fixed. I think these patches make the existing code much more understandable (even if we can't enforce the assumptions), and we still get to have the perf win for now.

Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/920d49a96e6e Part 1: Split nsDisplayItem::mVisibleRect into two members, one for each stated purpose. Gets rid of the save/restore since we no longer modify the building rect. r=jnicol https://hg.mozilla.org/integration/autoland/rev/0571c2da7c43 Part 2: Only recompute visibility for items if they are newly added to this layer, or intersect one that changed. r=jnicol

https://hg.mozilla.org/mozilla-central/rev/920d49a96e6e https://hg.mozilla.org/mozilla-central/rev/0571c2da7c43

Yes definitely this is still an improvement. Let's file a follow up bug so we do make that change at some point though.

Perf improvements! \o/ == Change summary for alert #13244 (as of Wed, 16 May 2018 01:17:47 GMT) == Improvements: 7% displaylist_mutate windows7-32 pgo e10s stylo 4,311.47 -> 4,012.65 7% displaylist_mutate linux64 pgo e10s stylo 3,173.43 -> 2,961.85 6% displaylist_mutate linux64 opt e10s stylo 3,506.39 -> 3,302.16 6% displaylist_mutate windows7-32 opt e10s stylo 5,536.82 -> 5,216.50 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=13244