802894 - Add a memory reporter for nsEffectiveTLDService

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Nicholas Nethercote [inactive]]

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 [inactive]]

Attachment: 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]

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]

s/I'm I'm/If I'm/

Comment 4

[Nicholas Nethercote [inactive]]

Thanks for the quick review!  DMD is my "Dark matter detector" -- see https://wiki.mozilla.org/Performance/MemShrink/DMD.

Comment 5

[Andrew McCreight [:mccr8]]

You could be less jargony by saying "measurement" instead of "DMD".

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: 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]

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 [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/883af38f81c5

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/883af38f81c5

Comment 10

[Nicholas Nethercote [inactive]]

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]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/579e3f9737a3

Comment 12

[Mihai Morar, (:MihaiMorar)]

Can someone please attach a testcase ?