Closed Bug 1367206 Opened 2 years ago Closed 2 years ago

Look into optimizing out the hashtable lookups from nsContainerFrame::DestroyFrom and nsContainerFrame::SafelyDestroyFrameListProp

Categories

(Core :: Layout, enhancement)

53 Branch
enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: bzbarsky, Assigned: mats)

References

Details

(Whiteboard: [qf:p1])

Attachments

(1 file, 2 obsolete files)

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.
Whiteboard: [qf] → [qf:p1]
Assignee: nobody → mats
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)
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)
Just wondering, now that frame properties no longer involve a hashtable lookup, how much value is there left in this?
> 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)
Attachment #8873137 - Flags: review?(jfkthame) → review+
> (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 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
https://hg.mozilla.org/mozilla-central/rev/10328a332b65
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.