669611 - Add memory reporter for GC heap fragmentation

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Justin Lebar (not reading bugmail)]

If we're going to try and improve GC heap fragmentation, we should measure it first by adding appropriate figures to about:memory and telemetry.

Comment 1

[Nicholas Nethercote [inactive]]

khuey looked at this.  For telemetry it's not a good idea.  That's because to compute the amount of unused heap space requires iterating over every used object in the GC heap, which is expensive and not something we should do every 60 seconds.

For about:memory it's a fine idea.  We probably  want to add UNITS_PERCENTAGE to nsIMemoryReporter as part of this.

Comment 2

[Justin Lebar (not reading bugmail)]

(In reply to comment #1)
> khuey looked at this.  For telemetry it's not a good idea.  That's because
> to compute the amount of unused heap space requires iterating over every
> used object in the GC heap, which is expensive and not something we should
> do every 60 seconds.

Okay, that makes sense.

> For about:memory it's a fine idea.  We probably  want to add
> UNITS_PERCENTAGE to nsIMemoryReporter as part of this.

We already report heap-dirty, which is [number of committed pages which don't participate in an allocation] times [page size].  We could report gc-heap-dirty similarly.  (That is, I don't think we should report this as a percentage without revisiting other measurements.)

Comment 3

[Nicholas Nethercote [inactive]]

about:memory already reports "gc-heap-chunk-unused" and "js-gc-heap".  I was expecting this fragmentation number to just be the former divided by the latter.  Showing this number saves you from having to find the two numbers and do the calculation yourself.  Are you proposing something different?

Comment 4

[Justin Lebar (not reading bugmail)]

Ah, I didn't notice that because js-gc-heap is in "other measurements", while js-gc-heap-unused is stuck between a bunch of compartments.  (I understand that js-gc-heap can't be under "js" because it's the sum of all the compartments' gc-heaps.)

Maybe the compartments should all be grouped together.  It's easy to miss something stuck in-between.

In any case, I'm fine showing the result of that division, since it's clearly easy to miss the data otherwise.  Maybe we also want to compare heap-used to heap-dirty, but we can decide about that separately.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch:
- Add support for percentage amounts in nsIMemoryReporter.  There's a hack
  to allow fractional percentages -- all percentage amounts are multiplied
  by 100.  I couldn't think of a better way to do this.

- Adds a js-gc-heap-unused percentage to the "other measurements" list in
  about:memory.

- Adds some missing _units fields to various objects in aboutMemory.js.
  This should have gone into an earlier patch, probably in bug 668893, and
  was causing some cmpAmount() calls to be comparing undefined properties.

Comment 6

[Justin Lebar (not reading bugmail)]

> about:memory already reports "gc-heap-chunk-unused" and "js-gc-heap".  I was 
> expecting this fragmentation number to just be the former divided by the latter.

The patch does something more complicated than this -- it sums all the arena unused values and adds them in.

Can we also report that js-gc-arena-unused total?  I'm fine doing division and reporting a percentage, but I think we should make it easy to get all the raw numbers we used to calculate the percentage.  (Otherwise you'd have to add up all the compartments' arena unused, or do some algebra...)

Comment 7

[Justin Lebar (not reading bugmail)]

>diff --git a/js/src/xpconnect/src/xpcjsruntime.cpp b/js/src/xpconnect/src/xpcjsruntime.cpp
>--- a/js/src/xpconnect/src/xpcjsruntime.cpp
>+++ b/js/src/xpconnect/src/xpcjsruntime.cpp
>@@ -1538,37 +1538,44 @@ public:
>                                                CellCallback);
>             JS_EndRequest(cx);
>             JS_DestroyContextNoGC(cx);
>         }
> 
>         NS_NAMED_LITERAL_CSTRING(p, "");
> 
>         PRInt64 gcHeapChunkTotal = gXPCJSChunkAllocator.GetGCChunkBytesInUse();
>-        // This is initialized to gcHeapChunkUnused, and then we subtract used
>+        // This is initialized to gcHeapChunkTotal, and then we subtract used
>         // space from it each time around the loop.
>         PRInt64 gcHeapChunkUnused = gcHeapChunkTotal;
>+        PRInt64 gcHeapArenaUnused = 0;
> 
>         #define DO(path, kind, amount, desc) \
>             callback->Callback(p, path, kind, nsIMemoryReporter::UNITS_BYTES, \
>                                amount, NS_LITERAL_CSTRING(desc), closure);
> 
>+        #define DO_PERC(path, kind, amount, desc) \
>+            callback->Callback(p, path, kind, nsIMemoryReporter::UNITS_PERCENTAGE, \
>+                               amount, NS_LITERAL_CSTRING(desc), closure);

Nit: I'd prefer DO_PERCENT.

>+        DO_PERC(NS_LITERAL_CSTRING("js-gc-heap-unused"),
>+           nsIMemoryReporter::KIND_OTHER, gcHeapUnusedPercentage,
>+    "Percentage of the garbage-collected JavaScript heap that is unused. "
>+    "Computed from 'js-gc-heap', 'explicit/js/gc-heap-chunk-unused', and "
>+    "each compartment's 'gc-heap/arena-unused'.");

If we reported js-gc-arena-unused (or js-gc-total-arena-unused), then we could say:

 Percentage of the garbage-collected JavaScript heap that is unused.  Computed
 as 'explicit/js/gc-heap-chunk-unused' plus 'js-gc-total-arena-unused' all over
 'js-gc-heap'.

As it is, it's not immediately clear that the first variable is the denominator
and the second two are summed to make the numerator.

I'd also prefer "mapped but not part of an active allocation" (or something
like that) rather than "unused".

>+        // Multiply by 10000x:  100x because it's a percentage, and another
>+        // 100x because nsIMemoryReporter requires that for percentage amounts.
>+        PRInt64 gcHeapUnusedPercentage =
>+            (gcHeapChunkUnused + gcHeapArenaUnused) * 10000 /
>+            gXPCJSChunkAllocator.GetGCChunkBytesInUse();

Nit: "Multiply by 10000".  (You're not multiplying by "10000 times".) Also one
space after colon.

>+  // The secondary sort is on the _amount property if the _units is
>+  // UNIT_BYTES...
>+  if (a._units == UNITS_BYTES) {
>+    return b._amount - a._amount;
>   }

Maybe indicate that this is descending, while the other two sorts are
ascending.

>@@ -303,44 +311,48 @@ function genProcessText(aProcess, aRepor
>         if (aKids[i]._name === aName) {
>           return aKids[i];
>         }
>       }
>       return undefined;
>     }
> 
>     // We want to process all reporters that begin with 'treeName'.
>-    // First we build the tree but only filling in '_name', '_kind', '_kids'
>-    // and maybe '._hasReporter'.  This is done top-down from the reporters.
>+    // First we build the tree but only filling in '_name', '_kind', '_units',
>+    // '_kids' and maybe '._hasReporter'.  This is done top-down from the
>+    // reporters.

s/filling/fill.  Also comma before "and".  (At least, that's the common style
in America, journalism excepted.)

>+   *  - PERCENTAGE: The amount contains a fraction that should be expressed as
>+   *    a percentage.  NOTE!  The |amount| field should be given a value 100x
>+   *    the actual percentage;  this number will be divided by 100 when shown.
>+   *    This allows a fractional percentage to be shown even though |amount| is
>+   *    an integer.  Eg. if the actual percentage is 12.34%, |amount| should be
>+   *    1234.

One space after semicolon, period after "E" in "Eg.".  I'd also s/NOTE!  /Note: /,
but that's up to you.

r=me, but let's agree on whether we want to add a js-gc-arena-unused reporter.

Comment 8

[Nicholas Nethercote [inactive]]

So you want all of these in the "other" list:

- js-gc-heap              - BYTES
- js-gc-heap-chunk-unused - BYTES (new)
- js-gc-heap-arena-unused - BYTES (new)
- js-gc-heap-unused       - PERCENTAGE (new)

Is that right?  It's a bit weird that the 4th one is a percentage when it could easily be bytes.  Maybe I should add "-fraction" to its name?

Comment 9

[Justin Lebar (not reading bugmail)]

(In reply to comment #8)
> So you want all of these in the "other" list:
> 
> - js-gc-heap              - BYTES
> - js-gc-heap-chunk-unused - BYTES (new)
> - js-gc-heap-arena-unused - BYTES (new)
> - js-gc-heap-unused       - PERCENTAGE (new)

So you're proposing to report js-gc-heap-chunk-unused both in "other" and "explicit"?  I don't like the duplication, but I like more having all these numbers in the same place.

And yes, if we're reporting a computed fraction, I think we should also report the numbers we used to compute the fraction.

Since we landed bug 667085, it's now "heap-allocated" and "heap-unallocated", so maybe we should s/unused/unallocated here and in the description.

> Is that right?  It's a bit weird that the 4th one is a percentage when it
> could easily be bytes.  Maybe I should add "-fraction" to its name?

Sure, adding "-fraction" sounds good to me.

Comment 10

[Nicholas Nethercote [inactive]]

The dev-doc-needed will bee needed for the new nsIMemoryReporter::UNITS_PERCENTAGE constant, once this lands.

Comment 11

[Nicholas Nethercote [inactive]]

Attachment: patch, v2

New things in this patch:

- Renames js-gc-heap-unused as js-gc-heap-unused-fraction.

- Computes js-gc-heap-arena-unused, which is the sum of all the
  per-compartment gc-heap-arena-unused values.

- Duplicates explicit/js/gc-heap-chunk-unused as js-gc-heap-chunk-unused in
  the 'other measurements' list, to make the js-gc-heap-unused-fraction
  computation more obvious.

- Adds some stronger checking of certain invariants in aboutMemory.js,
  throwing exceptions if they are violated.

- Removes the addition of _units from tree nodes in aboutMemory.js;  they
  aren't needed because all tree nodes have UNITS_BYTES.  (This was enabled
  by splitting cmpAmount into two.) 

- Tweaks compartment sub-reporters' descriptions to indicate that they
  describe a single compartment.

- Changes the sort order of the "other measurements" list.  It's now
  entirely alphabetical by path.  This keeps all the js numbers together,
  all the heap numbers together, etc.  It also gives a predictable
  ordering.  And it's simpler.  An example:

     40,552 B -- gfx-surface-image
 37,232,122 B -- heap-allocated
 39,845,888 B -- heap-committed
    237,568 B -- heap-dirty
  2,612,100 B -- heap-unallocated
  8,388,608 B -- js-gc-heap
    169,432 B -- js-gc-heap-arena-unused
  1,309,184 B -- js-gc-heap-chunk-unused
       17.62% -- js-gc-heap-unused-fraction
            2 -- page-faults-hard
       20,725 -- page-faults-soft
 79,097,856 B -- resident
537,837,568 B -- vsize

  I think this is an improvement.

- I left the "unused" names in place, changing them doesn't feel compelling.

Comment 12

[Mozilla RelEng Bot]

Try run for 42218019f1de is complete.
Detailed breakdown of the results available here:
    http://tbpl.mozilla.org/?tree=Try&rev=42218019f1de
Results:
    success: 145
    warnings: 16
    failure: 2
Total buildrequests: 163

Comment 13

[Justin Lebar (not reading bugmail)]

Comment on attachment 546986 [details] [diff] [review]
patch, v2

Please forgive me for nit'ing on the memory reporter descriptions which you didn't substantively change.  If you don't want to mess with them in this bug, I'll file another one.  But maybe we can just handle it here, since we're already touching the code.

I've split out the nits you need to address below these, which you don't.  It would make me happy if you did, though.  :)

===== Comments on code only incidentally touched =====

>-            DO(mkPath(name, "gc-heap/arena-padding"),
>+            BYTES(mkPath(name, "gc-heap/arena-padding"),
>                JS_GC_HEAP_KIND, stats->gcHeapArenaPadding,
>-    "Memory on the garbage-collected JavaScript heap, within arenas, that is "
>-    "unused and present only so that other data is aligned.");
>+    "Memory on the compartment's garbage-collected JavaScript heap, within "
>+    "arenas, that is unused and present only so that other data is aligned.");

Maybe say that this is "internal fragmentation" in the description, since that's immediately meaningful to many of us.

> 
>-            DO(mkPath(name, "gc-heap/arena-unused"),
>+            BYTES(mkPath(name, "gc-heap/arena-unused"),
>                JS_GC_HEAP_KIND, stats->gcHeapArenaUnused,
>-    "Memory on the garbage-collected JavaScript heap, within arenas, that "
>-    "could be holding useful data but currently isn't.");
>+    "Memory on the compartment's garbage-collected JavaScript heap, within "
>+    "arenas, that could be holding useful data but currently isn't.");

Call this "external fragmentation".

>-            DO(mkPath(name, "object-slots"),
>+            BYTES(mkPath(name, "gc-heap/strings"),
>+               JS_GC_HEAP_KIND, stats->gcHeapStrings,
>+    "Memory on the compartment's garbage-collected JavaScript heap that holds "
>+    "string headers.");

Would it be worthwhile to explain what these are, or is it too complicated?  I could guess what "gc-heap/strings" is, but not what "string headers" are, or how to know when they're too big.

>+            BYTES(mkPath(name, "object-slots"),
>                nsIMemoryReporter::KIND_HEAP, stats->objectSlots,
>-    "Memory allocated for non-fixed object slot arrays, which are used "
>-    "to represent object properties.  Some objects also contain a fixed "
>-    "number of slots which are stored on the JavaScript heap;  those slots "
>-    "are not counted here, but in 'gc-heap/objects'.");
>+    "Memory allocated for the compartment's non-fixed object slot arrays, "
>+    "which are used to represent object properties.  Some objects also "
>+    "contain a fixed number of slots which are stored on the compartment's "
>+    "JavaScript heap; those slots are not counted here, but in "
>+    "'gc-heap/objects'.");

How about: "those slots are not counted here, but are in 'gc-heap/objects' {instead}."

> 
>-            DO(mkPath(name, "string-chars"),
>+            BYTES(mkPath(name, "string-chars"),
>                nsIMemoryReporter::KIND_HEAP, stats->stringChars,
>-    "Memory allocated to hold string characters.  Not all of this allocated "
>-    "memory is necessarily used to hold characters.  Each string also "
>-    "includes a header which is stored on the JavaScript heap;  that header "
>-    "is not counted here, but in 'gc-heap/strings'.");
>+    "Memory allocated to hold the compartment's string characters.  Not all "
>+    "of this allocated memory is necessarily used to hold characters.  Each "
>+    "string also includes a header which is stored on the compartment's "
>+    "JavaScript heap;  that header is not counted here, but in "
>+    "'gc-heap/strings'.");

Okay, this is obviously different from gc-heap/strings.  Maybe that one should be string-headers, since it apparently doesn't store the strings themselves.

Also, the second sentence contradicts the first.  I'd say "Memory allocated to hold the compartment's string characters and other stuff," but maybe you can be more precise.

>-            DO(mkPath(name, "tjit-data/allocators-main"),
>+            BYTES(mkPath(name, "tjit-data/allocators-main"),
>                nsIMemoryReporter::KIND_HEAP, stats->tjitDataAllocatorsMain,
>-    "Memory used by the trace JIT's VMAllocators.");
>+    "Memory used by the trace JIT in the compartment's VMAllocators.");

Can you elaborate on what this is?



===== Comments on code actually touched =====

>-            DO(mkPath(name, "mjit-data"),
>+            BYTES(mkPath(name, "mjit-data"),
>                nsIMemoryReporter::KIND_HEAP, stats->mjitData,
>-    "Memory used by the method JIT for the following data: "
>+    "Memory used by the method JIT for the compartment's following data: "
>     "JITScripts, native maps, and inline cache structs.");

How about "Memory used by the method JIT for the compartment's JITScripts,
native maps, and inline cache structs."

>-        DO(NS_LITERAL_CSTRING("explicit/js/gc-heap-chunk-admin"),
>+        BYTES(NS_LITERAL_CSTRING("js-gc-heap-chunk-unused"),
>+           nsIMemoryReporter::KIND_OTHER, gcHeapChunkUnused,
>+    "The same as 'explicit/js/gc-heap-chunk-unused'.  Shown here for "
>+    "easy comparison with 'js-gc-heap' and 'js-gc-heap-arena-unused'.");

I'd copy the description from explicit/js/gc-heap-chunk-unused in here.  ("The same as 'explicit/js/gc-heap-chunk-unused': Memory on the garbage-colleged JavaScript heap, within chunks, that could be holding useful data but currently isn't.")  This is all about convenience, after all.  :)

>+        BYTES(NS_LITERAL_CSTRING("js-gc-heap-arena-unused"),
>+           nsIMemoryReporter::KIND_OTHER, gcHeapArenaUnused,
>+    "Memory on the garbage-collected JavaScript heap, within arenas, that "
>+    "could be holding useful data but currently isn't.");

Please say that this is the sum of the compartments' arena-unused figures.

>+
>+        PERCENTAGE(NS_LITERAL_CSTRING("js-gc-heap-unused-fraction"),
>+           nsIMemoryReporter::KIND_OTHER, gcHeapUnusedPercentage,
>+    "Fraction of the garbage-collected JavaScript heap that is unused. "
>+    "Computed by adding 'explicit/js/gc-heap-chunk-unused' to "
>+    "'js-gc-heap-arena-unused', and dividing by 'js-gc-heap'");

Since we're duplicating explicit/js/gc-heap-chunk-unused as js-gc-heap-chunk-unused, I think we should reference the latter here.

>+// Compare two memory reporter nodes from the 'other measurements' list.
>+function cmpOtherReporters(a, b)
>+{
>+  return a._path < b._path ? -1 :
>+         a._path > b._path ?  1 :
>+         0;
>+};

I think this new ordering is much better.

>+function assert(aCond, aMsg)
>+{
>+  if (!aCond) {
>+    throw("assertion failed: " + aMsg);
>+  }
>+}

Is this the cleanest way we have of doing an assertion in JS code?  That's kind of sad if it is...

>diff --git a/toolkit/components/aboutmemory/tests/chrome/test_aboutmemory.xul b/toolkit/components/aboutmemory/tests/chrome/test_aboutmemory.xul
>--- a/toolkit/components/aboutmemory/tests/chrome/test_aboutmemory.xul
>+++ b/toolkit/components/aboutmemory/tests/chrome/test_aboutmemory.xul
>           f("explicit/d",       NONHEAP, BYTES, 499 * KB); // omitted
>           f("explicit/e",       NONHEAP, BYTES, 100 * KB); // omitted
>           f("explicit/f/g/h/i", HEAP,    BYTES,  20 * MB);
>        }
>      },
>      { collectReports: function(cbObj, closure) {
>           function f(p, k, u, a) { cbObj.callback("", p, k, u, a, "(desc)", closure); }
>           f("explicit/g",       HEAP,    BYTES,  14 * MB); // internal
>+          f("other3",           OTHER,   COUNT, 777);
>           f("other2",           OTHER,   BYTES, 222 * MB);
>-          f("other3",           OTHER,   COUNT, 777);
>+          f("perc2",            OTHER,  PERCENTAGE, 10000);
>+          f("perc1",            OTHER,  PERCENTAGE, 4567);

I don't really care about the indentation in this test, but you might.

>diff --git a/xpcom/base/nsIMemoryReporter.idl b/xpcom/base/nsIMemoryReporter.idl
>--- a/xpcom/base/nsIMemoryReporter.idl
>+++ b/xpcom/base/nsIMemoryReporter.idl
>@@ -45,17 +45,17 @@ interface nsISimpleEnumerator;
>  * Use this when it makes sense to gather this measurement without gathering
>  * related measurements at the same time.
>  *
>  * Note that the |amount| field may be implemented as a function, and so
>  * accessing it can trigger significant computation;  the other fields can
>  * be accessed without triggering this computation.  (Compare and contrast
>  * this with nsIMemoryMultiReporter.)  
>  */
>-[scriptable, uuid(b2c39f65-1799-4b92-a806-ab3cf6af3cfa)]
>+[scriptable, uuid(275edfc0-23a0-47cf-a896-df8f0412de48)]
> interface nsIMemoryReporter : nsISupports

You didn't modify the vtable, so I don't think you need to modify the uuid.

>+   *  - PERCENTAGE: The amount contains a fraction that should be expressed as
>+   *    a percentage.  NOTE!  The |amount| field should be given a value 100x
>+   *    the actual percentage;  this number will be divided by 100 when shown.
>+   *    This allows a fractional percentage to be shown even though |amount| is
>+   *    an integer.  Eg. if the actual percentage is 12.34%, |amount| should be
>+   *    1234.

s/Eg./E.g.

Comment 14

[Nicholas Nethercote [inactive]]

(In reply to comment #13)
> 
> Please forgive me for nit'ing on the memory reporter descriptions which you
> didn't substantively change. 

Yes:  major scope creep.  I'll make the changes.


> Is this the cleanest way we have of doing an assertion in JS code?  That's
> kind of sad if it is...

JS doesn't have assertions as such.  This achieves a similar effect -- the page won't draw (making the problem obvious), but it won't take down the browser.


> s/Eg./E.g.

"Eg." is a valid informal form (eg. http://en.wiktionary.org/wiki/e.g.) but I'm going to give up and stop using it because I'm sick of people telling me it's wrong.

Comment 15

[Justin Lebar (not reading bugmail)]

Wiktionary may just be saying that "eg." is *used* in informal writing (which is certainly true), not that it's an accepted form.  "eg." isn't in Merriam-Webster or American Heritage, but it is in Collins ("e.g.", "eg.", and "eg").

But I think keeping "Eg." in exchange for fixing the JS memory reporter descriptions is more than a fair trade.  :)

Comment 16

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/cd0b09b76016

Comment 17

[Igor Bukanov]

The patch uses  gcHeapChunkUnused to calculate the heap fragmentation. However, those chunks can be readily released and do not reflect the real fragmentation problem. It would be nice to report the fragmentation and percentage based on the heap size without pooled chunks.

Comment 18

[Nicholas Nethercote [inactive]]

(In reply to comment #17)
> The patch uses  gcHeapChunkUnused to calculate the heap fragmentation.
> However, those chunks can be readily released and do not reflect the real
> fragmentation problem. It would be nice to report the fragmentation and
> percentage based on the heap size without pooled chunks.

What is the "real fragmentation problem"?  AFAICT all the unused memory can be reused easily -- i.e. there's no external fragmentation.  And there's minimal internal fragmentation, mostly due to arena padding.

Comment 19

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/cd0b09b76016

Comment 20

[Igor Bukanov]

(In reply to comment #18)
> What is the "real fragmentation problem"?

I define this as the amount of unused memory in GC chunks after the GC. This memory can only be used to allocate GC things and cannot be returned to the system.

>  AFAICT all the unused memory can
> be reused easily -- i.e. there's no external fragmentation.  And there's
> minimal internal fragmentation, mostly due to arena padding.

The chunk pooling is just an optimization to workaround slow mmap calls. We can release it even without running the GC and it is released when heuristics tells about it. The fact that we do not release it on low memory should be considered as a bug. That is the reason I prefer to exclude it from the fragmentation stats.

Comment 21

[Eric Shepherd [:sheppy]]

Already documented:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIMemoryReporter#Unit_type_constants

Also already mentioned on Firefox 8 for developers.