Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Report telemetry overhead in about:memory

RESOLVED FIXED in mozilla18

Status

()

Toolkit
Telemetry
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: (dormant account), Assigned: froydnj)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

unspecified
mozilla18
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Telemetry use is expanding rapidly. We have histograms, hashtables, lots of decent size strings(as keys for hose hashtables). Would be good to get this surfaced in about:memory to see if it's a problem.
Blocks: 563700
Whiteboard: [MemShrink] → [MemShrink:P2]
Taras says the memory usage should be mostly there after only a short time -- we don't need to wait long.
(Assignee)

Comment 2

5 years ago
I will poke at this; the main pain is setting up helper functions for all the hash tables we have.
Assignee: nobody → nfroyd
OS: Windows 7 → All
Hardware: x86_64 → All
(Assignee)

Comment 3

5 years ago
Created attachment 665739 [details] [diff] [review]
patch

The ipc/ parts of this are a little ugly; I didn't think adding an dependency on xpcom would be a good idea, so the histogram bits have a manual definition of the moral equivalent of nsMallocSizeOfFun.

Not checked for double-counting with DMD< but I'm reasonably certain there's no double-counting here.

The good news is that on startup with a couple of tabs, Telemetry is taking a minimal amount of memory (~50K or so).  So not a big chunk of the 15% of heap-unclassified I see nowadays on my Linux box, but progress nonetheless.
Attachment #665739 - Flags: review?(taras.mozilla)
Attachment #665739 - Flags: review?(n.nethercote)
Comment on attachment 665739 [details] [diff] [review]
patch

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

::: ipc/chromium/src/base/histogram.cc
@@ +414,5 @@
> +{
> +  size_t n = 0;
> +  n += aMallocSizeOf(this);
> +  // We're not allowed to do deep dives into STL data structures.  This
> +  // is as close as we can get to measuring this array.

We use STL data structures in this code?  Yikes.

::: ipc/chromium/src/base/histogram.h
@@ +344,5 @@
>  
>      bool Serialize(Pickle* pickle) const;
>      bool Deserialize(void** iter, const Pickle& pickle);
>  
> +    size_t SizeOfExcludingThis(size_t (*aMallocSizeOf)(const void*));

Can you create a |MallocSizeOf| typedef within Histogram?  And maybe mention in a comment why you're not using the standard |nsMallocSizeOf|.
Attachment #665739 - Flags: review?(n.nethercote) → review+
(Reporter)

Comment 5

5 years ago
Comment on attachment 665739 [details] [diff] [review]
patch

Cool. Looks good to me.
(Reporter)

Comment 6

5 years ago
(In reply to Nicholas Nethercote [:njn] from comment #4)
> Comment on attachment 665739 [details] [diff] [review]
> patch
> 
> Review of attachment 665739 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/chromium/src/base/histogram.cc
> @@ +414,5 @@
> > +{
> > +  size_t n = 0;
> > +  n += aMallocSizeOf(this);
> > +  // We're not allowed to do deep dives into STL data structures.  This
> > +  // is as close as we can get to measuring this array.
> 
> We use STL data structures in this code?  Yikes.

What's wrong with STL in this case?
(Reporter)

Updated

5 years ago
Attachment #665739 - Flags: review?(taras.mozilla) → review+
> What's wrong with STL in this case?

Doesn't Mozilla code generally avoid STL because it uses exceptions and we don't?
(Assignee)

Comment 8

5 years ago
(In reply to Nicholas Nethercote [:njn] from comment #7)
> > What's wrong with STL in this case?
> 
> Doesn't Mozilla code generally avoid STL because it uses exceptions and we
> don't?

There was a short discussion about this on IRC the other day.  vector and string seem to be OK (!); there was at least one other header that was deemed OK (deque?).

I'm not really sure how infallible new and exceptions thereof figure in to that, though. =/

In any event, both the ipc code and the webrtc code use them pretty liberally...
(Assignee)

Comment 9

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9859a8c68136
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/9859a8c68136
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 806514
You need to log in before you can comment on or make changes to this bug.