414894 - Remove content arena

Status: RESOLVED FIXED

Description

[Stuart Parmenter]

We should get rid of the content arena.  The idea was good but the results are flawed.

Comment 1

[Olli Pettay [:smaug]]

So we're ready to regress tp 1.5% - if I'm reading qm-mini-xp01 talos correctly?
We have still *significantly* worse tp_memset regression than content arena.
And the changes to content arena have possibly removed memset regression.

I'm not against removing content arenas, just want to understand pros and cons
of removal.
Though, if jemalloc helps with fragmentation, the need (even theoretical) for 
content arena sure might go away.

Comment 2

[Stuart Parmenter]

There are a few pros for the content arena:  namely, it removes a large number of allocations.  I don't think the perf wins were anything more than a significant number of fewer allocations.

Unfortunately there are a handful of pretty bad cons: worse fragmentation, more memory use.  We discussed several ways to replace the arena with a better implementation, but none of them were optimal.  The fragmentation in the relatively small arena is going to be significantly worse than the fragmentation you'll get in the bigger global allocation pool.  In cases where we might move nodes from one document to another, we don't believe that you can compact the arena much due to fragmentation in the common case.  This is going to result in most likely entire arenas getting stuck in memory.

If, at some point, we're able to do something like one arena per top-level window or per document (the whole thing) with the ability to copy nodes out we might want to revisit this.

Comment 3

[[e-mail]]

-> me. per discussion with stuart, probably want this for b3.

will have patch later today

Comment 4

[[e-mail]]

Created attachment 300958 [details] [diff] [review]
back it out

i rolled this patch by:

1) reverting the MH changes in bug 408720
2) where there were conflicts in reverting bug 403830, i checked cvs logs to see what'd gone in since, made sure the backout wouldn't affect them, and manually reverted
3) fixed NS_NewMathMLElement() (bug 355548).
4) revved the nsINode IID and updated contentbase.gqi.

Comment 5

[Olli Pettay [:smaug]]

Comment on attachment 300958 [details] [diff] [review]
back it out

>Index: content/base/src/contentbase.gqi
...
>-%pseudo-iid nsINode cf677826-d7f1-4ec5-bf3a-d41811ac5846
>+%pseudo-iid nsINode d1c2e967-854a-436b-bfa5-f6a49a974674
Um, I don't like whole .gqi thing, but hopefully it one day shows some
perf boost :)

> void
>+nsAttrAndChildArray::Compact()
Could you perhaps file a separate bug to remove ::Compact(), thanks.

> // Attribute helper class used to wrap up an attribute with a dom
> // object that implements nsIDOMAttr, nsIDOM3Attr, nsIDOMNode, nsIDOM3Node
>-class nsDOMAttribute : public nsIAttribute,
>-                       public nsIDOMAttr,
>-                       public nsIDOM3Attr
>+class nsDOMAttribute : public nsIDOMAttr,
>+                       public nsIDOM3Attr,
>+                       public nsIAttribute
The original change is something we should keep. So either don't change it here or file
a new bug to move nsIAttribute to be the first one to inherit.

r=me if this passes mochitest.

Comment 6

[[e-mail]]

Created attachment 300973 [details] [diff] [review]
v2

added back the nsDOMAttribute change. looks like mochitests are unchanged with this patch - they fail on a clean pull for me. :/

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 300973 [details] [diff] [review]
v2

We do actually depend on that nsIAttribute is the first inherited class in XPConnect these days, so definitely leave that as is.

Would be great to investigate if the Compact call is useful or not, stuart might be able to pull some numbers.

Comment 8

[Stuart Parmenter]

Comment on attachment 300973 [details] [diff] [review]
v2

schrep: we still want this for b3?

Comment 9

[Mike Schroepfer]

Comment on attachment 300973 [details] [diff] [review]
v2

Yep we do to prevent any edge-case memory spikes.  We should re-run our numbers after this landing.

Comment 10

[[e-mail]]

checked in. windows gave a bunch of trouble over gqi, and needed clobbering, but otherwise things went fine.

Comment 11

[Hideo Oshima]

I think this fix causes bug 415422.

Comment 12

[Olli Pettay [:smaug]]

So tp/tp2 regressed as expected, but did talos-memset go down?

Comment 13

[[e-mail]]

(In reply to comment #4)
>4) revved the nsINode IID and updated contentbase.gqi.
As nsIContent inherits from nsINode shouldn't it have been revved too?

Comment 14

[Mike Schroepfer]

(In reply to comment #12)
> So tp/tp2 regressed as expected, but did talos-memset go down?
> 

Looks like it did slightly:

http://graphs.mozilla.org/#spst=range&spstart=1201478400&spend=1202059315&bpst=Cursor&bpstart=1201478400&bpend=1202059315&m1tid=53222&m1bl=0&m1avg=1&m2tid=53258&m2bl=0&m2avg=1&m3tid=53240&m3bl=0&m3avg=1&m4tid=53280&m4bl=0&m4avg=1

And this looks to be about a 2% tp hit which isn't great.   I'll be curious to see how both are affected by the new allocator - we should re-measure then.

Comment 15

[Mike Schroepfer]

Note that Mac Tp is unchanged 


http://graphs.mozilla.org/#spst=range&spstart=1201478400&spend=1202052745&bpst=Cursor&bpstart=1201478400&bpend=1202052745&m1tid=103002&m1bl=0&m1avg=0&m2tid=103067&m2bl=0&m2avg=0&m3tid=103117&m3bl=0&m3avg=0&m4tid=102925&m4bl=0&m4avg=0


We are pretty sure their native allocator is much better than on windows (replacement with jemalloc has much less effect) - that makes me hopeful that replacing the windows allocator will take back these wins.