Closed Bug 746714 Opened 13 years ago Closed 13 years ago

Report telemetry overhead in about:memory

Categories

(Toolkit :: Telemetry, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: taras.mozilla, Assigned: froydnj)

References

(Blocks 1 open bug)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file)

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: DarkMatter
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.
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
Attached patch patchSplinter Review
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+
Comment on attachment 665739 [details] [diff] [review] patch Cool. Looks good to me.
(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?
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?
(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...
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: