Consider adding more reporting for various Telemetry hashtables

RESOLVED FIXED in mozilla34

Status

()

Toolkit
Telemetry
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

unspecified
mozilla34
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P3])

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
There are a number of hashtable fields of Telemetry that do not report any memory owned by their entries.

The entries of mAddonMap are a subclass of nsCStringKey. The data is possibly uniquely owned, but I'm not sure.

mHistogramMap uses an entry based on nsCharPtrHashKey, so that could possibly be measured, too.

mPrivateSQL and mSanitizedSQL also own cstrings.  Similarly, with mTrackedDBs.

Probably the thing to do here is to whip up a prototype and then see if the results add up to anything worth tracking.
(Assignee)

Comment 1

4 years ago
I guess I'll take a look at this.
Assignee: nobody → continuation
(Assignee)

Updated

4 years ago
Depends on: 1046281
(Assignee)

Comment 2

4 years ago
Created attachment 8466428 [details] [diff] [review]
Report memory held onto by more Telemetry hash tables.

mAddonMap seems to require an addon opting in, and AdBlockPlus doesn't seem to use it, so I followed the advice of the comment and left it alone.

mHistogramMap seems to increase by 30k or so when I include what it holds onto, which seems worth counting.  The other tables are only a few k, but the code is easier to read like this, so I figured I'd just convert them over.

All told, this seems to increase the amount of memory in the telemetry entry from about 70k to around 145k.  I'm not sure how that breakdown works, but there you go.
Attachment #8466428 - Flags: review?(nfroyd)
Comment on attachment 8466428 [details] [diff] [review]
Report memory held onto by more Telemetry hash tables.

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

::: toolkit/components/telemetry/Telemetry.cpp
@@ +2607,3 @@
>    // Ignore the hashtables in mAddonMap; they are not significant.
>    n += mAddonMap.SizeOfExcludingThis(nullptr, aMallocSizeOf);
> +  n += mHistogramMap.SizeOfExcludingThis(aMallocSizeOf);

Oh, ugh.  Can you file a followup to make mHistogramMap use nsDepCharHashKey?  I didn't realize that nsCharHashKey strdup's its argument, and that behavior is completely unnecessary in that case.  That should provide a nice reduction in telemetry's memory usage.  Might even be backportable to b2g...?
Attachment #8466428 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 4

4 years ago
(In reply to Nathan Froyd (:froydnj) from comment #3)
> Can you file a followup to make mHistogramMap use nsDepCharHashKey?

Filed bug 1047648.  I'll undo the part of this patch to change mHistogramMap, as it will just have to get reverted anyways for that bug.
(In reply to Andrew McCreight [:mccr8] from comment #4)
> (In reply to Nathan Froyd (:froydnj) from comment #3)
> > Can you file a followup to make mHistogramMap use nsDepCharHashKey?
> 
> Filed bug 1047648.  I'll undo the part of this patch to change
> mHistogramMap, as it will just have to get reverted anyways for that bug.

Either that or add the necessary machinery to nsDepCharHashKey.
(Assignee)

Comment 6

4 years ago
> Either that or add the necessary machinery to nsDepCharHashKey.

nsDepCharHashKey does not own its key, so we cannot report it.
Right, so SizeOfExcludingThis. should just return 0.
(Assignee)

Comment 8

4 years ago
Well, ideally in that case, I'd like to do SFINAE overloading when there's no SizeOfExcludingThis method to make it just treat the entries as 0 (like when we pass in nullptr for the entry callback), so we avoiding iterating over the hash table just to return 0 for every entry.
(In reply to Andrew McCreight [:mccr8] from comment #8)
> Well, ideally in that case, I'd like to do SFINAE overloading when there's
> no SizeOfExcludingThis method to make it just treat the entries as 0 (like
> when we pass in nullptr for the entry callback), so we avoiding iterating
> over the hash table just to return 0 for every entry.

In most cases that would probably be an error. As in "oh this just auto queries the size of function, I'll use that" and then you forget to implement the size of function, but no error is encountered.
Whiteboard: [MemShrink] → [MemShrink:P3]
https://hg.mozilla.org/mozilla-central/rev/1b7fd4e3536b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.