1436904 - Lookup DisplayItemData during display list building

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement, P2)

Description

[Matt Woodrow (:mattwoodrow)]

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.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Add a static constructor method for display items

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.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Lookup DisplayItemData during display list building

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 3

[Bas Schouten (:bas.schouten)]

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 4

[Bas Schouten (:bas.schouten)]

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 5

[Markus Stange [:mstange]]

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()?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
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

Comment 7

[Raul Gurzau (:RaulG)]

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27f0762d4472
Followup to part 2, add hunk that got lost during rebasing.

Comment 9

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/27f0762d4472

Comment 10

[Dorel Luca [:dluca]]

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

Comment 11

[Jim Jeffery not reading bug-mail 1/2/11]

Need to re-open due to backout ?

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Stefan Hindli [:stefan_hindli]]

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

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Can we opt out of this when running with WebRender?

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

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