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

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Jon Coppeard (:jonco)]

Attachment: unused-gc-thing-breakdown

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.

Comment 1

[Nicholas Nethercote [inactive]]

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.

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Nicholas Nethercote [:njn] from comment #1)
Thanks for the comments!

https://hg.mozilla.org/integration/mozilla-inbound/rev/d60d2602deab

Comment 3

[Jon Coppeard (:jonco)]

And backed out again for breaking test_memoryReporters.xul.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c419d483d834

Comment 4

[Jon Coppeard (:jonco)]

Attachment: bug1134255-unused-gc-thing-breakdown v2

Requesting review for updated test code.

Comment 5

[Nicholas Nethercote [inactive]]

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?

Comment 6

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f08229fa0025

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f08229fa0025