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.

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.

(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.)

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?

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.

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.

> 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...)

>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.

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?

(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.

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

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.

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 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.

(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.

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. :)

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.

(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.

(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.

Already documented: https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIMemoryReporter#Unit_type_constants Also already mentioned on Firefox 8 for developers.