Last Comment Bug 674480 - Add memory reporter for number of empty GC chunks
: Add memory reporter for number of empty GC chunks
Status: RESOLVED FIXED
[inbound]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Igor Bukanov
:
Mentors:
Depends on: 669611
Blocks:
  Show dependency treegraph
 
Reported: 2011-07-27 02:11 PDT by Igor Bukanov
Modified: 2011-08-04 02:51 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (3.06 KB, patch)
2011-07-27 02:53 PDT, Igor Bukanov
n.nethercote: review+
justin.lebar+bug: review-
Details | Diff | Review
v2 (11.16 KB, patch)
2011-07-29 07:42 PDT, Igor Bukanov
n.nethercote: review+
Details | Diff | Review
v3 (17.82 KB, patch)
2011-08-02 08:23 PDT, Igor Bukanov
n.nethercote: review+
Details | Diff | Review

Description Igor Bukanov 2011-07-27 02:11:51 PDT
+++ 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 2011-07-27 02:53:41 PDT
Created attachment 548730 [details] [diff] [review]
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 [:njn] (on vacation until July 11) 2011-07-27 03:00:31 PDT
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 2011-07-27 03:41:18 PDT
(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 [:njn] (on vacation until July 11) 2011-07-27 04:33:00 PDT
(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) 2011-07-27 07:49:57 PDT
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 2011-07-29 07:42:58 PDT
Created attachment 549372 [details] [diff] [review]
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 [:njn] (on vacation until July 11) 2011-08-01 18:20:18 PDT
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 [:njn] (on vacation until July 11) 2011-08-01 21:14:29 PDT
> 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 2011-08-02 08:23:54 PDT
Created attachment 550086 [details] [diff] [review]
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 11 Marco Bonardo [::mak] 2011-08-04 02:51:11 PDT
http://hg.mozilla.org/mozilla-central/rev/50a3378c5940

Note You need to log in before you can comment on or make changes to this bug.