Regressions in Trace Malloc Leaks from multi-thread support in profiler

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mbrubeck, Assigned: BenWa)

Tracking

({memory-leak, regression})

Trunk
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox22 unaffected, firefox23- affected, firefox24 affected)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 attachment, 2 obsolete attachments)

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
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.
Posted patch patch (obsolete) — Splinter Review
Here's a patch that should help. Haven't ran it through try yet.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Posted patch patch (obsolete) — Splinter Review
Attachment #751543 - Attachment is obsolete: true
Attachment #751549 - Flags: review?(snorp)
Attachment #751549 - Flags: review?(snorp) → review+
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)
Blocks: 874658
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)
Not tracking given comment 5 - we can take a low risk patch for uplift but don't need to track here.
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)?
(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]
(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.
Duplicate of this bug: 887100
https://hg.mozilla.org/mozilla-central/rev/0f758ffc6db1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Blocks: 887826
You need to log in before you can comment on or make changes to this bug.