Looking up the display item data for a display item requires dereferencing the nsIFrame*, which is almost certainly a case miss during FrameLayerBuilder. If we pre-load these during building we can do while the frame is already in the cache, plus we get to retain the lookup with retained-dl.

This will let us add code that runs after we've fully constructed the display item (so that the virtual GetPerFrameKey is usable), where we can initialize the display item data.

The previous ownership of DisplayItemData required that the mDisplayItems hashtable held the only strong reference, and the list on each frame held a raw pointer to the items. When the entry gets removed from mDisplayItems it would call the destructor, which was responsible for cleaning up all the references from the frames. Since we now want to hold a strong reference from nsDisplayItem (we don't want to pay the cost of tracking down all references and clearing them via WeakPtr or similar), we need to change this a bit. I added the Disconnect method, so that whenever we clear the mDisplayItems reference, we clean up all the raw pointers from the frame lists. Any remaining strong references from display items will live longer, but we'll detect that they're disconnected if we try paint again, and trigger creation of a new one.

Comment on attachment 8949626 [details] [diff] [review] Part 1: Add a static constructor method for display items Review of attachment 8949626 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/painting/nsDisplayList.h @@ +1954,5 @@ > class nsDisplayWrapList; > > +template<typename T, typename... Args> > +T* > +MakeDisplayItem(nsDisplayListBuilder* aBuilder, Args&&... aArgs) We should probably mark this MOZ_ALWAYS_INLINE

Comment on attachment 8949627 [details] [diff] [review] Part 2: Lookup DisplayItemData during display list building Review of attachment 8949627 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me, let's see what Markus says. ::: layout/painting/FrameLayerBuilder.cpp @@ +340,5 @@ > MOZ_COUNT_CTOR(LayerManagerData); > } > ~LayerManagerData() { > MOZ_COUNT_DTOR(LayerManagerData); > + nit: whitespace ::: layout/painting/nsDisplayList.h @@ +1957,5 @@ > T* > MakeDisplayItem(nsDisplayListBuilder* aBuilder, Args&&... aArgs) > { > + T* item = new (aBuilder) T(aBuilder, mozilla::Forward<Args>(aArgs)...); > + nit: whitespace @@ +1961,5 @@ > + > + const mozilla::SmallPointerArray<mozilla::DisplayItemData>& array = > + item->Frame()->DisplayItemData(); > + for (uint32_t i = 0; i < array.Length(); i++) { > + mozilla::DisplayItemData* did = array.ElementAt(i); If we're going to stop using AssertDisplayItemData here we may as well kill it altogether. Which may be fair, since we're changing the whole lifetime model here anyway. @@ +4894,5 @@ > virtual void GetMergedFrames(nsTArray<nsIFrame*>* aFrames) const override > { > aFrames->AppendElements(mMergedFrames); > } > + nit: whitespace

Comment on attachment 8949627 [details] [diff] [review] Part 2: Lookup DisplayItemData during display list building Review of attachment 8949627 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/painting/nsDisplayList.h @@ +1963,5 @@ > + item->Frame()->DisplayItemData(); > + for (uint32_t i = 0; i < array.Length(); i++) { > + mozilla::DisplayItemData* did = array.ElementAt(i); > + if (did->GetDisplayItemKey() == item->GetPerFrameKey()) { > + if (did->GetMergedFrameCount() == 1) { which is it, GetMergedFramesCount() or HasMergedFrames()?

Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2b4d117c27dc Part 1: Add a static constructor function for display items. r=Bas https://hg.mozilla.org/integration/mozilla-inbound/rev/27640f52e188 Part 2: Lookup DisplayItemData during display list building when the frame is already in cache. r=Bas,mstange

https://hg.mozilla.org/mozilla-central/rev/2b4d117c27dc https://hg.mozilla.org/mozilla-central/rev/27640f52e188

Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/27f0762d4472 Followup to part 2, add hunk that got lost during rebasing.

Backed out 3 changesets (bug 1436904) for many crashes see bugs: 1440281, 1440302, 1440303, 1440313. a=backout Backout: https://hg.mozilla.org/mozilla-central/rev/5e9bd04333f20e00911b8c1dfbf2b2e070c61e2d

Need to re-open due to backout ?

Pushed by mwoodrow@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d8fa6d30c99b Part 1: Add a static constructor function for display items. r=Bas https://hg.mozilla.org/integration/mozilla-inbound/rev/dec7c40f3be3 Part 2: Lookup DisplayItemData during display list building when the frame is already in cache. r=Bas,mstange

https://hg.mozilla.org/mozilla-central/rev/d8fa6d30c99b https://hg.mozilla.org/mozilla-central/rev/dec7c40f3be3

Can we opt out of this when running with WebRender?

I see talos perf improvements here: == Change summary for alert #11717 (as of Fri, 23 Feb 2018 05:25:50 GMT) == Improvements: 26% basic_compositor_video windows7-32 opt e10s stylo 3.67 -> 2.71 6% displaylist_mutate linux64 opt e10s stylo 2,589.08 -> 2,426.58 6% displaylist_mutate linux64 pgo e10s stylo 2,297.75 -> 2,154.58 For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=11717