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

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Matt Brubeck (:mbrubeck)]

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

Comment 1

[Benoit Girard (:BenWa)]

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.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch

Here's a patch that should help. Haven't ran it through try yet.

Comment 3

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 5

[Benoit Girard (:BenWa)]

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.

Comment 6

[Lukas Blakk [:lsblakk] use ?needinfo]

Not tracking given comment 5 - we can take a low risk patch for uplift but don't need to track here.

Comment 7

[Nicholas Nethercote [inactive]]

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)?

Comment 8

[Benoit Girard (:BenWa)]

(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.

Comment 9

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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 :-/

Comment 10

[Benoit Girard (:BenWa)]

(In reply to Kyle Huey [:khuey] ([e-mail]) 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.

Comment 12

[Benoit Girard (:BenWa)]

Attachment: patch rebased

https://tbpl.mozilla.org/?tree=Try&rev=b03c1f65d484

Comment 13

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f758ffc6db1

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0f758ffc6db1