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

Status: RESOLVED FIXED

(Core :: Layout, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Mats Palmgren (:mats)]

Attachment: Iterate the frame property list once to collect which child list properties we have

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

Comment 2

[Mats Palmgren (:mats)]

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.

Comment 3

[Jonathan Kew (:jfkthame)]

Just wondering, now that frame properties no longer involve a hashtable lookup, how much value is there left in this?

Comment 4

[Mats Palmgren (:mats)]

Attachment: Iterate the frame property list once to collect which child list properties we have

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

Comment 5

[Mats Palmgren (:mats)]

Attachment: Iterate the frame property list once to collect which child list properties we have

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

Comment 6

[Jonathan Kew (: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

Comment 7

[Pulsebot]

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

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/10328a332b65