If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 55

Status

()

Core
Layout
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: bz, Assigned: mats)

Tracking

53 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:p1])

Attachments

(1 attachment, 2 obsolete attachments)

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)

Updated

4 months ago
Assignee: nobody → mats
(Assignee)

Comment 1

4 months ago
Created attachment 8872174 [details] [diff] [review]
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
Attachment #8872174 - Flags: review?(jfkthame)
(Assignee)

Comment 2

4 months 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)
Just wondering, now that frame properties no longer involve a hashtable lookup, how much value is there left in this?
(Assignee)

Comment 4

4 months ago
Created attachment 8873137 [details] [diff] [review]
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)
Attachment #8872174 - Attachment is obsolete: true
Attachment #8873137 - Flags: review?(jfkthame)
Attachment #8873137 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 5

4 months ago
Created attachment 8873351 [details] [diff] [review]
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())) {
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+

Comment 7

4 months ago
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
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.