Closed Bug 1193838 Opened 9 years ago Closed 9 years ago

Make it possible to gather profiles from processes that have exited

Categories

(Core :: Gecko Profiler, defect, P1)

Product:
Core
Component:
Gecko Profiler
Platform:
x86
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
e10s + ---
firefox45 --- fixed

People

(Reporter: mconley, Assigned: mconley)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 4 obsolete files)

MozReview Request: Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa
40 bytes, text/x-review-board-request
BenWa
: review+
Details
MozReview Request: Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r=BenWa
40 bytes, text/x-review-board-request
BenWa
: review+
Details
MozReview Request: Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r=BenWa
40 bytes, text/x-review-board-request
BenWa
: review+
Details
Bugnotes
9.89 KB, application/zip
Details
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.
Bug 1193838 - Make ProfileGatherer exist during the lifetime of a GeckoSampler. r?BenWa
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
Attachment #8648745 - Attachment description: MozReview Request: Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler. r?BenWa → MozReview Request: Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r?BenWa
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.
Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r?BenWa
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 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 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 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 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 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
Bug 1103094 - Add accessor for nsIProfilerStartParams to nsIProfiler. r?BenWa
Bug 1103094 - Start profiling subprocesses if the parent process is already profiling. r?BenWa
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 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 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
Add some instrumentation and logging
Make talos use async profile dumping stuff
kats - I've rebased this patch off of central in case any of this WIP is useful to you.
Thanks! I'll do a push to try and hope it works :)
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 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 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 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 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 on attachment 8669894 [details] MozReview Request: Add some instrumentation and logging Add some instrumentation and logging
Comment on attachment 8669895 [details] MozReview Request: Make talos use async profile dumping stuff Make talos use async profile dumping stuff
Priority: -- → P1
Attachment #8648744 - Flags: review?(bgirard)
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/
Attachment #8648745 - Flags: review?(bgirard)
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/
Attachment #8649513 - Flags: review?(bgirard)
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/
Attachment #8669892 - Attachment is obsolete: true
Attachment #8669893 - Attachment is obsolete: true
Attachment #8669894 - Attachment is obsolete: true
Attachment #8669895 - Attachment is obsolete: true
Attachment #8648744 - Flags: review?(bgirard)
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 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?
Attachment #8649513 - Flags: review?(bgirard) → review+
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.
Attachment #8648745 - Flags: review?(bgirard) → review+
(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?
Flags: needinfo?(bgirard)
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.
Flags: needinfo?(bgirard)
(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.
Blocks: 1229441
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 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/
Attachment #8648744 - Flags: review?(bgirard)
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/
Attachment #8648745 - Attachment description: MozReview Request: Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r?BenWa → MozReview Request: Bug 1193838 - Expose ProfileGatherer as an nsISupports through nsIProfiler for process parent actors. r=BenWa
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/
Attachment #8649513 - Attachment description: MozReview Request: Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r?BenWa → MozReview Request: Bug 1193838 - Allow ProfileGatherer to gather profiles from exiting processes. r=BenWa
Also filed bug 1229441 to do the work described in comment 35.
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
Attachment #8648744 - Flags: review?(bgirard) → review+
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
Depends on: 1333464
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: