TreeMatchContext::InitAncestors is rather malloc heavy

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Ehsan, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

a year ago
See for example this profile from running the VueJS speedometer tests: https://perfht.ml/2vY4Thg

I think most of these are coming from the nsTArray allocations under AncestorFilter::PushAncestor().  It'd also be nice to not malloc for the bloom filter, perhaps we can use Maybe<> to dynamically create the object on the stack when InitAncestors() is called?
Two thoughts:

1)  Stylo makes this go away, but may have its own overheads as well.  We should really do some speedometer measurement with stylo enabled.

2)  We could consider making mPopTargets and mHashes auto arrays.  I expect that those are the places where we're allocating, and it might be a cheap and simple fix for this until we ship stylo.
(Reporter)

Comment 2

a year ago
Hmm, it's kind of hard to figure out a good size of mHashes.  https://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/layout/style/nsCSSRuleProcessor.cpp#4041 uses a preallocated cache of 50 ancestors.  While running Speedometer, we mostly see mHashes in the tens or twenty items ranges.  But with general browsing, the numbers are all over the place.  The number 50 seems to come from the initial landing of this code in bug 705877.  I'm inclined to pick the same 50 for mHashes, and maybe 16 for mPopTargets as arrays larger than that seems to be rare.
(Reporter)

Comment 3

a year ago
Created attachment 8890109 [details] [diff] [review]
Try to avoid dynamic allocations in TreeMatchContext::InitAncestors if possible
Attachment #8890109 - Flags: review?(bzbarsky)
Comment on attachment 8890109 [details] [diff] [review]
Try to avoid dynamic allocations in TreeMatchContext::InitAncestors if possible

>+  mozilla::Maybe<Filter> mFilter;

So this part is pretty dangerous.  The filter is quite large, and if we have any recursion involving multiple TreeMatchContexts we will very quickly run out of stack.

I'd really prefer we left this part out of this patch, unless we have good evidence that this is not going to cause more out-of-stack crashes.

r=me on the array changes, though it would be good to document where those sizes come from (including "we totally just guessed with no measuring" if that's where they come from!).
Attachment #8890109 - Flags: review?(bzbarsky) → review+
(Reporter)

Comment 5

a year ago
(In reply to Boris Zbarsky [:bz] from comment #4)
> Comment on attachment 8890109 [details] [diff] [review]
> Try to avoid dynamic allocations in TreeMatchContext::InitAncestors if
> possible
> 
> >+  mozilla::Maybe<Filter> mFilter;
> 
> So this part is pretty dangerous.  The filter is quite large, and if we have
> any recursion involving multiple TreeMatchContexts we will very quickly run
> out of stack.
> 
> I'd really prefer we left this part out of this patch, unless we have good
> evidence that this is not going to cause more out-of-stack crashes.

Thanks for pointing that out.  I'd be happy to leave that part out.

> r=me on the array changes, though it would be good to document where those
> sizes come from (including "we totally just guessed with no measuring" if
> that's where they come from!).

For mHashes, I'd say something along the lines of:

  // 50 is chosen to be the same as the preallocated buffer size used in
  // TreeMatchContext::InitAncestors().  This value seems to be large
  // enough for Speedometer 2, although some casual testing browsing real
  // sites suggested they can easily require values much larger (in the
  // range of hundreds.)

For mPopTargets, I'd say something along the lines of:

  // 16 is chosen because it's enough to avoid most allocations for the
  // Speedometer 2 benchmark, and also it covers many cases in some casual
  // local browsing tests performed.  Higher values in the range of tens
  // were observed while testing through local browsing but they were rare.

Comment 6

a year ago
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8486350a848a
Try to avoid dynamic allocations in TreeMatchContext::InitAncestors if possible; r=bzbarsky

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8486350a848a
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.