We should land the code to prevent bit rot (and make sure it's safe to ride the trains), and then work to enable it.

Comment on attachment 8913518 [details] Bug 1404181 - Part 9: Add code for detecting if display list building happened for a given frame, and use it to add some tests for retained display lists. https://reviewboard.mozilla.org/r/184872/#review193602 ::: layout/generic/nsIFrame.h:3837 (Diff revision 1) > > // Checks if we (or any of our descendents) have NS_FRAME_PAINTED_THEBES set, and > // clears this bit if so. > bool CheckAndClearPaintedState(); > > + // Checks if we (or any of our descendents) have NS_FRAME_BUILT_DISPLAY_LIST set, and You're not using a frame state bit any more.

Comment on attachment 8913510 [details] Bug 1404181 - Part 1: Make AnimatedGeometryRoot/ActiveScrolledRoot retained and refcounted on the heap. https://reviewboard.mozilla.org/r/184856/#review191188 ::: layout/painting/nsDisplayList.h:205 (Diff revision 1) > + : mFrame(aFrame) > + , mParentAGR(aParent) > + , mIsAsync(aIsAsync) > + , mIsRetained(aIsRetained) > + { > + MOZ_ASSERT(mParentAGR || mIsAsync); Maybe add an assertion message here, like "The root AGR should always be treated as an async AGR." ::: layout/painting/nsDisplayList.h:249 (Diff revision 1) > + > + RefPtr<ActiveScrolledRoot> asr; > + if (aIsRetained) { > + asr = f->GetProperty(ActiveScrolledRootCache()); > + } > + end-of-line whitespace ::: layout/painting/nsDisplayList.h:287 (Diff revision 1) > static nsCString ToString(const mozilla::ActiveScrolledRoot* aActiveScrolledRoot); > > // Call this when inserting an ancestor. > void IncrementDepth() { mDepth++; } > > const ActiveScrolledRoot* mParent; Should this be a RefPtr?

Comment on attachment 8913511 [details] Bug 1404181 - Part 2: Track a 'visible' rect for display list building as well as the 'dirty' rect, so that we can do partial building by specifying a smaller 'dirty' rect. https://reviewboard.mozilla.org/r/184858/#review193698 ::: layout/generic/nsFrame.cpp:2712 (Diff revision 1) > > if (clipCapturedBy == ContainerItemType::eFilter) { > clipState.Restore(); > } > // Revert to the post-filter dirty rect. > - aBuilder->SetDirtyRect(dirtyRectOutsideSVGEffects); > + aBuilder->SetVisibleRect(visibleRectOutsideSVGEffects); Don't you need to restore both the visible and the dirty rect here? ::: layout/generic/nsFrame.cpp:2802 (Diff revision 1) > // Restore clip state now so nsDisplayTransform is clipped properly. > clipState.Restore(); > } > // Revert to the dirtyrect coming in from the parent, without our transform > // taken into account. > - aBuilder->SetDirtyRect(dirtyRectOutsideTransform); > + aBuilder->SetVisibleRect(visibleRectOutsideTransform); same here - I'd expect both the dirty and the visible rect to be set here. ::: layout/generic/nsFrame.cpp:2924 (Diff revision 1) > /** > * Check if a frame should be visited for building display list. > */ > static bool > DescendIntoChild(nsDisplayListBuilder* aBuilder, nsIFrame *aChild, > - const nsRect& aDirty) > + const nsRect& aDirty, const nsRect& aVisible) Could you make the order of the arguments here consistent with the AutoBuildingDisplayList constructor and with DecideScrollableLayer? First visible, then dirty. ::: layout/generic/nsIFrame.h:4237 (Diff revision 1) > + > + bool mForceDescendIntoIfVisible : 1; End-of-line whitespace, and this needs a comment, and the comment below mIsPrimaryFrame about the size of the hole needs to be adjusted. ::: layout/generic/nsPageFrame.cpp:564 (Diff revision 1) > // these extra pages are pruned so that only display items for the > // page we currently care about (which we would have reached by > // following placeholders to their out-of-flows) end up on the list. > nsIFrame* page = child; > while ((page = GetNextPage(page)) != nullptr) { > nsRect childDirty = dirtyRect + child->GetOffsetTo(page); Rename dirtyRect to visibleRect and childDirty to childVisible? I think that would be clearer. ::: layout/painting/nsDisplayList.cpp:1000 (Diff revision 1) > > nsCSSRendering::EndFrameTreesLocked(); > } > > -static void MarkFrameForDisplay(nsIFrame* aFrame, nsIFrame* aStopAtFrame) { > +void > +nsDisplayListBuilder::MarkFrameForDisplay(nsIFrame* aFrame, nsIFrame* aStopAtFrame) { move brace into next line since you're touching this line anyway ::: layout/painting/nsDisplayList.cpp:1015 (Diff revision 1) > } > } > } > > +void > +nsDisplayListBuilder::MarkFrameForDisplayIfVisible(nsIFrame* aFrame, nsIFrame* aStopAtFrame) { move brace ::: layout/painting/nsDisplayList.cpp:1128 (Diff revision 1) > + // TODO: We probably don't want visible and dirty to be the same here, figure > + // out what to do. > + visible = dirtyRectRelativeToDirtyFrame; What's the problem here? ::: layout/xul/nsRootBoxFrame.cpp:179 (Diff revision 1) > - nsRect displayPortBase = > + nsRect displayPortBase = aBuilder->GetVisibleRect().Intersect(nsRect(nsPoint(0, 0), GetSize())); > - aBuilder->GetDirtyRect().Intersect(nsRect(nsPoint(0, 0), GetSize())); Let's keep the line break :) ::: layout/xul/nsSliderFrame.cpp:425 (Diff revision 1) > refSize.height /= scale.height; > } > - nsRect dirty = aBuilder->GetDirtyRect().Intersect(thumbRect); > - dirty = nsLayoutUtils::ComputePartialPrerenderArea(aBuilder->GetDirtyRect(), overflow, refSize); > + nsRect dirty = aBuilder->GetVisibleRect().Intersect(thumbRect); > + dirty = nsLayoutUtils::ComputePartialPrerenderArea(aBuilder->GetVisibleRect(), overflow, refSize); > > + // TODO: Should we be setting visible and dirty to the same thing here? Well, what is necessary for prerendering? I'd assume we just need to enlarge the visible rect, and take the dirty rect as is... unless the dirty rect has been affected by clipping from ancestor frames, in which case some of the dirtiness information may have been lost...

Comment on attachment 8913512 [details] Bug 1404181 - Part 3: Track the current set of alive display items on nsIFrame. https://reviewboard.mozilla.org/r/184860/#review193704 ::: layout/generic/nsFrame.cpp:919 (Diff revision 1) > + > +bool > +nsIFrame::HasDisplayItems() > +{ > + DisplayItemArray* items = GetProperty(DisplayItems()); > + return !!items; return items != nullptr; is the more common way to do this, I think ::: layout/generic/nsIFrame.h:1138 (Diff revision 1) > nsPoint GetPositionIgnoringScrolling(); > > typedef AutoTArray<nsIContent*, 2> ContentArray; > static void DestroyContentArray(ContentArray* aArray); > > + typedef AutoTArray<nsDisplayItem*, 4> DisplayItemArray; Have you investigated whether using a SmallPointerArray for this one gives any benefits?

Comment on attachment 8913513 [details] Bug 1404181 - Part 4: Add code to save and restore changes made to display items during FrameLayerBuilder so that we can use them again. https://reviewboard.mozilla.org/r/184862/#review193718 ::: layout/painting/nsDisplayList.cpp:2891 (Diff revision 1) > + SetClipChain(aBuilder->CreateClipChainIntersection(ancestorClip, mClipChain, aOther), > + true); We have some callers of IntersectClip in ApplyOpacity methods. For those we should probably be passing aStore = false here. So I think IntersectClip needs to grow an aStore argument.

Comment on attachment 8913514 [details] Bug 1404181 - Part 5: Store an annotated list of rectangles in nsDisplayLayerEventRegions so that we can remove contributions that belong to invalidated frames. https://reviewboard.mozilla.org/r/184864/#review193722 ::: layout/painting/nsDisplayList.h:4139 (Diff revision 1) > + // TODO: It would be nice if we could ask the initial construction above > + // to include simplification. Yeah, constructing a region from an array of lots of rectangles is probably going to be slower than what we had before, and this is something that impacts even builds with retained display lists off. ::: layout/painting/nsDisplayList.cpp:4709 (Diff revision 1) > - mDispatchToContentHitRegion.OrWith(CombinedTouchActionRegion()); > - mDispatchToContentHitRegion.SimplifyOutward(8); > + // TODO: Is it possible that merging event region items > + // would change whether this needs to happen, and we can't > + // undo it? Should we defer this work until we need the result > + // instead? I don't know, I haven't thought through the alreadyHadRegions case.

Comment on attachment 8913515 [details] Bug 1404181 - Part 6: Compute and store the frame that contributed the style data for backgrounds so that we can invalidate the display item when either frame changes. https://reviewboard.mozilla.org/r/184866/#review194598

Comment on attachment 8913511 [details] Bug 1404181 - Part 2: Track a 'visible' rect for display list building as well as the 'dirty' rect, so that we can do partial building by specifying a smaller 'dirty' rect. https://reviewboard.mozilla.org/r/184858/#review193698 > Don't you need to restore both the visible and the dirty rect here? I don't think I do, since we're past the point that the dirty rect is used to determine if we want to descend into frames. The remainder of the function just uses the visible rect to initialize mVisibleRect on the display items, and then we'll pop the AutoBuildingDisplayList off the stack and overwrite it. > What's the problem here? The existing code overrides the dirty/visible rect when we're building an async fixed position frame. We definitely don't want to make the dirty/visible the same (which is what it does now), since that results in us rebuilding all fixed position content. I haven't yet understood the code well enough to figure out what transformation to apply to the dirty rect though. I'll do this as a follow-up to avoid breaking things. > Well, what is necessary for prerendering? I'd assume we just need to enlarge the visible rect, and take the dirty rect as is... unless the dirty rect has been affected by clipping from ancestor frames, in which case some of the dirtiness information may have been lost... Yeah indeed, the clipping makes this hard. I don't think it matters just building using the visible rect, so I'll just drop the comment.

Comment on attachment 8913512 [details] Bug 1404181 - Part 3: Track the current set of alive display items on nsIFrame. https://reviewboard.mozilla.org/r/184860/#review193704 > Have you investigated whether using a SmallPointerArray for this one gives any benefits? SmallPointerArray is a space-optimized AutoTArray<T,2>. We frequently have more than 2 items (especially after partial building, but before merging when there can be two instances of a logical item), so I think the space savings would be cancelled out by more frequent allocations. I haven't actually measured this though.

Comment on attachment 8913516 [details] Bug 1404181 - Part 7: Add a way to check if existing painted items might need to be redrawn for a sync decode. https://reviewboard.mozilla.org/r/184868/#review195034

Comment on attachment 8913517 [details] Bug 1404181 - Part 8: Add refcounting for clip chains. https://reviewboard.mozilla.org/r/184870/#review195036 ::: layout/painting/nsDisplayList.cpp:1351 (Diff revision 2) > + // Reverse iterate the clip chains, so that we destroy descendants > + // first which will drop the ref count on their ancestors. > + auto it = mClipChainsToDestroy.begin(); Please also say in the comment that the clip chains are stored in backwards order in the list, because we add the newest one at the front of the list. Otherwise it's a bit hard to understand how the code could be iterating over the list backwards.

Comment on attachment 8913518 [details] Bug 1404181 - Part 9: Add code for detecting if display list building happened for a given frame, and use it to add some tests for retained display lists. https://reviewboard.mozilla.org/r/184872/#review195042

Comment on attachment 8913519 [details] Bug 1404181 - Part 10: Track theme geometry contributions per-frame so that can remove contributions from invalidated frames. https://reviewboard.mozilla.org/r/184874/#review195048 ::: layout/painting/nsDisplayList.h:3694 (Diff revision 2) > const nsRect& aBackgroundRect); > virtual ~nsDisplayThemedBackground(); > > + void Destroy(nsDisplayListBuilder* aBuilder) override > + { > + aBuilder->UnregisterThemeGeometry(mFrame); Maybe add a comment that it's fine to remove all theme geometries for the frame, because there will never be more than one nsDisplayThemedBackground per frame. Actually, if that's the case, do we need to have arrays in the hashtable at all?

Comment on attachment 8913520 [details] Bug 1404181 - Part 11: Factor out PresShell paint count code se we can still call it without needing to build the display items. https://reviewboard.mozilla.org/r/184876/#review195068

Comment on attachment 8913522 [details] Bug 1404181 - Part 12: Add pref for retained display lists (starting with it disabled). https://reviewboard.mozilla.org/r/184880/#review195078

Comment on attachment 8913523 [details] Bug 1404181 - Part 13: Expose DisplayItemData better from FrameLayerBuilder so we can query the geometry (for sync decode invalidations). https://reviewboard.mozilla.org/r/184882/#review195082

Comment on attachment 8913524 [details] Bug 1404181 - Part 15: Compute scrollbar thumb AGR state in advance, rather than setting it during building. https://reviewboard.mozilla.org/r/184884/#review195086 ::: layout/generic/nsGfxScrollFrame.cpp:5073 (Diff revision 2) > + nsIContent* content = mOuter->GetContent(); > return mHasBeenScrolledRecently || > IsAlwaysActive() || > - mWillBuildScrollableLayer; > + nsLayoutUtils::HasDisplayPort(content) || > + nsContentUtils::HasScrollgrab(content); > } > > bool > ScrollFrameHelper::IsScrollingActive(nsDisplayListBuilder* aBuilder) const > { > const nsStyleDisplay* disp = mOuter->StyleDisplay(); > if (disp && (disp->mWillChangeBitField & NS_STYLE_WILL_CHANGE_SCROLL) && > aBuilder->IsInWillChangeBudget(mOuter, GetScrollPositionClampingScrollPortSize())) { > return true; > } > > + nsIContent* content = mOuter->GetContent(); > return mHasBeenScrolledRecently || > IsAlwaysActive() || > - mWillBuildScrollableLayer; > + nsLayoutUtils::HasDisplayPort(content) || > + nsContentUtils::HasScrollgrab(content); I think these changes are on mozilla-central now.

Comment on attachment 8913525 [details] Bug 1404181 - Part 14: Add a bunch of new helpers to display list builder and display item for retaining and reusing items. https://reviewboard.mozilla.org/r/184886/#review195088 ::: layout/painting/nsDisplayList.h:2513 (Diff revision 2) > - return Frame()->In3DContextAndBackfaceIsHidden(); > + if (mBackfaceHidden) { > + return *mBackfaceHidden; > + } > + > + bool backfaceHidden = Frame()->In3DContextAndBackfaceIsHidden(); > + mBackfaceHidden.emplace(backfaceHidden); Maybe add a comment somewhere that this is a cache that never needs to be invalidated because the display item will be recreated if the backface visibility changes.

Comment on attachment 8913526 [details] Bug 1404181 - Part 15: Add the notion of 'modified' frames, that need new display items built for them (and all their descendants). https://reviewboard.mozilla.org/r/184888/#review195090 ::: layout/painting/nsDisplayList.h:620 (Diff revision 2) > > bool IsBuilding() const { return mIsBuilding; } > void SetIsBuilding(bool aIsBuilding) > { > mIsBuilding = aIsBuilding; > + for (nsIFrame* f : mModifiedFramesDuringBuilding) { Some serious end-of-line whitespace here

Comment on attachment 8913527 [details] Bug 1404181 - Part 16: Track window dragging area contributions per-frame so that can remove contributions from invalidated frames. https://reviewboard.mozilla.org/r/184890/#review195096 ::: layout/painting/nsDisplayList.cpp:1862 (Diff revision 2) > + mWindowDraggingFrames[i] = mWindowDraggingFrames[length - 1]; > + mWindowDraggingRects[i] = mWindowDraggingRects[length - 1]; > + length--; This took me a second to understand, but it's not complicated enough to make me complain about it, so I think you can keep it. ::: layout/painting/nsDisplayList.cpp:1871 (Diff revision 2) > + i++; > + } > + } > + mWindowDraggingFrames.resize(length); > + mWindowDraggingRects.SetLength(length); > + whitespace

Comment on attachment 8913528 [details] Bug 1404181 - Part 17: Track will-change contributions per-frame so that can remove contributions from invalidated frames. https://reviewboard.mozilla.org/r/184892/#review195098 ::: layout/generic/nsFrame.cpp:3112 (Diff revision 2) > } > > nsIFrame* child = aChild; > if (child->GetStateBits() & NS_FRAME_TOO_DEEP_IN_FRAME_TREE) > return; > + whitespace ::: layout/generic/nsIFrame.h:4291 (Diff revision 2) > */ > bool mBuiltDisplayList : 1; > > bool mFrameIsModified : 1; > > + bool mMayHaveWillChangeBudget : 1; Another frame bit? :( You need to update the comment about the size of the gap again. ::: layout/painting/nsDisplayList.cpp:1987 (Diff revision 2) > + > + DocumentWillChangeBudget& budget = end-of-line whitespace in both lines

Comment on attachment 8913529 [details] Bug 1404181 - Part 18: Use nsPresArena for the display list builder arena since it supports recycling. https://reviewboard.mozilla.org/r/184894/#review195104 ::: layout/painting/nsDisplayList.cpp:1449 (Diff revision 2) > } > } > } > } > > +uint32_t gDisplayItemSizes[static_cast<uint32_t>(DisplayItemType::TYPE_MAX)] = { 0 }; Would be nice to initialize this statically, maybe with some nsDisplayItemTypesList.h #include magic and constexpr?

Comment on attachment 8913530 [details] Bug 1404181 - Part 19: Some items don't use style data from mFrame, so add overrides that let us check the right frame when determining if an item is invalid. https://reviewboard.mozilla.org/r/184896/#review195526 ::: layout/painting/nsDisplayList.h:1997 (Diff revision 2) > MOZ_ASSERT(mFrame, "Trying to use display item after deletion!"); > return mFrame; > } > > + /** > + * @return the frame for invalidation. Is this frame the only frame that causes this item to be invalidated? Or is it an *additional* frame that causes the item to be invalidated, next to item->Frame()? If it's the latter, I think ExtraFrameForInvalidation would be a better name. I also think the comment could be a little more verbose. ::: layout/painting/nsDisplayList.cpp:2564 (Diff revision 2) > uint32_t nsDisplayList::Count() const { > - uint32_t count = 0; > + return mLength; > - for (nsDisplayItem* i = GetBottom(); i; i = i->GetAbove()) { > - ++count; > - } > - return count; > } > > nsDisplayItem* nsDisplayList::RemoveBottom() { > nsDisplayItem* item = mSentinel.mAbove; > if (!item) > return nullptr; > mSentinel.mAbove = item->mAbove; > if (item == mTop) { > // must have been the only item > mTop = &mSentinel; > } > item->mAbove = nullptr; > + mLength--; > return item; > } Would be better to move these changes to part 16, where you're adding mLength.

Comment on attachment 8913531 [details] Bug 1404181 - Part 20: Add code to override the display list builder dirty area for a stacking context or displayport. This lets us restrict partial building to within one of these contexts. https://reviewboard.mozilla.org/r/184898/#review195532 ::: commit-message-b76b8:1 (Diff revision 2) > +Bug 1404181 - Part 22: Add code for override the display list builder dirty area for a stacking context or displayport. This lets us restrict partial building to within one of these contexts. r?mstange for -> to ::: layout/generic/nsFrame.cpp:2620 (Diff revision 2) > + dirtyRect = data->mDirtyRect; > + dirtyRect.IntersectRect(dirtyRect, visibleRect); I suggest dirtyRect = data->mDirtyRect.Intersect(visibleRect) ::: layout/generic/nsGfxScrollFrame.cpp:3748 (Diff revision 2) > + if (!aBuilder->IsPartialUpdate() || > + aBuilder->InInvalidSubtree()) { These could go in one line. ::: layout/generic/nsGfxScrollFrame.cpp:3748 (Diff revision 2) > + if (!aBuilder->IsPartialUpdate() || > + aBuilder->InInvalidSubtree()) { > - *aDirtyRect = displayPort; > + *aDirtyRect = displayPort; > + if (aUsingDisplayPortInvalidRect) { > + *aUsingDisplayPortInvalidRect = true; > + } > + } else if (mOuter->HasOverrideDirtyRegion()) { > + nsRect* rect = > + mOuter->GetProperty(nsDisplayListBuilder::DisplayListBuildingDisplayPortRect()); > + if (rect) { > + *aDirtyRect = *rect; > + if (aUsingDisplayPortInvalidRect) { > + *aUsingDisplayPortInvalidRect = true; I don't really understand the commonality between the two cases where you set aUsingDisplayPortInvalidRect to true. In the first case, you use the full display port, and in the second case, you use the override rectangle. I guess in the "*aUsingDisplayPortInvalidRect == false" case, the dirty rect is unchanged? Maybe the argument should be renamed to "dirtyRectHasBeenOverridden" or "dirtyRectHasPotentiallyBeenEnlarged"?

Comment on attachment 8913513 [details] Bug 1404181 - Part 4: Add code to save and restore changes made to display items during FrameLayerBuilder so that we can use them again. https://reviewboard.mozilla.org/r/184862/#review195722 C/C++ static analysis found 1 defect in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: layout/generic/nsTextFrame.cpp:4950 (Diff revision 3) > virtual ~nsDisplayText() { > MOZ_COUNT_DTOR(nsDisplayText); > } > #endif > > + virtual void RestoreState() override Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

Comment on attachment 8913516 [details] Bug 1404181 - Part 7: Add a way to check if existing painted items might need to be redrawn for a sync decode. https://reviewboard.mozilla.org/r/184868/#review195726 C/C++ static analysis found 2 defects in this patch. You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp` If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: layout/generic/nsBulletFrame.cpp:191 (Diff revision 3) > { > nsBulletFrame* f = static_cast<nsBulletFrame*>(aItem->Frame()); > mOrdinal = f->GetOrdinal(); > } > > + virtual bool InvalidateForSyncDecodeImages() const override Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override] ::: layout/generic/nsPluginFrame.cpp:919 (Diff revision 3) > const ContainerLayerParameters& aParameters) override > { > return LAYER_ACTIVE; > } > + > + virtual nsDisplayItemGeometry* AllocateGeometry(nsDisplayListBuilder* aBuilder) override Warning: 'virtual' is redundant since the function is already declared 'override' [clang-tidy: modernize-use-override]

(In reply to Markus Stange [:mstange] from comment #72) > Comment on attachment 8913519 [details] > Bug 1404181 - Part 10: Track theme geometry contributions per-frame so that > can remove contributions from invalidated frames. > > https://reviewboard.mozilla.org/r/184874/#review195048 > > ::: layout/painting/nsDisplayList.h:3694 > (Diff revision 2) > > const nsRect& aBackgroundRect); > > virtual ~nsDisplayThemedBackground(); > > > > + void Destroy(nsDisplayListBuilder* aBuilder) override > > + { > > + aBuilder->UnregisterThemeGeometry(mFrame); > > Maybe add a comment that it's fine to remove all theme geometries for the > frame, because there will never be more than one nsDisplayThemedBackground > per frame. > > Actually, if that's the case, do we need to have arrays in the hashtable at > all? The reason for using the array is because nsTreeBodyFrame might register more than one ThemeGeometry, one for each selected row. http://searchfox.org/mozilla-central/rev/d0448c970093f94bd986a21d3a9e8c366b316eb4/layout/xul/tree/nsTreeBodyFrame.cpp#2881 Currently though, I am not sure if any UI elements[1] using this code actually allow multiple selection. [1] http://searchfox.org/mozilla-central/search?q=mac-source-list&path=

Comment on attachment 8913532 [details] Bug 1404181 - Part 21: Add RetainedDisplayListBuilder with the code for doing partial display list builds, and merging it into an existing display list. https://reviewboard.mozilla.org/r/184900/#review196506 ::: layout/base/nsLayoutUtils.cpp:3722 (Diff revision 2) > builder.ClearHaveScrollableDisplayPort(); > if (builder.IsPaintingToWindow()) { > MaybeCreateDisplayPortInFirstScrollFrameEncountered(aFrame, builder); > } > > nsRect dirtyRect = visibleRegion.GetBounds(); Should this variable be renamed to visibleRect maybe? ::: layout/base/nsLayoutUtils.cpp:3777 (Diff revision 2) > + builder.EnterPresShell(aFrame); > + builder.SetDirtyRect(dirtyRect); > + builder.ClearWindowDraggingRegion(); > + aFrame->BuildDisplayListForStackingContext(&builder, &list); > - AddExtraBackgroundItems(builder, list, aFrame, canvasArea, visibleRegion, aBackstop); > + AddExtraBackgroundItems(builder, list, aFrame, canvasArea, visibleRegion, aBackstop); > - > + end-of-line whitespace ::: layout/base/nsLayoutUtils.cpp:3987 (Diff revision 2) > } > > - builder.EndFrame(); > + { > + AutoProfilerTracing tracing("Paint", "DisplayListResources"); > > + end-of-line whitespace ::: layout/base/nsLayoutUtils.cpp:3992 (Diff revision 2) > + delete listPtr; > + delete builderPtr; This seems brittle to me. Could you maybe have local variables like this: Maybe<nsDisplayListBuilder> nonRetainedBuilder; Maybe<nsDisplayList> nonRetainedList; which you make listPtr and builderPtr point to in the non-retained case? Then you wouldn't have to free these objects manually. ::: layout/painting/RetainedDisplayListBuilder.cpp:752 (Diff revision 2) > + //printf_stderr("Painting --- Merged list:\n"); > + //nsFrame::PrintDisplayList(&builder, list); > + > + merged = true; > + } > + end-of-line whitespace

Comment on attachment 8913532 [details] Bug 1404181 - Part 21: Add RetainedDisplayListBuilder with the code for doing partial display list builds, and merging it into an existing display list. https://reviewboard.mozilla.org/r/184900/#review196514 ::: layout/base/nsLayoutUtils.cpp:3767 (Diff revision 3) > + const bool paintedPreviously = > + aFrame->HasProperty(nsIFrame::ModifiedFrameList()); > + > + bool merged = false; > + if (retainedBuilder && paintedPreviously) { > + merged = retainedBuilder->AttemptPartialUpdate(aBackstop); So this does mBuilder.RootReferenceFrame()->BuildDisplayListForStackingContext(...) internally. Maybe it would be better to pass aFrame in here? When I first read this, it wasn't obvious to me just by looking at the caller side that AttemptPartialUpdate is going to actually do the display list building + merging, as opposed to just doing some preparatory work. Or maybe just a comment would help. ::: layout/base/nsLayoutUtils.cpp:3784 (Diff revision 3) > + // if (XRE_IsContentProcess()) { > + // printf_stderr("Painting --- Full list:\n"); > + // nsFrame::PrintDisplayList(&builder, list); > + // } May want to remove this before landing, or maybe turn it into an #if 0. (Or add a pref, or an #ifdef with a name, or whatever you prefer.) ::: layout/painting/RetainedDisplayListBuilder.h:22 (Diff revision 3) > + nsDisplayListBuilder* Builder() { return &mBuilder; } > + > + nsDisplayList* List() { return &mList; } > + > + bool AttemptPartialUpdate(nscolor aBackstop); This is a very small public API. Good! ::: layout/painting/RetainedDisplayListBuilder.cpp:55 (Diff revision 3) > + } else if (!(i->GetFlags() & TYPE_RENDERS_NO_IMAGES)) { > + invalidate = true; > + } > + > + if (invalidate) { > + i->Frame()->MarkNeedsDisplayItemRebuild(); Maybe also do this for i->FrameForInvalidation()? Or is the assumption that items with images always have Frame() == FrameForInvalidation()? If that's the case, please add a comment. ::: layout/painting/RetainedDisplayListBuilder.cpp:95 (Diff revision 3) > + continue; > + } > + > + if (i->GetChildren()) { > + AnimatedGeometryRoot *childAGR = aAGR; > + if (i->Frame()->IsStackingContext()) { You use i->Frame() quite a bit in this function. May want to store it in a variable. ::: layout/painting/RetainedDisplayListBuilder.cpp:133 (Diff revision 3) > +{ > + return aFirst->Frame() == aSecond->Frame() && > + aFirst->GetPerFrameKey() == aSecond->GetPerFrameKey(); > +} > + > +struct DisplayItemKey { I think the style guide wants a line break before the opening brace. ::: layout/painting/RetainedDisplayListBuilder.cpp:141 (Diff revision 3) > + return mFrame == aOther.mFrame && > + mKey == aOther.mKey; > + } > + > + nsIFrame* mFrame; > + uint32_t mKey; Maybe call this mPerFrameKey? The different meanings of the word "key" in the nsDisplayItemHashEntry implementations were a bit confusing to me. ::: layout/painting/RetainedDisplayListBuilder.cpp:187 (Diff revision 3) > + } > + > + aArray.RemoveElementAt(aArray.Length() - 1); > +} > + > +void MergeFrameRects(nsDisplayLayerEventRegions* aOldItem, static void ::: layout/painting/RetainedDisplayListBuilder.cpp:249 (Diff revision 3) > + } > +} > + > +void MergeLayerEventRegions(nsDisplayItem* aOldItem, > + nsDisplayItem* aNewItem, > + bool aUpdateOld) I'd like to buy an UPDATE_OLD_ITEM | UPDATE_NEW_ITEM enum to replace the bool with. ::: layout/painting/RetainedDisplayListBuilder.cpp:298 (Diff revision 3) > + * > + * The basic algorithm is: > + * > + * For-each item in the new list: > + * If the item has a matching item in the old list: > + * Pop items off the old list until we reach the matching item: Not sure what "Pop items off" means; remove them from the start? Do the removed items get destroyed? ::: layout/painting/RetainedDisplayListBuilder.cpp:301 (Diff revision 3) > + * For-each item in the new list: > + * If the item has a matching item in the old list: > + * Pop items off the old list until we reach the matching item: > + * Add valid items to the merged list, destroy invalid items. > + * Destroy the matching item from the old list. > + * Add the item from the new lis into the merged list. lis -> list ::: layout/painting/RetainedDisplayListBuilder.cpp:344 (Diff revision 3) > +RetainedDisplayListBuilder::MergeDisplayLists(nsDisplayList* aNewList, > + nsDisplayList* aOldList, > + nsDisplayList* aOutList) > +{ > + nsDisplayList merged(&mBuilder); > + nsDisplayItem* old; The declaration of this variable is so far away from its uses. I'd prefer to have two different variables for this in the two places where its used, just before the respective loop. ::: layout/painting/RetainedDisplayListBuilder.cpp:381 (Diff revision 3) > + } > + newListLookup.Put({ i->Frame(), i->GetPerFrameKey() }, i); > + } > +#endif > + > + while (nsDisplayItem* i = aNewList->RemoveBottom()) { Maybe call i newItem instead ::: layout/painting/RetainedDisplayListBuilder.cpp:398 (Diff revision 3) > + // If the new item has a matching counterpart in the old list, copy all valid > + // items from the old list into the merged list until we get to the repeat. We already know that there is a matching counterpart in the old list, because we've null-checked oldItem. Maybe rephrase the comment? ::: layout/painting/RetainedDisplayListBuilder.cpp:408 (Diff revision 3) > + } else { > + oldListLookup.Remove({ old->Frame(), old->GetPerFrameKey() }); > + old->Destroy(&mBuilder); > + } > + } > + MOZ_ASSERT(old && IsSameItem(i, old)); Would MOZ_ASSERT(old == oldItem); be a valid assertion here? ::: layout/painting/RetainedDisplayListBuilder.cpp:467 (Diff revision 3) > + > + aOutList->AppendToTop(&merged); > +} > + > +static void > +AddModifiedFramesFromRootFrame(std::vector<WeakFrame>& aFrames, How about TakeAndAddModifiedFramesFromRootFrame? Would make it clearer that the array in the property will be cleared. ::: layout/painting/RetainedDisplayListBuilder.cpp:476 (Diff revision 3) > + for (WeakFrame& frame : *frames) { > + aFrames.push_back(Move(frame)); > + } > + > + frames->clear(); How about: aFrames.swap(*frames); frames->clear(); instead of the loop ::: layout/painting/RetainedDisplayListBuilder.cpp:522 (Diff revision 3) > + > + return modifiedFrames; > +} > + > +// ComputeRebuildRegion debugging > +// #define CRR_DEBUG 0 If it's commented out anyway, might as well #define it to 1 :) ::: layout/painting/RetainedDisplayListBuilder.cpp:562 (Diff revision 3) > + nsRect* aOutDirty, > + AnimatedGeometryRoot** aOutModifiedAGR, > + nsTArray<nsIFrame*>* aOutFramesWithProps) > +{ > + CRR_LOG("Computing rebuild regions for %d frames:\n", aModifiedFrames.size()); > + for (nsIFrame* f : aModifiedFrames) { ... neat (how you're getting the nsIFrame* out of the nsWeakFrame here) ::: layout/painting/RetainedDisplayListBuilder.cpp:626 (Diff revision 3) > + // Don't contribute to the root dirty area at all. > + overflow.SetEmpty(); This is cool! No more slow display list building because of an offscreen throbber animation. ::: layout/painting/RetainedDisplayListBuilder.cpp:637 (Diff revision 3) > + if (currentFrame->IsStackingContext()) { > + CRR_LOG("Frame belongs to stacking context frame %p\n", currentFrame); > + // If we found an intermediate stacking context with an existing display item > + // then we can store the dirty rect there and stop. > + if (currentFrame != mBuilder.RootReferenceFrame() && > + currentFrame->HasDisplayItems()) { I forget: Does this also return true if any descendant frames have display items? It looks like we're not rebuilding any part of the stacking context if the stacking context does not have display items. Why is that ok? Couldn't there be a case where the stacking context had no display items before, and then something inside the stacking context changed so that the stacking context needs display items now? ::: layout/painting/RetainedDisplayListBuilder.cpp:652 (Diff revision 3) > + currentFrame->SetProperty(nsDisplayListBuilder::DisplayListBuildingRect(), data); > + currentFrame->SetHasOverrideDirtyRegion(true); > + aOutFramesWithProps->AppendElement(currentFrame); > + } > + data->mDirtyRect.UnionRect(data->mDirtyRect, overflow); > + CRR_LOG("Adding area to stacking context draw area: %d %d %d %d\n", overflow.x, overflow.y, overflow.width, overflow.height); Should probably break this line somewhere. ::: layout/painting/RetainedDisplayListBuilder.cpp:766 (Diff revision 3) > + f->DeleteProperty(nsDisplayListBuilder::DisplayListBuildingRect()); > + f->DeleteProperty(nsDisplayListBuilder::DisplayListBuildingDisplayPortRect()); Can you add a comment that explains the life cycle of these properties?

Comment on attachment 8913533 [details] Bug 1404181 - Part 22: Make sure we mark frames as modified any time they change position or style data and make sure we don't accidentally mark the root as being modified when we don't need to. https://reviewboard.mozilla.org/r/184902/#review196548 ::: dom/animation/KeyframeEffectReadOnly.cpp:977 (Diff revision 3) > EffectSet::GetOrCreateEffectSet(mTarget->mElement, mTarget->mPseudoType); > effectSet->AddEffect(*this); > mInEffectSet = true; > UpdateEffectSet(effectSet); > + if (mTarget->mElement->GetPrimaryFrame()) { > + mTarget->mElement->GetPrimaryFrame()->MarkNeedsDisplayItemRebuild(); What about continuation frames?

Comment on attachment 8913534 [details] Bug 1404181 - Part 23: Only rebuild items within a displayport when the displayport changes, rather than rebuilding the whole document. https://reviewboard.mozilla.org/r/184904/#review196550 ::: dom/base/nsDOMWindowUtils.cpp:477 (Diff revision 3) > + nsLayoutUtils::InvalidateForDisplayPortChange(content, !!currentData, > + currentData->mRect, displayport); Either !!currentData is always true or currentData->mRect can crash.

Comment on attachment 8913535 [details] Bug 1404181 - Part 24: Rebuild all display items when we encounter a blend mode, since we can't easily track changes to whether we need the blend container or not. https://reviewboard.mozilla.org/r/184906/#review196552 ::: layout/generic/nsFrame.cpp:2780 (Diff revision 3) > + // to the stacking context frame, or the blend mode frame (e.g. by moving > + // an intermediate frame). > + // When we gain/remove a blend container item, we need to mark this frame > + // as invalid and have the full display list for merging to track > + // the change correctly. > + // It seems really hard to track this in advanced, as the bookkeeping advanced -> advance ::: layout/generic/nsIFrame.h:4317 (Diff revision 3) > * True if frame has will-change, and currently has display > * items consuming some of the will-change budget. > */ > bool mMayHaveWillChangeBudget : 1; > > + bool mBuiltBlendContainer : 1; need to adjust the comment about the size of the hole again

Comment on attachment 8913536 [details] Bug 1404181 - Part 25: Force rebuilding when we detect changes to the displaylist that didn't have invalidations. Also forces rebuilding of the canvas frame every time so that AddExtraBackgroundItems produces a consistent result. https://reviewboard.mozilla.org/r/184908/#review196554 ::: layout/generic/nsSubDocumentFrame.cpp:456 (Diff revision 3) > + // The value of needsOwnLayer can change between builds without > + // an invalidation recorded for this frame. If this happens, Can you add an example of when this might happen? ::: layout/generic/nsSubDocumentFrame.cpp:468 (Diff revision 3) > + // We could just invalidate the old and new frame > + // areas and save some work here. Isn't that what RetainedDisplayListBuilder does for the root document? Could it descend into subdocuments and do it for them as well? ::: layout/painting/nsDisplayList.cpp:1265 (Diff revision 3) > + // We are forcing a rebuild of nsDisplayCanvasBackgroundColor to make sure > + // that the canvas background color will be set correctly, and that only one > + // unscrollable item will be created. It feels like we could be doing better here. But I suppose we can fix it up later.

Comment on attachment 8913537 [details] Bug 1404181 - Part 26: Skip DLBI for reused items (since they must not be invalid). https://reviewboard.mozilla.org/r/184910/#review196556

Comment on attachment 8913538 [details] Bug 1404181 - Part 27: Add some retained-dl debugging tools. https://reviewboard.mozilla.org/r/184912/#review196558

Comment on attachment 8913532 [details] Bug 1404181 - Part 21: Add RetainedDisplayListBuilder with the code for doing partial display list builds, and merging it into an existing display list. https://reviewboard.mozilla.org/r/184900/#review196514 > How about: > > aFrames.swap(*frames); > frames->clear(); > > instead of the loop Going to leave this here, since Miko has a follow-up where we does the WeakFrame -> nsIFrame conversion at this point, and we need the loop. > I forget: Does this also return true if any descendant frames have display items? > > It looks like we're not rebuilding any part of the stacking context if the stacking context does not have display items. Why is that ok? Couldn't there be a case where the stacking context had no display items before, and then something inside the stacking context changed so that the stacking context needs display items now? Not quite, it's that we only stop converting the dirty rect into ancestor coordinate space at this stacking context if we had an existing display item for that stacking context. The idea is that we can do MarkFrameForDisplayIfVisible to rebuild the stacking context display item itself (which will merge into the old list without needing infront/behind items, since it'll just match to the same spot), and then we can use the override dirty area to build what actually changed within the stacking context. If we don't have any existing display item for the stacking context, then we won't actually be recursing into the merge algorithm with the matched item, so we can't do this optimization. In this case we continue the outer loop, and keep bubbling up to the next stacking context.

Comment on attachment 8913536 [details] Bug 1404181 - Part 25: Force rebuilding when we detect changes to the displaylist that didn't have invalidations. Also forces rebuilding of the canvas frame every time so that AddExtraBackgroundItems produces a consistent result. https://reviewboard.mozilla.org/r/184908/#review196554 > Isn't that what RetainedDisplayListBuilder does for the root document? Could it descend into subdocuments and do it for them as well? It could, but finding the set of to-be-painted subdocs can be a pain, and we have to do the equivalent of EnterPresShell to check if the caret frame changed. I'll leave this for now, but add a comment explaining that we could do it there if it ever shows up as mattering.

Comment on attachment 8913524 [details] Bug 1404181 - Part 15: Compute scrollbar thumb AGR state in advance, rather than setting it during building. https://reviewboard.mozilla.org/r/184884/#review195086 > I think these changes are on mozilla-central now. Indeed, dropped.

(In reply to Miko Mynttinen [:miko] from comment #115) > (In reply to Markus Stange [:mstange] from comment #72) > > Comment on attachment 8913519 [details] > > Bug 1404181 - Part 10: Track theme geometry contributions per-frame so that > > can remove contributions from invalidated frames. > > > > https://reviewboard.mozilla.org/r/184874/#review195048 > > > > ::: layout/painting/nsDisplayList.h:3694 > > (Diff revision 2) > > > const nsRect& aBackgroundRect); > > > virtual ~nsDisplayThemedBackground(); > > > > > > + void Destroy(nsDisplayListBuilder* aBuilder) override > > > + { > > > + aBuilder->UnregisterThemeGeometry(mFrame); > > > > Maybe add a comment that it's fine to remove all theme geometries for the > > frame, because there will never be more than one nsDisplayThemedBackground > > per frame. > > > > Actually, if that's the case, do we need to have arrays in the hashtable at > > all? > > The reason for using the array is because nsTreeBodyFrame might register > more than one ThemeGeometry, one for each selected row. > http://searchfox.org/mozilla-central/rev/ > d0448c970093f94bd986a21d3a9e8c366b316eb4/layout/xul/tree/nsTreeBodyFrame. > cpp#2881 > > Currently though, I am not sure if any UI elements[1] using this code > actually allow multiple selection. > > [1] http://searchfox.org/mozilla-central/search?q=mac-source-list&path= It looks like when Destroy() is called, we delete all entries for that frame. What happens when we build a second item (during a partial build), so that we have two alive, then run merging and call Destroy on one of them? Do we end up losing both copies? Might need to tag entries with the display item that added them, and make sure we clean up only what's our.

Comment on attachment 8913529 [details] Bug 1404181 - Part 18: Use nsPresArena for the display list builder arena since it supports recycling. https://reviewboard.mozilla.org/r/184894/#review195104 > Would be nice to initialize this statically, maybe with some nsDisplayItemTypesList.h #include magic and constexpr? That would be cool! I had a look, nsDisplayItemTypesList.h doesn't encode the actual class name, only the identifier, so there's no easy way to get sizeof() without rewriting that. The power of 2 functions we use aren't constepxr either, and the implementation is pretty complex and platform dependent.

Comment on attachment 8913514 [details] Bug 1404181 - Part 5: Store an annotated list of rectangles in nsDisplayLayerEventRegions so that we can remove contributions that belong to invalidated frames. https://reviewboard.mozilla.org/r/184864/#review193722 > I don't know, I haven't thought through the alreadyHadRegions case. I had more of a think about this. I think that if we previously had two touch-actions (and both were in the dispatch to content region), and then we had a partial update that removed one of them, we wouldn't remove the other. That's probably not a big deal really, but it's easy to fix. I moved the combining into the final computation of DispatchToContentHitRegion() that adds the touch regions if there are multiple.

(In reply to Matt Woodrow (:mattwoodrow) from comment #127) > (In reply to Miko Mynttinen [:miko] from comment #115) > > (In reply to Markus Stange [:mstange] from comment #72) > > > Comment on attachment 8913519 [details] > > > Bug 1404181 - Part 10: Track theme geometry contributions per-frame so that > > > can remove contributions from invalidated frames. > > > > > > https://reviewboard.mozilla.org/r/184874/#review195048 > > > > > > ::: layout/painting/nsDisplayList.h:3694 > > > (Diff revision 2) > > > > const nsRect& aBackgroundRect); > > > > virtual ~nsDisplayThemedBackground(); > > > > > > > > + void Destroy(nsDisplayListBuilder* aBuilder) override > > > > + { > > > > + aBuilder->UnregisterThemeGeometry(mFrame); > > > > > > Maybe add a comment that it's fine to remove all theme geometries for the > > > frame, because there will never be more than one nsDisplayThemedBackground > > > per frame. > > > > > > Actually, if that's the case, do we need to have arrays in the hashtable at > > > all? > > > > The reason for using the array is because nsTreeBodyFrame might register > > more than one ThemeGeometry, one for each selected row. > > http://searchfox.org/mozilla-central/rev/ > > d0448c970093f94bd986a21d3a9e8c366b316eb4/layout/xul/tree/nsTreeBodyFrame. > > cpp#2881 > > > > Currently though, I am not sure if any UI elements[1] using this code > > actually allow multiple selection. > > > > [1] http://searchfox.org/mozilla-central/search?q=mac-source-list&path= > > It looks like when Destroy() is called, we delete all entries for that frame. > > What happens when we build a second item (during a partial build), so that > we have two alive, then run merging and call Destroy on one of them? Do we > end up losing both copies? > > Might need to tag entries with the display item that added them, and make > sure we clean up only what's our. I think you are right, good catch. Fixing this is probably just the matter of replacing nsIFrame key with nsDisplayItem key.

Comment on attachment 8913519 [details] Bug 1404181 - Part 10: Track theme geometry contributions per-frame so that can remove contributions from invalidated frames. https://reviewboard.mozilla.org/r/184874/#review195048 > Maybe add a comment that it's fine to remove all theme geometries for the frame, because there will never be more than one nsDisplayThemedBackground per frame. > > Actually, if that's the case, do we need to have arrays in the hashtable at all? As discussed in the bug, we do need multiple, and there's an issue with clearing them all at once. We're going to fix this as a follow-up (since the issues don't affect us when preffed off), so that we can land before things bitrot.