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)

x86_64
Linux
defect
Not set
normal

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)

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] 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
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+
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: