Last Comment Bug 702240 - [CC] Investigate nsPurpleBuffer::Block sizing
: [CC] Investigate nsPurpleBuffer::Block sizing
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: x86_64 Linux
-- normal (vote)
: mozilla11
Assigned To: Olli Pettay [:smaug]
: Nathan Froyd [:froydnj][high latency until 6 March]
Depends on:
  Show dependency treegraph
Reported: 2011-11-14 05:13 PST by Olli Pettay [:smaug]
Modified: 2011-12-01 20:52 PST (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (659 bytes, patch)
2011-11-14 06:06 PST, Olli Pettay [:smaug]
continuation: review+
Details | Diff | Splinter Review

Description User image Olli Pettay [:smaug] 2011-11-14 05:13:48 PST
If my test is right, the size of block is 2056 (on 64bit), and jemalloc seems to
reserve from 4096 bucket.
Comment 1 User image Olli Pettay [:smaug] 2011-11-14 06:06:44 PST
Created attachment 574285 [details] [diff] [review]

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 User image Olli Pettay [:smaug] 2011-11-14 06:07:53 PST
Comment 3 User image Andrew McCreight [:mccr8] 2011-11-14 06:26:02 PST
Comment on attachment 574285 [details] [diff] [review]

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 User image Olli Pettay [:smaug] 2011-11-14 08:19:09 PST
Comment 5 User image Nicholas Nethercote [:njn] 2011-11-14 15:05:29 PST
Comment on attachment 574285 [details] [diff] [review]

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 User image Andrew McCreight [:mccr8] 2011-11-15 12:06:29 PST
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 User image Olli Pettay [:smaug] 2011-11-15 12:29:21 PST
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 User image Nicholas Nethercote [:njn] 2011-11-15 13:52:42 PST
What is sizeof(Block) on 32-bit?
Comment 9 User image Olli Pettay [:smaug] 2011-11-15 13:56:09 PST
The size used to be 1028, and should be now 2044, if I'm counting right.
Comment 10 User image Andrew McCreight [:mccr8] 2011-11-16 09:16:38 PST
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.

Note You need to log in before you can comment on or make changes to this bug.