Closed
Bug 674480
Opened 13 years ago
Closed 13 years ago
Add memory reporter for number of empty GC chunks
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: igor, Assigned: igor)
References
Details
(Whiteboard: [inbound])
Attachments
(1 file, 2 obsolete files)
17.82 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
+++ 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•13 years ago
|
||
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 2•13 years ago
|
||
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•13 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?
Comment 4•13 years ago
|
||
(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•13 years ago
|
||
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.
Updated•13 years ago
|
Attachment #548730 -
Flags: review-
Assignee | ||
Comment 6•13 years ago
|
||
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 7•13 years ago
|
||
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+
Comment 8•13 years ago
|
||
> 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•13 years ago
|
||
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)
Updated•13 years ago
|
Attachment #550086 -
Flags: review?(nnethercote) → review+
Assignee | ||
Comment 10•13 years ago
|
||
Whiteboard: [inbound]
Comment 11•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in
before you can comment on or make changes to this bug.
Description
•