The default bug view has changed. See this FAQ.

Add a memory reporter for nsEffectiveTLDService

RESOLVED FIXED in Firefox 18

Status

()

Core
Networking
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla19
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox18 fixed, firefox19 fixed)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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.
Attachment #672605 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

5 years ago
Blocks: 798484
Whiteboard: [MemShrink]
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...
Attachment #672605 - Flags: review?(jduell.mcbugs) → review-
s/I'm I'm/If I'm/
(Assignee)

Comment 4

5 years ago
Thanks for the quick review!  DMD is my "Dark matter detector" -- see https://wiki.mozilla.org/Performance/MemShrink/DMD.
You could be less jargony by saying "measurement" instead of "DMD".
(Assignee)

Updated

5 years ago
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 6

5 years ago
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.
Attachment #676466 - Flags: review?(jduell.mcbugs)
(Assignee)

Updated

5 years ago
Attachment #672605 - Attachment is obsolete: true
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!
Attachment #676466 - Flags: review?(jduell.mcbugs) → review+
(Assignee)

Comment 8

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/883af38f81c5
https://hg.mozilla.org/mozilla-central/rev/883af38f81c5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Blocks: 807359
(Assignee)

Comment 10

5 years ago
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.
Attachment #676466 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Attachment #676466 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/579e3f9737a3
status-firefox18: --- → fixed
status-firefox19: --- → fixed
Flags: in-testsuite-
Can someone please attach a testcase ?
You need to log in before you can comment on or make changes to this bug.