Closed
Bug 1384693
Opened 7 years ago
Closed 7 years ago
Do not include process exit profiles in the collected profile if they have no overlap with the profile from the parent process
Categories
(Core :: Gecko Profiler, enhancement)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox56 | --- | fixed |
People
(Reporter: mstange, Assigned: mstange)
Details
Attachments
(2 files)
When a content process shuts down, it sends a shutdown profile (also called "exit profile") to the parent. The parent holds on to this profile until it's asked to collect a profile, at which point it inserts the profile from the content process into the "processes" list of the parent process profile. However, a long time can pass between these two events. In that time, the parent process might have overwritten its profile buffer many times. The profile you can end up with will have an exit profile on its left end, current profiles on the right end, and a huge gap of nothingness in between. This is both confusing and unnecessary; usually, once the parent process has wrapped around in its profile buffer, you're no longer interested in things that happened before, regardless from what process. So I think we should just discard exit profiles if they have no overlap with the parent process profile.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8890499 [details] Bug 1384693 - Let callers of profiler_stream_json_for_this_process obtain the time of the first sample in the buffer. https://reviewboard.mozilla.org/r/161636/#review167038 I already find all the streaming functions hard to follow, esp. with some of them having many arguments, and this doesn't help. But I can see that it's useful. It feels like there is a refactoring to be done here, but not in this bug. ::: tools/profiler/core/ThreadInfo.h:214 (Diff revision 1) > // > > public: > void StreamJSON(const ProfileBuffer& aBuffer, SpliceableJSONWriter& aWriter, > const mozilla::TimeStamp& aProcessStartTime, > - double aSinceTime); > + double aSinceTime, double* aOutFirstSampleTime); Use the return value instead of an outparam? ::: tools/profiler/core/ThreadInfo.cpp:273 (Diff revision 1) > > // Measurement of the following members may be added later if DMD finds it > // is worthwhile: > // - mPlatformData > // - mSavedStreamedSamples > + // - mFirstSavedStreamedSampleTime mFirstSavedStreamedSampleTime is a scalar so it doesn't need to be mentioned here -- it doesn't involve additional memory outside of this object.
Attachment #8890499 -
Flags: review?(n.nethercote) → review+
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8890500 [details] Bug 1384693 - Cull exit profiles that don't have any overlap with the parent process profile. https://reviewboard.mozilla.org/r/161638/#review167044
Attachment #8890500 -
Flags: review?(n.nethercote) → review+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8890499 [details] Bug 1384693 - Let callers of profiler_stream_json_for_this_process obtain the time of the first sample in the buffer. https://reviewboard.mozilla.org/r/161636/#review167038 I agree.
Assignee | ||
Comment 8•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8890499 [details] Bug 1384693 - Let callers of profiler_stream_json_for_this_process obtain the time of the first sample in the buffer. https://reviewboard.mozilla.org/r/161636/#review167038 > mFirstSavedStreamedSampleTime is a scalar so it doesn't need to be mentioned here -- it doesn't involve additional memory outside of this object. Oops, I clearly wasn't actually reading what this was about. Fixed.
Pushed by mstange@themasta.com: https://hg.mozilla.org/integration/autoland/rev/4d932d0c9a76 Let callers of profiler_stream_json_for_this_process obtain the time of the first sample in the buffer. r=njn https://hg.mozilla.org/integration/autoland/rev/926fa981b826 Cull exit profiles that don't have any overlap with the parent process profile. r=njn
Comment 10•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4d932d0c9a76 https://hg.mozilla.org/mozilla-central/rev/926fa981b826
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in
before you can comment on or make changes to this bug.
Description
•