1431179 - Remove the limit of "exit profiles" that we hold on to

Status: RESOLVED FIXED

(Core :: Gecko Profiler, enhancement)

Description

[Markus Stange [:mstange]]

When a content process shuts down during profiling, we grab a profile from it and put into a list of "exit profiles". If somebody later grabs a profile on the parent process, those exit profiles are included among the subprocess profiles.

The list of exit profiles is currently limited to a length of 5. We should remove that limit. But we should also make sure we discard old profiles as the leave the time range that's currently in the buffer for the parent process.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1431179 - Add profiler_get_buffer_start_time.

Review commit: https://reviewboard.mozilla.org/r/213704/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/213704/

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1431179 - Keep all exit profiles that overlap with the parent process's buffer time range.

Review commit: https://reviewboard.mozilla.org/r/213706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/213706/

Comment 3

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

Comment on attachment 8943378 [details]
Bug 1431179 - Add profiler_get_buffer_start_time.

https://reviewboard.mozilla.org/r/213704/#review219480

::: tools/profiler/core/platform.cpp:2634
(Diff revision 1)
> +  Maybe<double> firstSampleTime =
> +    ActivePS::Buffer(lock).GetTimeOfFirstSampleInBuffer();
> +
> +  if (firstSampleTime) {
> +    return CorePS::ProcessStartTime() +
> +           TimeDuration::FromMilliseconds(*firstSampleTime);
> +  }

TIL about Maybe<T>. Very nice!

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8943378 [details]
Bug 1431179 - Add profiler_get_buffer_start_time.

https://reviewboard.mozilla.org/r/213704/#review219486

lgtm

Comment 5

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

Comment on attachment 8943379 [details]
Bug 1431179 - Keep all exit profiles that overlap with the parent process's buffer time range.

https://reviewboard.mozilla.org/r/213706/#review219488

Looks good! Thanks!

Comment 6

[Markus Stange [:mstange]]

I've thought about this again and have come to the conclusion that I could achieve the same behavior without any TimeStamps, by looking at the buffer's read and write pos and its generation instead. I'll try that out tomorrow.

Comment 7

[Markus Stange [:mstange]]

Attachment: Bug 1431179 - When storing an exit profile, remember the current write position of the parent process's profiler buffer, instead of a TimeStamp.

Review commit: https://reviewboard.mozilla.org/r/216004/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216004/

Comment 8

[Markus Stange [:mstange]]

Attachment: Bug 1431179 - Remove the API to obtain the time of the first sample during profile streaming.

Review commit: https://reviewboard.mozilla.org/r/216006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216006/

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8943379 [details]
Bug 1431179 - Keep all exit profiles that overlap with the parent process's buffer time range.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213706/diff/1-2/

Comment 10

[Markus Stange [:mstange]]

The new patches that I've attached here depend on the work in bug 1348959.

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8945922 [details]
Bug 1431179 - When storing an exit profile, remember the current write position of the parent process's profiler buffer, instead of a TimeStamp.

https://reviewboard.mozilla.org/r/216004/#review222110

::: tools/profiler/gecko/nsProfiler.cpp:553
(Diff revision 1)
>  void
>  nsProfiler::ReceiveShutdownProfile(const nsCString& aProfile)
>  {
>    MOZ_RELEASE_ASSERT(NS_IsMainThread());
>  
> -  if (!profiler_is_active()) {
> +  Maybe<ProfilerBufferInfo> bufferInfo = profiler_get_buffer_info();

Ah, now I see why you made that method return a `Maybe<T>` ^.^ - scratch that earlier comment.

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8945923 [details]
Bug 1431179 - Remove the API to obtain the time of the first sample during profile streaming.

https://reviewboard.mozilla.org/r/216006/#review222112

Yay deleting code~

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8945922 [details]
Bug 1431179 - When storing an exit profile, remember the current write position of the parent process's profiler buffer, instead of a TimeStamp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216004/diff/1-2/

Comment 14

[Markus Stange [:mstange]]

Comment on attachment 8945923 [details]
Bug 1431179 - Remove the API to obtain the time of the first sample during profile streaming.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216006/diff/1-2/

Comment 15

[Markus Stange [:mstange]]

Comment on attachment 8943379 [details]
Bug 1431179 - Keep all exit profiles that overlap with the parent process's buffer time range.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213706/diff/2-3/

Comment 16

[Markus Stange [:mstange]]

Comment on attachment 8945922 [details]
Bug 1431179 - When storing an exit profile, remember the current write position of the parent process's profiler buffer, instead of a TimeStamp.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216004/diff/2-3/

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8945923 [details]
Bug 1431179 - Remove the API to obtain the time of the first sample during profile streaming.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/216006/diff/2-3/

Comment 18

[Markus Stange [:mstange]]

Comment on attachment 8943379 [details]
Bug 1431179 - Keep all exit profiles that overlap with the parent process's buffer time range.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213706/diff/3-4/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/255a28ff7195
When storing an exit profile, remember the current write position of the parent process's profiler buffer, instead of a TimeStamp. r=mystor
https://hg.mozilla.org/integration/autoland/rev/ec0b493bf1bf
Remove the API to obtain the time of the first sample during profile streaming. r=mystor
https://hg.mozilla.org/integration/autoland/rev/8e6d2f2f7f30
Keep all exit profiles that overlap with the parent process's buffer time range. r=mconley

Comment 20

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/255a28ff7195
https://hg.mozilla.org/mozilla-central/rev/ec0b493bf1bf
https://hg.mozilla.org/mozilla-central/rev/8e6d2f2f7f30