Closed
Bug 872947
Opened 12 years ago
Closed 12 years ago
Regressions in Trace Malloc Leaks from multi-thread support in profiler
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla25
Tracking | Status | |
---|---|---|
firefox22 | --- | unaffected |
firefox23 | - | affected |
firefox24 | --- | affected |
People
(Reporter: mbrubeck, Assigned: BenWa)
References
Details
(Keywords: memory-leak, regression, Whiteboard: [MemShrink:P2])
Attachments
(1 file, 2 obsolete files)
1.81 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
Bug 734691 caused regressions in the Trace Malloc Leaks test on all platforms, including a 95% regression on Windows. Is this regression a problem that may affect our users? Should we back these patches out? (They are currently in the Aurora branch for Firefox 23.)
https://groups.google.com/d/topic/mozilla.dev.tree-management/KcDdDhu3PxQ/discussion
Assignee | ||
Comment 1•12 years ago
|
||
No we shouldn't. We're allocating a pseudostack per thread. Before it was only for the main thread but now it's for registered threads as well. We can clean up this memory on the main thread but we would need an additional branch when poping the pseudostack so we decided it wasn't worth the runtime cost. This is because we need to deinit the profiler when there's still a label expecting to pop.
Now that I think of it for secondary threads that we do a proper Join() on we can probably cleanup the pseudostack after the thread has completed. IMO it's not worth backing out over since it's a fixed amount of memory. I'll prepare a patch to address this regardless just to fix Trace Malloc.
Assignee | ||
Comment 2•12 years ago
|
||
Here's a patch that should help. Haven't ran it through try yet.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #751543 -
Attachment is obsolete: true
Attachment #751549 -
Flags: review?(snorp)
Updated•12 years ago
|
Attachment #751549 -
Flags: review?(snorp) → review+
Comment 4•12 years ago
|
||
Can you address the question "Is this regression a problem that may affect our users?" from comment 0? It's not clear we need to track this.
Flags: needinfo?(bgirard)
Assignee | ||
Comment 5•12 years ago
|
||
IMO no, this memory needs to stay for the life time of the application. This patch will silence the leak but the resolution gives no improvement for the user.
Flags: needinfo?(bgirard)
Comment 6•12 years ago
|
||
Not tracking given comment 5 - we can take a low risk patch for uplift but don't need to track here.
![]() |
||
Comment 7•12 years ago
|
||
Just to clarify: is this a fixed 60 KB allocation(s) that persists for the life of the browser? And does it occur in all browser instances (i.e. do you need to enable the profiler first)?
Assignee | ||
Comment 8•12 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #7)
> Just to clarify: is this a fixed 60 KB allocation(s) that persists for the
> life of the browser?
Yes
> And does it occur in all browser instances (i.e. do
> you need to enable the profiler first)?
It occurs for all instances WITHOUT enabling the profiler.
(In reply to Benoit Girard (:BenWa) from comment #8)
> (In reply to Nicholas Nethercote [:njn] from comment #7)
> > Just to clarify: is this a fixed 60 KB allocation(s) that persists for the
> > life of the browser?
>
> Yes
>
> > And does it occur in all browser instances (i.e. do
> > you need to enable the profiler first)?
>
> It occurs for all instances WITHOUT enabling the profiler.
Burning 60 KB per process on B2G for no gain isn't awesome :-/
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #9)
> (In reply to Benoit Girard (:BenWa) from comment #8)
> > (In reply to Nicholas Nethercote [:njn] from comment #7)
> > > Just to clarify: is this a fixed 60 KB allocation(s) that persists for the
> > > life of the browser?
> >
> > Yes
> >
> > > And does it occur in all browser instances (i.e. do
> > > you need to enable the profiler first)?
> >
> > It occurs for all instances WITHOUT enabling the profiler.
>
> Burning 60 KB per process on B2G for no gain isn't awesome :-/
Ahh right this is unacceptable for B2G I didn't think of that sorry. Let's disable it there for now.
Assignee | ||
Comment 12•12 years ago
|
||
Attachment #751549 -
Attachment is obsolete: true
Attachment #767836 -
Flags: review+
Assignee | ||
Comment 13•12 years ago
|
||
Comment 14•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in
before you can comment on or make changes to this bug.
Description
•