[CC] Investigate nsPurpleBuffer::Block sizing

RESOLVED FIXED in mozilla11

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: smaug, Assigned: smaug)

Tracking

unspecified
mozilla11
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [clownshoes])

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

6 years ago
Assignee: nobody → bugs
(Assignee)

Comment 1

6 years ago
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)
Attachment #574285 - Flags: review?(continuation)
(Assignee)

Comment 2

6 years ago
https://hg.mozilla.org/try/rev/531b0d50d43c
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.
Attachment #574285 - Flags: review?(continuation) → review+
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [clownshoes]
Version: unspecified → Trunk
(Assignee)

Comment 4

6 years ago
https://hg.mozilla.org/mozilla-central/rev/eb84780783ed
Status: NEW → RESOLVED
Last Resolved: 6 years ago
OS: All → Linux
Hardware: All → x86_64
Resolution: --- → FIXED
Version: Trunk → unspecified
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.
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.
(Assignee)

Comment 7

6 years ago
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?
(Assignee)

Comment 9

6 years ago
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.