The default bug view has changed. See this FAQ.

Rejigger JS memory reporters to mirror jemalloc's

RESOLVED FIXED in mozilla14

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: glandium, Assigned: Justin Lebar (not reading bugmail))

Tracking

Trunk
mozilla14
All
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
(Reporter)

Comment 2

5 years ago
It's 673M on the latter case, and 10M on the former case, which makes a significant amount of it not committed.
(Assignee)

Comment 3

5 years ago
(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?
(Reporter)

Comment 4

5 years ago
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.
(Reporter)

Comment 5

5 years ago
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]
(Assignee)

Comment 6

5 years ago
> 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"...
(Assignee)

Comment 7

5 years ago
Nick, what do you think about the changes above?
(Assignee)

Updated

5 years ago
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.
(Assignee)

Comment 9

5 years ago
> 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.
(Assignee)

Comment 11

5 years ago
> 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

5 years ago
P2 per Memshrink
Assignee: nobody → justin.lebar+bug
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 13

5 years ago
Created attachment 615207 [details] [diff] [review]
Patch v1
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 15

5 years ago
Mind if I rename gc-heap-unused --> gc-heap-committed-unused?
(Assignee)

Comment 16

5 years ago
Er, nevermind, that doesn't work with the other -unused reporters we have.  :(
(Assignee)

Comment 17

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/21f34fdd64a3
Component: jemalloc → JavaScript Engine
QA Contact: jemalloc → general
(Assignee)

Comment 18

5 years ago
... and backed out (before any tests even started) because telemetry uses js-gc-heap, which I removed.
(Assignee)

Comment 19

5 years ago
Bill, Terrence, would you mind if instead of reporting |js-gc-heap|, we reported js-gc-heap-committed in Telemetry?
(Assignee)

Comment 20

5 years ago
Created attachment 615220 [details] [diff] [review]
Part 2: Send js-gc-heap-committed instead of js-gc-heap in telemetry.
Attachment #615220 - Flags: review?(wmccloskey)
(Assignee)

Updated

5 years ago
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+
https://hg.mozilla.org/mozilla-central/rev/48315737bd3f
https://hg.mozilla.org/mozilla-central/rev/29c2bb6184b6
https://hg.mozilla.org/mozilla-central/rev/1c46346f8e68
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
(Assignee)

Comment 23

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 748627
(Assignee)

Updated

5 years ago
status-firefox14: --- → affected
status-firefox15: --- → affected
tracking-firefox14: --- → ?
tracking-firefox15: --- → ?
Keywords: regression
(Assignee)

Updated

5 years ago
status-firefox14: affected → ---
status-firefox15: affected → ---
tracking-firefox14: ? → ---
tracking-firefox15: ? → ---
Keywords: regression
(Assignee)

Updated

5 years ago
Depends on: 790621
You need to log in before you can comment on or make changes to this bug.