The default bug view has changed. See this FAQ.

Add memory reporter for number of empty GC chunks

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Igor Bukanov, Assigned: Igor Bukanov)

Tracking

unspecified
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 2 obsolete attachments)

v3
17.82 KB, patch
njn
: review+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

6 years ago
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.
Attachment #548730 - Flags: review?(justin.lebar+bug)
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.
Attachment #548730 - Flags: review?(justin.lebar+bug) → review+
(Assignee)

Comment 3

6 years ago
(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?
(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.
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.
Attachment #548730 - Flags: review-
(Assignee)

Comment 6

6 years ago
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.
Attachment #548730 - Attachment is obsolete: true
Attachment #549372 - Flags: review?(nnethercote)
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.
Attachment #549372 - Flags: review?(nnethercote) → review+
> This patch is going to clash with the one in 674721.  Can you let it land
> first?

That patch just landed on mozilla-inbound.
(Assignee)

Comment 9

6 years ago
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.
Attachment #549372 - Attachment is obsolete: true
Attachment #550086 - Flags: review?(nnethercote)
Attachment #550086 - Flags: review?(nnethercote) → review+
(Assignee)

Comment 10

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/50a3378c5940
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/50a3378c5940
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.