1200795 - CCGraph::mPtrToNodeMap not reported

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

While looking at a DMD report from bug 1156484, I noticed that somehow we don't report mPtrToNodeMap, which can be a pretty large hash table. I have no idea how that happened. Anyways, somebody just needs to add something for this to CCGraph::SizeOfExcludingThis(). In this log I have here, the hash table is 4MB, so it is quite large, but it only lives for the duration of the cycle collection, which isn't going to be more than a second at most.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: Add memory reporting for CCGraph::mPtrToNodeMap.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a4ad888ce19

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 8656793 [details] [diff] [review]
Add memory reporting for CCGraph::mPtrToNodeMap.

Review of attachment 8656793 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpcom/base/nsCycleCollector.cpp
@@ +1318,5 @@
>    void SizeOfIncludingThis(mozilla::MallocSizeOf aMallocSizeOf,
>                             size_t* aObjectSize,
>                             size_t* aGraphNodesSize,
>                             size_t* aGraphEdgesSize,
>                             size_t* aWeakMapsSize,

There are now enough arguments here that it might be worth bundling them up (including MallocSizeOf) into a struct. Bonus points if you do that :)

Comment 3

[Andrew McCreight [:mccr8]]

I was also thinking about eliminating the subcategories. It seems like overkill.

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> I was also thinking about eliminating the subcategories. It seems like
> overkill.

Ok by me; I don't really understand all the different things they're measuring anyway :)  And these don't show up often.

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Eliminate excessive detail from cycle collector graph memory reporting.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Add memory reporting for CCGraph::mPtrToNodeMap.

Comment 7

[Andrew McCreight [:mccr8]]

Comment on attachment 8656894 [details] [diff] [review]
part 2 - Add memory reporting for CCGraph::mPtrToNodeMap.

Actually adding the reporting is now trivial. Carrying over njn's review from the old patch.

Comment 8

[Nicholas Nethercote [inactive]]

Comment on attachment 8656893 [details] [diff] [review]
part 1 - Eliminate excessive detail from cycle collector graph memory reporting.

Review of attachment 8656893 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this is better.

::: xpcom/base/nsCycleCollector.cpp
@@ +3330,5 @@
>    REPORT("explicit/cycle-collector/collector-object", objectSize,
>           "Memory used for the cycle collector object itself.");
>  
> +  REPORT("explicit/cycle-collector/graph", graphSize,
> +         "Memory used for the cycle collector's graph. "

Nit: extraneous space at end.

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #8)
> Nit: extraneous space at end.

This space is needed as a separator for the next line of the description, so I left it in.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b780ee79856
https://hg.mozilla.org/integration/mozilla-inbound/rev/e995e2992559

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/2b780ee79856
https://hg.mozilla.org/mozilla-central/rev/e995e2992559