1377818 - Consider reserving some storage in nsContentList::mElements

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

I noticed in Speedometer profiles that nsContentList::PopulateSelf shows up, and around 30% of its cost comes form mallocs.

I added some logging to print out the size of the array at the end of that function and if you plot it, you will get something looking like this: https://plot.ly/~ehsana/0/.

Clearly this size is under the control of content, but for this workload 10 seems to be a good size to pick for reserved storage.

The unfortunate news is that right now the size of nsContentList on 64-bit platforms right now is exactly 128 bytes, so reserving any storage here will bump it up to the next jemalloc bucket.

Olli, what do you think?  Would you be OK with increasing the size here?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

We shouldn't have that many contentlists per page, I hope.

Isn't for example the nodelist returned by getElementByName larger than a normal nsContentList?

Anyhow, I think using AutoTArray sounds reasonable.

Comment 2

[(no longer active)]

(In reply to Olli Pettay [:smaug] from comment #1)
> We shouldn't have that many contentlists per page, I hope.

I found this sad pattern in an old jquery copy that the flightjs code in Speedometer uses:

https://github.com/WebKit/webkit/blob/f4ea0d3022e7453ae5262a7c6dbf13a5e99ab002/PerformanceTests/Speedometer/resources/flightjs-example-app/components/jquery/jquery.js#L5826
https://github.com/WebKit/webkit/blob/f4ea0d3022e7453ae5262a7c6dbf13a5e99ab002/PerformanceTests/Speedometer/resources/flightjs-example-app/components/jquery/jquery.js#L5846

Our LRU cache prevents us from creating many NodeList objects in this case, but we do create a lot if script calls jquery.remove() on many invididual nodes unfortunately.

(This has been fixed in jQuery upstream, I already checked.)

I think this is the main source of these PopulateSelf() calls showing up in profiles.  After eliminating the malloc overhead, the remaining cost is all in this loop: <https://searchfox.org/mozilla-central/rev/5bb6dfcb3355a912c20383578bc435c1d4ecf575/dom/base/nsContentList.cpp#925> with the |cur->IsElement()| check taking up about more than 50% of the cost of the function.  Basically we're paying the cost of pointer chasing getting the next elements, which is really hard to fix without improving the locality of the DOM nodes we allocate per page...

> Isn't for example the nodelist returned by getElementByName larger than a
> normal nsContentList?

Yes, I forgot to mention in comment 0 that the 128 size was only about nsContentList, all of its subclasses already fall into the next jemalloc bucket.

> Anyhow, I think using AutoTArray sounds reasonable.

OK, will do.  Thanks.

Comment 3

[(no longer active)]

Attachment: Reserve 10 elements of storage in nsContentList::mElements to avoid paying excessive allocation overhead when manipulating content lists

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f47b5163a36
Reserve 10 elements of storage in nsContentList::mElements to avoid paying excessive allocation overhead when manipulating content lists; r=smaug

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1f47b5163a36