1193838 - Make it possible to gather profiles from processes that have exited

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P1)

Description

[Mike Conley (:mconley) (:⚙️)]

While profiling, when a subprocess exits before the parent process requests the profiles, we lose the profile data from that subprocess.

The subprocess should cough up its performance profile data before it exists if it detects that the parent is currently profiling.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa

Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r=BenWa

Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler. r?BenWa

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8648744 [details]
MozReview Request: Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa

Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8648745 [details]
MozReview Request: Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r=BenWa

Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r?BenWa

We need to let ContentParent and PluginModuleParent get a reference to the ProfileGatherer
during the window of time that we're profiling so that if they start to die (the actor is
starting to go away), they have a gatherer they can send their last profile data to.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r=BenWa

Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r?BenWa

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8648744 [details]
MozReview Request: Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa

Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8648745 [details]
MozReview Request: Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r=BenWa

Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r?BenWa

We need to let ContentParent and PluginModuleParent get a reference to the ProfileGatherer
during the window of time that we're profiling so that if they start to die (the actor is
starting to go away), they have a gatherer they can send their last profile data to.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8649513 [details]
MozReview Request: Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r=BenWa

Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r?BenWa

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8648744 [details]
MozReview Request: Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa

Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8648745 [details]
MozReview Request: Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r=BenWa

Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r?BenWa

We need to let ContentParent and PluginModuleParent get a reference to the ProfileGatherer
during the window of time that we're profiling so that if they start to die (the actor is
starting to go away), they have a gatherer they can send their last profile data to.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8649513 [details]
MozReview Request: Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r=BenWa

Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r?BenWa

Comment 13

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1103094 - Add accessor for nsIProfilerStartParams to nsIProfiler. r?BenWa

Bug 1103094 - Add accessor for nsIProfilerStartParams to nsIProfiler. r?BenWa

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Bug 1103094 - Start profiling subprocesses if the parent process is already profiling. r?BenWa

Bug 1103094 - Start profiling subprocesses if the parent process is already profiling. r?BenWa

Comment 15

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8648744 [details]
MozReview Request: Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa

Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8648745 [details]
MozReview Request: Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r=BenWa

Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r?BenWa

We need to let ContentParent and PluginModuleParent get a reference to the ProfileGatherer
during the window of time that we're profiling so that if they start to die (the actor is
starting to go away), they have a gatherer they can send their last profile data to.

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8649513 [details]
MozReview Request: Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r=BenWa

Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r?BenWa

Comment 18

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Add some instrumentation and logging

Add some instrumentation and logging

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Attachment: MozReview Request: Make talos use async profile dumping stuff

Make talos use async profile dumping stuff

Comment 20

[Mike Conley (:mconley) (:⚙️)]

kats - I've rebased this patch off of central in case any of this WIP is useful to you.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Thanks! I'll do a push to try and hope it works :)

Comment 22

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8669892 [details]
MozReview Request: Bug 1103094 - Add accessor for nsIProfilerStartParams to nsIProfiler. r?BenWa

Bug 1103094 - Add accessor for nsIProfilerStartParams to nsIProfiler. r?BenWa

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8669893 [details]
MozReview Request: Bug 1103094 - Start profiling subprocesses if the parent process is already profiling. r?BenWa

Bug 1103094 - Start profiling subprocesses if the parent process is already profiling. r?BenWa

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8648744 [details]
MozReview Request: Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa

Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8648745 [details]
MozReview Request: Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r=BenWa

Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r?BenWa

We need to let ContentParent and PluginModuleParent get a reference to the ProfileGatherer
during the window of time that we're profiling so that if they start to die (the actor is
starting to go away), they have a gatherer they can send their last profile data to.

Comment 26

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8649513 [details]
MozReview Request: Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r=BenWa

Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r?BenWa

Comment 27

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8669894 [details]
MozReview Request: Add some instrumentation and logging

Add some instrumentation and logging

Comment 28

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8669895 [details]
MozReview Request: Make talos use async profile dumping stuff

Make talos use async profile dumping stuff

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8648744 [details]
MozReview Request: Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/16277/diff/6-7/

Comment 30

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8648745 [details]
MozReview Request: Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r=BenWa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/16279/diff/6-7/

Comment 31

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8649513 [details]
MozReview Request: Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r=BenWa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/16417/diff/5-6/

Comment 32

[Benoit Girard (:BenWa)]

Comment on attachment 8648744 [details]
MozReview Request: Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa

https://reviewboard.mozilla.org/r/16277/#review23801

::: tools/profiler/core/GeckoSampler.cpp:251
(Diff revision 7)
> +  mGatherer = new mozilla::ProfileGatherer(this);

This grabs a pointer to this non-reference counted and the ProfileGatherer can live longer than the GeckoSampler.

We've talked about a few solutions to this in person. We should fix this before landing this patch.

Comment 33

[Benoit Girard (:BenWa)]

Comment on attachment 8649513 [details]
MozReview Request: Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r=BenWa

https://reviewboard.mozilla.org/r/16417/#review23803

::: tools/profiler/gecko/ProfileGatherer.cpp:146
(Diff revision 6)
> +  if (mExitProfiles.Length() >= 5) {

We're keeping up to 5 child process profiles? I'm not sure how I feel about this magic number but it doesn't seem like it's useless either. Perhaps we should explictly document what the intension of this is.

::: tools/profiler/gecko/ProfileGatherer.cpp:150
(Diff revision 6)
> +  profileEntry = aProfile;

Can't we just mExistProfiles.AppendElement(aProfile) here?

Comment 34

[Benoit Girard (:BenWa)]

Comment on attachment 8648745 [details]
MozReview Request: Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r=BenWa

https://reviewboard.mozilla.org/r/16279/#review23807

Looks good once the lifetime issue is resolved.

Comment 35

[Mike Conley (:mconley) (:⚙️)]

(In reply to Benoit Girard (:BenWa) from comment #32)
> Comment on attachment 8648744 [details]
> MozReview Request: Bug 1193838 - Make ProfileGatherer exist during the
> lifetime of a GeckoSampler. r?BenWa
> 
> https://reviewboard.mozilla.org/r/16277/#review23801
> 
> ::: tools/profiler/core/GeckoSampler.cpp:251
> (Diff revision 7)
> > +  mGatherer = new mozilla::ProfileGatherer(this);
> 
> This grabs a pointer to this non-reference counted and the ProfileGatherer
> can live longer than the GeckoSampler.


Yeah, this is a problem. Good catch!

> 
> We've talked about a few solutions to this in person. We should fix this
> before landing this patch.

Okay, here's the solution I've been thinking about:

1. Combine GeckoSampler and ProfileGatherer
2. Have GeckoSampler+ProfileGatherer be refcounted
3. Continue to offer the ProfileGatherer interface to PluginModuleParent and ContentParent, which will inc the refcount on GeckoSampler+ProfileGatherer.
4. On stopping the profiler, instead of directly deleting the GeckoSampler, have platform.cpp just drop the reference to it.
5. If an async profile gathering is still in progress, have PluginModuleParent and ContentParent ignore any profile start/stop/gather observer notifications from the profiler.
6. Once an async profile gathering is complete, have the PluginModuleParent and ContentParent check whether or not we're still profiling. If not, stop and drop the old reference to the gatherer. If still profiling, get a reference to the current GeckoSampler+ProfileGatherer and overwrite mGatherer with it (this might be the same reference, but if we've stopped and restarted the sampler while waiting for the profile to be gathered, it won't).

The consequence of this is that subprocesses that are still in the midst of sending up their profiles to the parent will not hear or obey stop/start/gather commands until the profile has finished gathering.

Does that sound like an acceptable plan?

Comment 36

[Benoit Girard (:BenWa)]

I think it's fine like I said over lunch. It's not ideal but if it's the easiest way forward then it's worth the issues with ignoring/delaying stop/start/gather while we're gathering a profile.

Maybe we should just reject the promise if this happens? It would require a large internal change and it should still be very rare that it would happen.

Comment 37

[Mike Conley (:mconley) (:⚙️)]

(In reply to Benoit Girard (:BenWa) from comment #36)
> I think it's fine like I said over lunch. It's not ideal but if it's the
> easiest way forward then it's worth the issues with ignoring/delaying
> stop/start/gather while we're gathering a profile.
> 
> Maybe we should just reject the promise if this happens? It would require a
> large internal change and it should still be very rare that it would happen.

Yeah, we could do the work I'm suggesting in comment 35 in a follow-up, and just reject for now.

Comment 38

[Mike Conley (:mconley) (:⚙️)]

https://reviewboard.mozilla.org/r/16277/#review23801

> This grabs a pointer to this non-reference counted and the ProfileGatherer can live longer than the GeckoSampler.
> 
> We've talked about a few solutions to this in person. We should fix this before landing this patch.

The GeckoSampler now called Cancel on the ProfileGatherer which will reject the Promise and drop the reference to the GeckoSampler.

Comment 39

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8648744 [details]
MozReview Request: Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/16277/diff/7-8/

Comment 40

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8648745 [details]
MozReview Request: Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r=BenWa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/16279/diff/7-8/

Comment 41

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8649513 [details]
MozReview Request: Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r=BenWa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/16417/diff/6-7/

Comment 42

[Mike Conley (:mconley) (:⚙️)]

Also filed bug 1229441 to do the work described in comment 35.

Comment 43

[Benoit Girard (:BenWa)]

Comment on attachment 8648744 [details]
MozReview Request: Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa

https://reviewboard.mozilla.org/r/16277/#review24371

Comment 44

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a9d57405161

Comment 45

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f0a249fd061

Comment 46

[Mike Conley (:mconley) (:⚙️)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e60725bef357

Comment 47

[Mike Conley (:mconley) (:⚙️)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2dd0eb33623301166c1eeb6e5c0113456af467c
Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r=BenWa

https://hg.mozilla.org/integration/mozilla-inbound/rev/76da1f09f4ee8597d96565270c0e4cf6bcdd48d2
Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r=BenWa

https://hg.mozilla.org/integration/mozilla-inbound/rev/479e586e1a7fb017fcc2522a8b33ad386582762c
Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r=BenWa

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e2dd0eb33623
https://hg.mozilla.org/mozilla-central/rev/76da1f09f4ee
https://hg.mozilla.org/mozilla-central/rev/479e586e1a7f

Comment 49

[Mike Conley (:mconley) (:⚙️)]

Attachment: Bugnotes

http://people.mozilla.org/~mconley2/bugnotes/bug-1193838.html