Closed Bug 1134255 Opened 9 years ago Closed 9 years ago

Add breakdown of allocated and unused GC things by kind in memory reports


(Core :: JavaScript: GC, defect)

Not set



Tracking Status
firefox38 --- fixed


(Reporter: jonco, Assigned: jonco)


(Blocks 1 open bug)


(Whiteboard: [MemShrink])


(1 file, 1 obsolete file)

Attached patch unused-gc-thing-breakdown (obsolete) — 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)
Whiteboard: [MemShrink]
Comment on attachment 8566060 [details] [diff] [review]

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 @@
> +        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+
(In reply to Nicholas Nethercote [:njn] from comment #1)
Thanks for the comments!
And backed out again for breaking test_memoryReporters.xul.
Requesting review for updated test code.
Attachment #8566060 - Attachment is obsolete: true
Attachment #8566586 - Flags: review?(n.nethercote)
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 (^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+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.