Last Comment Bug 741378 - Rejigger JS memory reporters to mirror jemalloc's
: Rejigger JS memory reporters to mirror jemalloc's
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All Linux
: -- normal (vote)
: mozilla14
Assigned To: Justin Lebar (not reading bugmail)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 790621
Blocks: 748627
  Show dependency treegraph
 
Reported: 2012-04-02 06:28 PDT by Mike Hommey [:glandium]
Modified: 2012-09-12 07:13 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (10.39 KB, patch)
2012-04-15 16:55 PDT, Justin Lebar (not reading bugmail)
n.nethercote: review+
Details | Diff | Splinter Review
Part 2: Send js-gc-heap-committed instead of js-gc-heap in telemetry. (3.19 KB, patch)
2012-04-15 18:46 PDT, Justin Lebar (not reading bugmail)
wmccloskey: review+
Details | Diff | Splinter Review

Description Mike Hommey [:glandium] 2012-04-02 06:28:27 PDT
Right after a fresh start of a local build with jemalloc, here is what I can see in about:memory:

77,393,920 B (100.0%) -- resident
├──42,512,384 B (54.93%) -- anonymous
│  ├──42,168,320 B (54.49%) ── anonymous, outside brk() [rw-p] [42]
│  └─────344,064 B (00.44%) ── anonymous, outside brk() [rwxp] [5]
├──33,800,192 B (43.67%) ++ shared-libraries
├─────995,328 B (01.29%) ++ other-files
├──────81,920 B (00.11%) ── main thread's stack [rw-p]
└───────4,096 B (00.01%) ── vdso [r-xp]

 29,451,480 B ── heap-allocated
 34,652,160 B ── heap-committed
       14.96% ── heap-committed-fragmentation
  2,428,928 B ── heap-dirty
 14,586,128 B ── heap-unallocated

I would expect heap-committed to somehow match anonymous RSS. On an actual (crazy) session (crazy meaning that it has about 2000 tabs, although only a few are loaded), it looks like this:
1,568,550,912 B (100.0%) -- resident
├──1,535,102,976 B (97.87%) -- anonymous
│  ├──1,528,332,288 B (97.44%) ── anonymous, outside brk() [rw-p] [177]
│  └──────6,770,688 B (00.43%) ── anonymous, outside brk() [rwxp] [46]
├─────31,657,984 B (02.02%) ++ shared-libraries
├──────1,572,864 B (00.10%) ++ other-files
├────────212,992 B (00.01%) ── main thread's stack [rw-p]
└──────────4,096 B (00.00%) ── vdso [r-xp]

1,043,472,504 B ── heap-allocated
1,079,672,832 B ── heap-committed
          3.35% ── heap-committed-fragmentation
    1,523,712 B ── heap-dirty
   64,871,168 B ── heap-unallocated

The difference would seem to come from the gc-heap, for which we don't have a committed value.
Comment 1 Justin Lebar (not reading bugmail) 2012-04-02 06:33:43 PDT
Could the gc-heap be 500mb, or is it obviously smaller?  Most of the GC heap should be committed, except for js-main-runtime-gc-heap-decommitted.
Comment 2 Mike Hommey [:glandium] 2012-04-02 06:44:24 PDT
It's 673M on the latter case, and 10M on the former case, which makes a significant amount of it not committed.
Comment 3 Justin Lebar (not reading bugmail) 2012-04-02 07:11:25 PDT
(In reply to Mike Hommey [:glandium] from comment #2)
> It's 673M on the latter case, and 10M on the former case, which makes a
> significant amount of it not committed.

Yes, I think JS's accounting assumes that as soon as it mmap's a chunk, that whole chunk is committed.  If it later decommits part of the chunk, that is tracked in gc-heap-decommitted.

It seems likely to me that the bulk of the difference between anonymous RSS and heap-committed is due to the JS engine.  Do you agree?

If so, what would you like to get fixed here, exactly?  JS's accounting of what's committed?
Comment 4 Mike Hommey [:glandium] 2012-04-02 07:21:20 PDT
Let's see:
heap-committed + js-gc-heap - jsmain-runtime-gc-heap-decommitted = 1,079,672,832 + 673,185,792 - 198,139,904 = 1,554,718,720

Compared to rss anonymous = 1,535,102,976

I'd say close enough (probably). It might be helpful to have these things made clearer in about:memory. Having heap-committed instead of heap-decommitted for gc-heap could be a step forward.
Additionally, it may be helpful to have something like heap-allocated for gc-heap, as well as a fragmentation indicator.
Comment 5 Mike Hommey [:glandium] 2012-04-02 07:26:35 PDT
Also, something like js-main-runtime-gc-heap-unused-fraction that does not include js-main-runtime-gc-decommitted might be helpful. Because 44.72% is pretty scary, but 21% is less scary (which is what i get for (js-main-runtime-gc-heap-arena-unused + js-main-runtime-gc-heap-chunk-clean-unused + js-main-runtime-gc-heap-chunk-dirty-unused) / (js-gc-heap - js-main-runtime-gc-heap-decommitted) [which is still a lot by the way]
Comment 6 Justin Lebar (not reading bugmail) 2012-04-02 07:31:57 PDT
> Yes, I think JS's accounting assumes that as soon as it mmap's a chunk, that whole chunk is 
> committed.  If it later decommits part of the chunk, that is tracked in gc-heap-decommitted.

Okay, so maybe this was wrong!  That would be nice.

> Having heap-committed instead of heap-decommitted for gc-heap could be a step forward.

I could get behind this.

> Additionally, it may be helpful to have something like heap-allocated for gc-heap

FWIW I believe that's currently gc-heap-committed - gc-heap-arena-unused.  But we could rejigger these, sure.

> Also, something like js-main-runtime-gc-heap-unused-fraction that does not include js-main-
> runtime-gc-decommitted might be helpful.

Sure, agreed.

> 21% is still a lot by the way

Close your eyes, click your heels, and say "there's no place like a compacting generational garbage collector"...
Comment 7 Justin Lebar (not reading bugmail) 2012-04-02 07:32:53 PDT
Nick, what do you think about the changes above?
Comment 8 Terrence Cole [:terrence] 2012-04-02 13:00:44 PDT
(In reply to Justin Lebar [:jlebar] from comment #6)
> > Yes, I think JS's accounting assumes that as soon as it mmap's a chunk, that whole chunk is 
> > committed.  If it later decommits part of the chunk, that is tracked in gc-heap-decommitted.
> 
> Okay, so maybe this was wrong!  That would be nice.

You were right originally, actually.  The idea is that since we are out of memory and allocating more, we are probably going to want to use that memory pretty soon.  If we have not used it by the next GC, then we decommit it at that point.  The hope is that doing it this way we will take fewer page faults on average.  I have no solid data supporting this -- it's probably not a terribly important effect either way.  As you point out, the real right solution is a compacting GC, and we're working hard to make that happen.

> > 21% is still a lot by the way
> 
> Close your eyes, click your heels, and say "there's no place like a
> compacting generational garbage collector"...

Coming soon to a SpiderMonkey near you.
Comment 9 Justin Lebar (not reading bugmail) 2012-04-02 13:05:21 PDT
> If we have not used it by the next GC, then we decommit it at that point.

Not every GC will end up decommitting, right?  It has to be a "big" one, for some definition of "big".  How often do those run?

(The heuristic may be fine, but we may still want to adjust the memory reporter to reflect the reality that memory isn't actually committed until it's touched.)
Comment 10 Terrence Cole [:terrence] 2012-04-02 14:43:31 PDT
(In reply to Justin Lebar [:jlebar] from comment #9)
> > If we have not used it by the next GC, then we decommit it at that point.
> 
> Not every GC will end up decommitting, right?  It has to be a "big" one, for
> some definition of "big".  How often do those run?

Someone more knowledgeable will have to chime in to answer that: our GC scheduling is bafflingly complex (and probably not complex enough yet).

> (The heuristic may be fine, but we may still want to adjust the memory
> reporter to reflect the reality that memory isn't actually committed until
> it's touched.)

I'm all for making about:memory more user friendly: it's not just a debug tool anymore.  That said, I also don't think it makes much sense to quibble over an extra 1MiB that goes away on GC, particularly when it's just one of many other caches.

Perhaps it would make sense to automatically run a full GC when we browse to about:memory?  Not having to worry about caches or page states would simplify the problem quite a bit.
Comment 11 Justin Lebar (not reading bugmail) 2012-04-02 14:49:20 PDT
> Perhaps it would make sense to automatically run a full GC when we browse to about:memory?  Not having 
> to worry about caches or page states would simplify the problem quite a bit.

We do this in about:compartments, but we've resisted doing it in about:memory, since you occasionally want to observe the state of the world as it exists, before you trigger that GC.

> That said, I also don't think it makes much sense to quibble over an extra 1MiB that goes away on 
> GC, particularly when it's just one of many other caches.

I guess it's at most 1MB, isn't it?  In that case, I don't care much at all.  :)
Comment 12 Jet Villegas (:jet) 2012-04-03 16:12:57 PDT
P2 per Memshrink
Comment 13 Justin Lebar (not reading bugmail) 2012-04-15 16:55:28 PDT
Created attachment 615207 [details] [diff] [review]
Patch v1
Comment 14 Nicholas Nethercote [:njn] 2012-04-15 17:40:50 PDT
Comment on attachment 615207 [details] [diff] [review]
Patch v1

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +1776,5 @@
> +        REPORT_BYTES(NS_LITERAL_CSTRING("js-main-runtime-gc-heap-unused"),
> +                     nsIMemoryReporter::KIND_OTHER,
> +                     rtStats.gcHeapUnused,
> +                     "Amount of the GC heap that's committed, but that is "
> +                     "neither part of an active allocation and nor being for "

"and nor being for" -> "nor being used for"

@@ +1777,5 @@
> +                     nsIMemoryReporter::KIND_OTHER,
> +                     rtStats.gcHeapUnused,
> +                     "Amount of the GC heap that's committed, but that is "
> +                     "neither part of an active allocation and nor being for "
> +                     "bookkeeping.  Equal to 'gc-heap-chunk-dirty-unused' + "

Elsewhere we use "book-keeping".  Looks like "bookkeeping" is preferred -- can you remove hypens in the existing cases?

@@ +1801,2 @@
>                 "Fraction of the main JSRuntime's garbage-collected JavaScript "
>                 "heap that is unused. Computed as "

Maybe say "fraction of the committed part of the main JSRuntime's heap"?
Comment 15 Justin Lebar (not reading bugmail) 2012-04-15 18:10:35 PDT
Mind if I rename gc-heap-unused --> gc-heap-committed-unused?
Comment 16 Justin Lebar (not reading bugmail) 2012-04-15 18:11:06 PDT
Er, nevermind, that doesn't work with the other -unused reporters we have.  :(
Comment 17 Justin Lebar (not reading bugmail) 2012-04-15 18:28:38 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/21f34fdd64a3
Comment 18 Justin Lebar (not reading bugmail) 2012-04-15 18:34:49 PDT
... and backed out (before any tests even started) because telemetry uses js-gc-heap, which I removed.
Comment 19 Justin Lebar (not reading bugmail) 2012-04-15 18:37:06 PDT
Bill, Terrence, would you mind if instead of reporting |js-gc-heap|, we reported js-gc-heap-committed in Telemetry?
Comment 20 Justin Lebar (not reading bugmail) 2012-04-15 18:46:17 PDT
Created attachment 615220 [details] [diff] [review]
Part 2: Send js-gc-heap-committed instead of js-gc-heap in telemetry.
Comment 21 [PTO to Dec5] Bill McCloskey (:billm) 2012-04-15 19:02:44 PDT
Comment on attachment 615220 [details] [diff] [review]
Part 2: Send js-gc-heap-committed instead of js-gc-heap in telemetry.

I'm not aware of anyone ever using this data, so it seems fine to change it.
Comment 23 Justin Lebar (not reading bugmail) 2012-04-24 18:34:18 PDT
Alas, this still broke js-gc-heap-committed telemetry, because that value is contained in a multi-reporter, which isn't supported in telemetry ping.

We should back this out of Aurora so we don't have a big gap in our telemetry.  The fix for nightly is in bug 748440.

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