Closed
Bug 1134255
Opened 10 years ago
Closed 10 years ago
Add breakdown of allocated and unused GC things by kind in memory reports
Categories
(Core :: JavaScript: GC, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
References
(Blocks 1 open bug)
Details
(Whiteboard: [MemShrink])
Attachments
(1 file, 1 obsolete file)
20.43 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
To make is easier to see the effect compacting GC, we can add a breakdown of the allocated and unused GC things by kind in the memory reports.
Attachment #8566060 -
Flags: review?(n.nethercote)
Updated•10 years ago
|
Whiteboard: [MemShrink]
Comment 1•10 years ago
|
||
Comment on attachment 8566060 [details] [diff] [review]
unused-gc-thing-breakdown
Review of attachment 8566060 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good. There are a few nits and one substantial comment about adding an assertion. Thanks.
::: js/public/MemoryMetrics.h
@@ +434,5 @@
>
> #undef FOR_EACH_SIZE
> };
>
> +struct GCThingStats
Nit: Because this class is just a bunch of size_t fields, please call it GCThingSizes to match GCSizes, TabSizes, CodeSizes, etc.
@@ +457,5 @@
> + : FOR_EACH_SIZE(COPY_OTHER_SIZE)
> + dummy()
> + {}
> +
> + size_t &forKind(JSGCTraceKind kind) {
Returning a |size_t&| is weird. How about changing the signature to this:
> void addToKind(JSGCTraceKind kind, size_t n)
and then making it add |n| to the appropriate kind?
::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2927,5 @@
> "The same as 'explicit/js-non-window/gc-heap/unused-arenas'.");
>
> + REPORT_BYTES(NS_LITERAL_CSTRING("js-main-runtime-gc-heap-committed/unused/gc-things/objects"),
> + KIND_OTHER, rtStats.zTotals.unusedGCThings.object,
> + "Empty object cells within non-empty arenas.");
Use "Unused" instead of "Empty" as the first word of the description, to match the path. Ditto for all the cases below.
@@ +2971,5 @@
> "The same as 'js-main-runtime/zones/gc-heap-arena-admin'.");
>
> + REPORT_BYTES(NS_LITERAL_CSTRING("js-main-runtime-gc-heap-committed/used/gc-things/objects"),
> + KIND_OTHER, rtStats.cTotals.classInfo.objectsGCHeap,
> + "Allocated object cells");
Please use "Used" instead of "Allocated" to match the path. And please end all descriptions with a '.'. Ditto below.
More importantly, breaking up gcHeapGCThings into its constituent parts makes me nervous -- it would be easy to add a different kind of GC thing later on and not update this code correctly. I'd like you to add an assertion here that the constituent parts are equal to the whole. It's probably best to create a new macro, say, MREPORT_BYTES, which adds the size to a local variable, and then you can check that local variable against gcHeapGCThings down below. We already do something similar for some other cases, e.g. RREPORT_BYTES. I realize this is a bit of a pain, but this code is fragile and there's no good way to tell if it gets broken other than assertions. Thanks.
Attachment #8566060 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 2•10 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #1)
Thanks for the comments!
https://hg.mozilla.org/integration/mozilla-inbound/rev/d60d2602deab
Assignee | ||
Comment 3•10 years ago
|
||
And backed out again for breaking test_memoryReporters.xul.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c419d483d834
Assignee | ||
Comment 4•10 years ago
|
||
Requesting review for updated test code.
Attachment #8566060 -
Attachment is obsolete: true
Attachment #8566586 -
Flags: review?(n.nethercote)
Comment 5•10 years ago
|
||
Comment on attachment 8566586 [details] [diff] [review]
bug1134255-unused-gc-thing-breakdown v2
Review of attachment 8566586 [details] [diff] [review]:
-----------------------------------------------------------------
::: toolkit/components/aboutmemory/tests/test_memoryReporters.xul
@@ +82,5 @@
> } else if (aPath === "resident") {
> residentAmounts.push(aAmount);
> + } else if (aPath.search(/^js-main-runtime-gc-heap-committed\/used\/gc-things\//) >= 0) {
> + jsGcHeapAmount += aAmount;
> + jsGcHeapReporters[aPath] = (jsGcHeapReporters[aPath] | 0) + 1;
Nit: make the variable names closer to the path name? E.g. jsGcHeapUsedGcThingsTotal and jsGcHeapUsedGcThings?
Attachment #8566586 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 6•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•