702240 - [CC] Investigate nsPurpleBuffer::Block sizing

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

If my test is right, the size of block is 2056 (on 64bit), and jemalloc seems to
reserve from 4096 bucket.

Comment 1

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

Attachment: patch

This should use the same kind of bucket as what is used right now, but
actually utilize the allocated memory for something.


(I just accidentally noticed the strange size of Block while looking at some
other possible optimizations)

Comment 2

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

https://hg.mozilla.org/try/rev/531b0d50d43c

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 574285 [details] [diff] [review]
patch

Review of attachment 574285 [details] [diff] [review]:
-----------------------------------------------------------------

Good catch.  I've always idly wondered about clownshoes bugs in the cycle collector, because there are a lot of almost-powers-of-two sized blocks, but I never actually looked into it.

Because I'm sure Nick will be interested, the number of suspected (purple) objects varies fairly widely.  Looking at my own browser right now using javascript.options.mem.log, the number of suspected objects ranges from 76 to 7173.  At the upper end of that, we currently have 55 blocks, so using Olli's numbers from comment 0, that is going to be about 100k of wasted space.  The purple buffer is cleared at the start of every CC, then added to in between CCs, so there's pretty much always something in a purple buffer when the CC isn't running.  The CC is triggered 5 seconds after the purple buffer hits 1000 entries, so that probably places a bounds on how big it can get.  That trigger may get tuned in the future, which would affect the impact on memory.

Comment 4

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

https://hg.mozilla.org/mozilla-central/rev/eb84780783ed

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 574285 [details] [diff] [review]
patch

Review of attachment 574285 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't seen the cycle collector show up in my slop investigations, but those mostly involved loading a couple of sites and then quitting, so it's possible this slop could be significant in longer browsing sessions.  Good catch!

::: xpcom/base/nsCycleCollector.cpp
@@ +747,5 @@
>  {
>  private:
>      struct Block {
>          Block *mNext;
> +        nsPurpleBufferEntry mEntries[255];

I would have (and you still can!) put a brief comment here explaining why it's 255.

Comment 6

[Andrew McCreight [:mccr8]]

I don't know if this is a real or not (I tried looking at the graphs but they were incomprehensible to me), but this is showing up as causing a few regressions on tree-management:

Talos Regression :( Dromaeo (DOM) decrease 1.69% on XP Firefox-Non-PGO
Talos Regression :( Dromaeo (jslib) decrease 3.4% on Linux Firefox-Non-PGO

Perhaps this is causing the block size to increase on 32-bit systems?  I guess this could be causing larger allocations there in situations where we suspected <= 128 nodes between CCs?  Sorry, I should have thought about that.

Comment 7

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

This shouldn't have changed what kinds of jemalloc buckets are used. Just change that the ones 
which are used, are used as much as possible.

Comment 8

[Nicholas Nethercote [inactive]]

What is sizeof(Block) on 32-bit?

Comment 9

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

The size used to be 1028, and should be now 2044, if I'm counting right.

Comment 10

[Andrew McCreight [:mccr8]]

No other regressions blamed on this patch since then.  Seems a little weird that two different tests on two separate platforms could regress.  Maybe these are noise, or maybe noise elsewhere covered it up.