Closed Bug 741378 Opened 12 years ago Closed 12 years ago

Rejigger JS memory reporters to mirror jemalloc's

Categories

(Core :: JavaScript Engine, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: glandium, Assigned: justin.lebar+bug)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 files)

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.
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.
It's 673M on the latter case, and 10M on the former case, which makes a significant amount of it not committed.
(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?
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.
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]
> 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"...
Nick, what do you think about the changes above?
Summary: Unexpected big difference between heap-committed and anonymous rss → Rejigger JS memory reporters to mirror jemalloc's
(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.
> 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.)
(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.
> 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.  :)
P2 per Memshrink
Assignee: nobody → justin.lebar+bug
Whiteboard: [MemShrink] → [MemShrink:P2]
Attached patch Patch v1Splinter Review
Attachment #615207 - Flags: review?(n.nethercote)
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"?
Attachment #615207 - Flags: review?(n.nethercote) → review+
Mind if I rename gc-heap-unused --> gc-heap-committed-unused?
Er, nevermind, that doesn't work with the other -unused reporters we have.  :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/21f34fdd64a3
Component: jemalloc → JavaScript Engine
QA Contact: jemalloc → general
... and backed out (before any tests even started) because telemetry uses js-gc-heap, which I removed.
Bill, Terrence, would you mind if instead of reporting |js-gc-heap|, we reported js-gc-heap-committed in Telemetry?
Attachment #615220 - Attachment description: Part 2: Send js-gc-heap-committed instead of js-gc-heap. → Part 2: Send js-gc-heap-committed instead of js-gc-heap in telemetry.
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.
Attachment #615220 - Flags: review?(wmccloskey) → review+
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.
Blocks: 748627
Depends on: 790621
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: