Closed Bug 702240 Opened 11 years ago Closed 11 years ago

[CC] Investigate nsPurpleBuffer::Block sizing


(Core :: XPCOM, defect)

Not set





(Reporter: smaug, Assigned: smaug)


(Whiteboard: [clownshoes])


(1 file)

If my test is right, the size of block is 2056 (on 64bit), and jemalloc seems to
reserve from 4096 bucket.
Assignee: nobody → bugs
Attached patch patchSplinter 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)
Attachment #574285 - Flags: review?(continuation)
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.
Attachment #574285 - Flags: review?(continuation) → review+
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [clownshoes]
Version: unspecified → Trunk
Closed: 11 years ago
OS: All → Linux
Hardware: All → x86_64
Resolution: --- → FIXED
Version: Trunk → unspecified
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.
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.
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.
What is sizeof(Block) on 32-bit?
The size used to be 1028, and should be now 2044, if I'm counting right.
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.
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.