674480 - Add memory reporter for number of empty GC chunks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

+++ This bug was initially created as a clone of Bug #669611 +++

Currently from about:memory it is impossible to tell if unused chunk memory comes from an empty chunks that can be released back to the system or from chunks that has at least one allocated GC thing and can only be used for other GC allocations.

I suggest to add a reporter that explicitly shows the memory stored in empty chunks.

Comment 1

[Igor Bukanov]

Attachment: v1

With this patch I can press the GC button in about:memory few times until the empty chunk count goes to zero to get numbers for the GC heap that is taken by live GC things and that cannot be returned to the system.

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 548730 [details] [diff] [review]
v1

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

Stealing the review.  r=me with the issue below addressed.

::: js/src/xpconnect/src/xpcjsruntime.cpp
@@ +1770,5 @@
>  
> +        BYTES(NS_LITERAL_CSTRING("explicit/js/gc-heap-chunk-empty"),
> +           JS_GC_HEAP_KIND, gcHeapChunkEmpty,
> +    "Memory on the garbage-collected JavaScript heap used by empty chunks, "
> +    "that soon will be released unless claimed for new allocations.");

AFAICT this overlaps with explicit/js/gc-heap.  That's bad -- if any explicit/* reporters overlap (i.e. we double-count some bytes) we end up with nonsense results in the "explicit allocations" tree.  So I suggest removing this and just reporting js-gc-heap-chunk-empty.

Comment 3

[Igor Bukanov]

(In reply to comment #2)
> Stealing the review.  r=me with the issue below addressed.


Just to be sure, I do not need to update the about:memory test as it tests a generic reporting functionality, not the exact counters that displayed, right?

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to comment #3)
> 
> Just to be sure, I do not need to update the about:memory test as it tests a
> generic reporting functionality, not the exact counters that displayed,
> right?

That's right.

Comment 5

[Justin Lebar (not reading bugmail)]

I think the more non-overlapping reporters we have, the better.  js-gc-heap-chunk-empty overlaps with js-gc-heap-chunk-unused, and I'd prefer that it didn't.

So can you modify js-gc-heap-chunk-unused to be js-gc-heap-chunk-dirty-unused and count the amount of unused memory in non-empty chunks?  You'll need to modify this reporter's description and also the description of js-gc-heap-unused-fraction to indicate that the numerator is the sum of three things now.

We could also change the name to js-gc-heap-chunk-clean-unused, for parallelism with -dirty and because it counts towards unused-fraction, but I don't care much either way.

Comment 6

[Igor Bukanov]

Attachment: v2

The new version addresses the comments and also removes redundant XPConnectGCChunkAllocator::mNumGCChunksInUse. That number instead is exposed from the JS engine directly.

Comment 7

[Nicholas Nethercote [inactive]]

Comment on attachment 549372 [details] [diff] [review]
v2

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

This patch is going to clash with the one in 674721.  Can you let it land first?  I think fixing the conflicts on yours will be easier, because your patch is a lot smaller.

::: js/src/xpconnect/src/xpcjsruntime.cpp
@@ +1764,5 @@
> +        BYTES(NS_LITERAL_CSTRING("explicit/js/gc-heap-chunk-dirty-unused"),
> +           JS_GC_HEAP_KIND, gcHeapChunkDirtyUnused,
> +    "Memory on the garbage-collected JavaScript heap, within chunks with at "
> +    "least one allocated GC thing, that could be holding useful data but "
> +    "currently isn't.");

jlebar, is that the same meaning of "dirty" that's used by the jemalloc reporters?  I don't particularly like this use of "clean" and "dirty", but I won't r- because of it.

@@ +1769,5 @@
>  
> +        BYTES(NS_LITERAL_CSTRING("explicit/js/gc-heap-chunk-clean-unused"),
> +           JS_GC_HEAP_KIND, gcHeapChunkCleanUnused,
> +    "Memory on the garbage-collected JavaScript heap taken by cached unused "
> +    "chunks, that soon will be released unless claimed for new allocations.");

"cached"?  Sounds strange.  Maybe "...taken by completely empty chunks, that soon will..."

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

And also 'js-gc-heap-chunk-clean-unused'.  I suggest future-proofing this description against further changes by changing it to "Shown here for easy comparison with other 'js-gc' reporters."

@@ +1784,5 @@
> +
> +        BYTES(NS_LITERAL_CSTRING("js-gc-heap-chunk-clean-unused"),
> +           nsIMemoryReporter::KIND_OTHER, gcHeapChunkCleanUnused,
> +    "The same as 'explicit/js/gc-heap-chunk-clean-unused'.  Shown here for "
> +    "easy comparison with 'js-gc-heap'.");

Same here.

Comment 8

[Nicholas Nethercote [inactive]]

> This patch is going to clash with the one in 674721.  Can you let it land
> first?

That patch just landed on mozilla-inbound.

Comment 9

[Igor Bukanov]

Attachment: v3

Here is a rebase to account for changes from the bug 674721. Ideally js-heap totals at the end of about:memory should include stats for all JS heaps, but that is for another bug.

Comment 10

[Igor Bukanov]

http://hg.mozilla.org/integration/mozilla-inbound/rev/50a3378c5940

Comment 11

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/50a3378c5940