Closed
Bug 1047523
Opened 10 years ago
Closed 10 years ago
Consider adding more reporting for various Telemetry hashtables
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla34
People
(Reporter: mccr8, Assigned: mccr8)
References
(Blocks 1 open bug)
Details
(Whiteboard: [MemShrink:P3])
Attachments
(1 file)
1.57 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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 2•10 years ago
|
||
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 3•10 years ago
|
||
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•10 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.
Comment 5•10 years ago
|
||
(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•10 years ago
|
||
> Either that or add the necessary machinery to nsDepCharHashKey.
nsDepCharHashKey does not own its key, so we cannot report it.
Comment 7•10 years ago
|
||
Right, so SizeOfExcludingThis. should just return 0.
Assignee | ||
Comment 8•10 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.
Assignee | ||
Comment 9•10 years ago
|
||
try run: https://tbpl.mozilla.org/?tree=Try&rev=2c1fd7b1cd8c
Comment 10•10 years ago
|
||
(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.
Updated•10 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P3]
Assignee | ||
Comment 11•10 years ago
|
||
try run: https://tbpl.mozilla.org/?tree=Try&rev=c2d7bf52d31d https://hg.mozilla.org/integration/mozilla-inbound/rev/1b7fd4e3536b
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1b7fd4e3536b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in
before you can comment on or make changes to this bug.
Description
•