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

RESOLVED FIXED in Firefox 38

Status

()

Core
JavaScript: GC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jonco, Assigned: jonco)

Tracking

(Blocks: 1 bug)

unspecified
mozilla38
x86_64
Linux
Points:
---

Firefox Tracking Flags

(firefox38 fixed)

Details

(Whiteboard: [MemShrink])

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 8566060 [details] [diff] [review]
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.
Attachment #8566060 - Flags: review?(n.nethercote)
Whiteboard: [MemShrink]
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+
(In reply to Nicholas Nethercote [:njn] from comment #1)
Thanks for the comments!

https://hg.mozilla.org/integration/mozilla-inbound/rev/d60d2602deab
And backed out again for breaking test_memoryReporters.xul.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c419d483d834
Created attachment 8566586 [details] [diff] [review]
bug1134255-unused-gc-thing-breakdown v2

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 (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+
https://hg.mozilla.org/mozilla-central/rev/f08229fa0025
Status: NEW → RESOLVED
Last Resolved: 4 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.