Closed
Bug 1436904
Opened 7 years ago
Closed 7 years ago
Lookup DisplayItemData during display list building
Categories
(Core :: Web Painting, enhancement, P2)
Core
Web Painting
Tracking
()
RESOLVED
FIXED
mozilla60
Tracking | Status | |
---|---|---|
firefox60 | --- | fixed |
People
(Reporter: mattwoodrow, Assigned: mattwoodrow, NeedInfo)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
116.70 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
14.29 KB,
patch
|
bas.schouten
:
review+
mstange
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•7 years ago
|
||
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)
Assignee | ||
Comment 2•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8949627 -
Flags: review?(mstange)
Comment 3•7 years ago
|
||
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 4•7 years ago
|
||
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+
Assignee | ||
Updated•7 years ago
|
Priority: -- → P2
Comment 5•7 years ago
|
||
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
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2b4d117c27dc https://hg.mozilla.org/mozilla-central/rev/27640f52e188
Status: NEW → RESOLVED
Closed: 7 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.
Comment 9•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/27f0762d4472
Comment 10•7 years ago
|
||
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)
Comment 11•7 years ago
|
||
Need to re-open due to backout ?
Updated•7 years ago
|
Status: RESOLVED → REOPENED
status-firefox60:
fixed → ---
Resolution: FIXED → ---
Target Milestone: mozilla60 → ---
Comment 13•7 years ago
|
||
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
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d8fa6d30c99b https://hg.mozilla.org/mozilla-central/rev/dec7c40f3be3
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 15•7 years ago
|
||
Can we opt out of this when running with WebRender?
Comment 16•7 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•