Closed
Bug 746714
Opened 13 years ago
Closed 13 years ago
Report telemetry overhead in about:memory
Categories
(Toolkit :: Telemetry, defect)
Toolkit
Telemetry
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: taras.mozilla, Assigned: froydnj)
References
(Blocks 1 open bug)
Details
(Whiteboard: [MemShrink:P2])
Attachments
(1 file)
8.57 KB,
patch
|
taras.mozilla
:
review+
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
||
Updated•13 years ago
|
Blocks: DarkMatter
Whiteboard: [MemShrink] → [MemShrink:P2]
![]() |
||
Comment 1•13 years ago
|
||
Taras says the memory usage should be mostly there after only a short time -- we don't need to wait long.
![]() |
Assignee | |
Comment 2•13 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•13 years ago
|
||
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 4•13 years ago
|
||
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•13 years ago
|
||
Comment on attachment 665739 [details] [diff] [review]
patch
Cool. Looks good to me.
Reporter | ||
Comment 6•13 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•13 years ago
|
Attachment #665739 -
Flags: review?(taras.mozilla) → review+
![]() |
||
Comment 7•13 years ago
|
||
> 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•13 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•13 years ago
|
||
Status: NEW → ASSIGNED
Comment 10•13 years ago
|
||
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.
Description
•