Last Comment Bug 802894 - Add a memory reporter for nsEffectiveTLDService
: Add a memory reporter for nsEffectiveTLDService
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla19
Assigned To: Nicholas Nethercote [:njn]
:
Mentors:
Depends on:
Blocks: B2GDarkMatter 807359
  Show dependency treegraph
 
Reported: 2012-10-17 17:21 PDT by Nicholas Nethercote [:njn]
Modified: 2013-01-14 06:37 PST (History)
7 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Add a memory reporter for nsEffectiveTLDService. (4.98 KB, patch)
2012-10-17 17:21 PDT, Nicholas Nethercote [:njn]
jduell.mcbugs: review-
Details | Diff | Splinter Review
Add a memory reporter for nsEffectiveTLDService. (5.61 KB, patch)
2012-10-29 21:06 PDT, Nicholas Nethercote [:njn]
jduell.mcbugs: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2012-10-17 17:21:41 PDT
The effective-TLD-service hash table takes up 128 KiB of memory on Linux64.
It's not a great deal, but it is one of the larger unaccounted-for chunks
remaining, and pretty easy to measure.
Comment 1 Nicholas Nethercote [:njn] 2012-10-17 17:21:57 PDT
Created attachment 672605 [details] [diff] [review]
Add a memory reporter for nsEffectiveTLDService.

This patch adds a memory reporter with the path
"explicit/components/effective-TLD-service".  I added a |gService| global to
point to the singleton object.

The patch also removes a (related) dead variable in
nsWindowMemoryReporter.cpp.
Comment 2 Jason Duell [:jduell] (needinfo me) 2012-10-17 21:53:59 PDT
Comment on attachment 672605 [details] [diff] [review]
Add a memory reporter for nsEffectiveTLDService.

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

I'll +r this if I'm wrong about the 'strings' variable and you fix the various nits.

::: netwerk/dns/nsEffectiveTLDService.cpp
@@ +56,5 @@
>  #undef ETLD_STR_NUM1
>  
>  // ----------------------------------------------------------------------
>  
> +nsEffectiveTLDService *gService = nullptr;

You should make this static, or if you're feeling trendy, put it in an anonymous namespace.  I'm old school on this issue myself (insert mumbling rant about C++ "improvements" here :)

@@ +124,5 @@
> +size_t
> +nsEffectiveTLDService::SizeOfIncludingThis(nsMallocSizeOfFun aMallocSizeOf)
> +{
> +  size_t n = aMallocSizeOf(this);
> +  n += mHash.SizeOfExcludingThis(NULL, aMallocSizeOf);

Speaking of C++ improvements: nullptr?

Looking at this code, I'm not confident that you're including the size of the static 'strings' member of nsDomainEntry.  This where we're storing all the actual host strings, and it's definitely north of 64K at this point (see bug 704848 comment 18)

  http://mxr.mozilla.org/mozilla-central/source/netwerk/dns/nsEffectiveTLDService.h#96

Perhaps I'm missing something?

@@ +127,5 @@
> +  size_t n = aMallocSizeOf(this);
> +  n += mHash.SizeOfExcludingThis(NULL, aMallocSizeOf);
> +
> +  // Measurement of the following members may be added later if DMD finds it is
> +  // worthwhile:

DMD = Director of Marketing Dept? Wow, their fingers are everywhere now :)

I'm I'm wrong about this perhaps you could expand the acronym...
Comment 3 Jason Duell [:jduell] (needinfo me) 2012-10-17 21:56:14 PDT
s/I'm I'm/If I'm/
Comment 4 Nicholas Nethercote [:njn] 2012-10-17 21:58:47 PDT
Thanks for the quick review!  DMD is my "Dark matter detector" -- see https://wiki.mozilla.org/Performance/MemShrink/DMD.
Comment 5 Andrew McCreight [:mccr8] 2012-10-18 04:14:16 PDT
You could be less jargony by saying "measurement" instead of "DMD".
Comment 6 Nicholas Nethercote [:njn] 2012-10-29 21:06:13 PDT
Created attachment 676466 [details] [diff] [review]
Add a memory reporter for nsEffectiveTLDService.

Updated patch which fixes the nits.  I haven't measured nsDomainEntry::strings
because that's static memory, and we don't measure static memory in
about:memory.
Comment 7 Jason Duell [:jduell] (needinfo me) 2012-10-29 22:41:07 PDT
Comment on attachment 676466 [details] [diff] [review]
Add a memory reporter for nsEffectiveTLDService.

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

> we don't measure static memory in about:memory.

Ah, OK.

Thanks for the patch!
Comment 8 Nicholas Nethercote [:njn] 2012-10-30 21:59:29 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/883af38f81c5
Comment 9 Ed Morley [:emorley] 2012-10-31 07:15:32 PDT
https://hg.mozilla.org/mozilla-central/rev/883af38f81c5
Comment 10 Nicholas Nethercote [:njn] 2012-10-31 15:37:10 PDT
Comment on attachment 676466 [details] [diff] [review]
Add a memory reporter for nsEffectiveTLDService.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):  N/A

User impact if declined:  less understanding of B2G memory consumption.

Testing completed (on m-c, etc.):  has been on m-c for a day without problem.

Risk to taking this patch (and alternatives if risky):  low;  the code is only run when viewing about:memory or dumping memory reports.

String or UUID changes made by this patch:   none.
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-11-01 19:17:51 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/579e3f9737a3
Comment 12 Mihai Morar, (:MihaiMorar) 2013-01-14 06:37:54 PST
Can someone please attach a testcase ?

Note You need to log in before you can comment on or make changes to this bug.