Last Comment Bug 702240 - [CC] Investigate nsPurpleBuffer::Block sizing
: [CC] Investigate nsPurpleBuffer::Block sizing
Status: RESOLVED FIXED
[clownshoes]
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla11
Assigned To: Olli Pettay [:smaug] (vacation Aug 25-28)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-14 05:13 PST by Olli Pettay [:smaug] (vacation Aug 25-28)
Modified: 2011-12-01 20:52 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description Olli Pettay [:smaug] (vacation Aug 25-28) 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 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-11-14 06:06:44 PST
Created attachment 574285 [details] [diff] [review]
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] (vacation Aug 25-28) 2011-11-14 06:07:53 PST
https://hg.mozilla.org/try/rev/531b0d50d43c
Comment 3 Andrew McCreight [:mccr8] 2011-11-14 06:26:02 PST
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] (vacation Aug 25-28) 2011-11-14 08:19:09 PST
https://hg.mozilla.org/mozilla-central/rev/eb84780783ed
Comment 5 Nicholas Nethercote [:njn] 2011-11-14 15:05:29 PST
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] 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 Olli Pettay [:smaug] (vacation Aug 25-28) 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 Nicholas Nethercote [:njn] 2011-11-15 13:52:42 PST
What is sizeof(Block) on 32-bit?
Comment 9 Olli Pettay [:smaug] (vacation Aug 25-28) 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 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.