Closed
Bug 1367206
Opened 7 years ago
Closed 7 years ago
Look into optimizing out the hashtable lookups from nsContainerFrame::DestroyFrom and nsContainerFrame::SafelyDestroyFrameListProp
Categories
(Core :: Layout, enhancement)
Tracking
()
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: MatsPalmgren_bugz)
References
Details
Attachments
(1 file, 2 obsolete files)
7.70 KB,
patch
|
jfkthame
:
review+
|
Details | Diff | Splinter Review |
We end up doing three hashtable lookups for every container frame destruction, via nsContainerFrame::SafelyDestroyFrameListProp. Most container frames have no frame lists there. This might be helped by bug 1365982 too.
Updated•7 years ago
|
Whiteboard: [qf] → [qf:p1]
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mats
Assignee | ||
Comment 1•7 years ago
|
||
I think all these child list properties are rare so iterating over the properties once upfront instead of four separate lookups should be a win. We can improve it further with frame bits I guess, but let's do that in follow-up bugs if necessary. We could also improve SafelyDestroyFrameListProp itself (that does a GetProperty followed by RemoveProperty) but let's do that in a follow-up bug and fix all such patterns. (maybe you already filed one?) It would be great to get rid of SafelyDestroyFrameListProp at some point but that probably requires a lot more research because we introduced it to fix crash bugs with security issues... https://treeherder.mozilla.org/#/jobs?repo=try&revision=da49ebfe8eb2029498209f81f37db5bfaa09ab4b
Attachment #8872174 -
Flags: review?(jfkthame)
Assignee | ||
Comment 2•7 years ago
|
||
Comment on attachment 8872174 [details] [diff] [review] Iterate the frame property list once to collect which child list properties we have Actually, I think we can do better than this with some work. Hold the review for now.
Attachment #8872174 -
Flags: review?(jfkthame)
Comment 3•7 years ago
|
||
Just wondering, now that frame properties no longer involve a hashtable lookup, how much value is there left in this?
Assignee | ||
Comment 4•7 years ago
|
||
> Just wondering, now that frame properties no longer involve
> a hashtable lookup, how much value is there left in this?
Probably not much, but I don't see any downsides either - it's faster
to iterate once instead of four times. That said, I think there are
still ways we can improve frame properties, so I regard this as
a temporary measure.
(I'll remove the mProperties null-checks once your nsTArray patch lands)
Attachment #8872174 -
Attachment is obsolete: true
Attachment #8873137 -
Flags: review?(jfkthame)
Updated•7 years ago
|
Attachment #8873137 -
Flags: review?(jfkthame) → review+
Assignee | ||
Comment 5•7 years ago
|
||
> (I'll remove the mProperties null-checks once your nsTArray patch lands) Actually, I think it's still worth "null-checking" (since it's the common case) in nsContainerFrame::DestroyFrom to avoid some branches there. So, adding a FrameProperties::IsEmpty() for that seems worthwhile. The essential difference to the last patch: In layout/base/FrameProperties.h > /** > + * Return true if we have no properties, otherwise return false. > + */ > + bool IsEmpty() const { return mProperties.IsEmpty(); } > + > + /** In layout/generic/nsContainerFrame.cpp < + if (mProperties) { --- > + if (MOZ_UNLIKELY(!mProperties.IsEmpty())) {
Attachment #8873137 -
Attachment is obsolete: true
Attachment #8873351 -
Flags: review?(jfkthame)
Comment 6•7 years ago
|
||
Comment on attachment 8873351 [details] [diff] [review] Iterate the frame property list once to collect which child list properties we have Review of attachment 8873351 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #8873351 -
Flags: review?(jfkthame) → review+
Pushed by mpalmgren@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/10328a332b65 Iterate the frame property list once to collect which child list properties we have. r=jfkthame
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/10328a332b65
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•2 years ago
|
Performance Impact: --- → P1
Whiteboard: [qf:p1]
You need to log in
before you can comment on or make changes to this bug.
Description
•