Last Comment Bug 746714 - Report telemetry overhead in about:memory
: Report telemetry overhead in about:memory
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Toolkit
Classification: Components
Component: Telemetry (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla18
Assigned To: Nathan Froyd [:froydnj]
:
:
Mentors:
Depends on: 806514
Blocks: DarkMatter
  Show dependency treegraph
 
Reported: 2012-04-18 13:18 PDT by (dormant account)
Modified: 2012-10-29 13:05 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (8.57 KB, patch)
2012-09-27 20:07 PDT, Nathan Froyd [:froydnj]
taras.mozilla: review+
n.nethercote: review+
Details | Diff | Splinter Review

Description (dormant account) 2012-04-18 13:18:30 PDT
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 [:njn] 2012-05-01 16:12:30 PDT
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] 2012-09-12 08:47:37 PDT
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] 2012-09-27 20:07:35 PDT
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.
Comment 4 Nicholas Nethercote [:njn] 2012-09-27 21:46:45 PDT
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) 2012-10-02 13:22:46 PDT
Comment on attachment 665739 [details] [diff] [review]
patch

Cool. Looks good to me.
Comment 6 (dormant account) 2012-10-02 13:24:07 PDT
(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 [:njn] 2012-10-02 19:24:06 PDT
> 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] 2012-10-02 19:31:12 PDT
(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 10 Ed Morley [:emorley] 2012-10-06 12:50:09 PDT
https://hg.mozilla.org/mozilla-central/rev/9859a8c68136

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