Closed Bug 1436904 Opened 2 years ago Closed 2 years ago

Lookup DisplayItemData during display list building

Categories

(Core :: Web Painting, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: mattwoodrow, Assigned: mattwoodrow, NeedInfo)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

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.
Attachment #8949626 - Flags: review?(bas)
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.
Attachment #8949627 - Flags: review?(bas)
Attachment #8949627 - Flags: review?(mstange)
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
Attachment #8949626 - Flags: review?(bas) → review+
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
Attachment #8949627 - Flags: review?(bas) → review+
Priority: -- → P2
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()?
Attachment #8949627 - Flags: review?(mstange) → review+
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
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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.
Depends on: 1440302
Depends on: 1440303
Depends on: 1440313
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
Flags: needinfo?(matt.woodrow)
Need to re-open due to backout ?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla60 → ---
Duplicate of this bug: 1440350
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
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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
Depends on: 1468738
You need to log in before you can comment on or make changes to this bug.