746714 - Report telemetry overhead in about:memory

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Description

[(dormant account)]

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.

Comment 1

[Nicholas Nethercote [inactive]]

Taras says the memory usage should be mostly there after only a short time -- we don't need to wait long.

Comment 2

[Nathan Froyd [:froydnj]]

I will poke at this; the main pain is setting up helper functions for all the hash tables we have.

Comment 3

[Nathan Froyd [:froydnj]]

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

Comment 4

[Nicholas Nethercote [inactive]]

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

Comment 5

[(dormant account)]

Comment on attachment 665739 [details] [diff] [review]
patch

Cool. Looks good to me.

Comment 6

[(dormant account)]

(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?

Comment 7

[Nicholas Nethercote [inactive]]

> What's wrong with STL in this case?

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

Comment 8

[Nathan Froyd [:froydnj]]

(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...

Comment 9

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9859a8c68136

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9859a8c68136