Set process phases in the StackSamplingProfiler.

Set process phases in the StackSamplingProfiler.

BUG=657012
Review-Url: https://codereview.chromium.org/2530043002
Cr-Commit-Position: refs/heads/master@{#443394}
Committed: https://chromium.googlesource.com/chromium/src/+/da097d30d0e99243c59c75d336a761b0ec5e8def

[bcwhite, 2016-11-24 19:39:34]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 19:39:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 19:43:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 19:43:18]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bcwhite, 2016-11-24 20:18:15]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 20:18:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-24 20:21:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 20:21:27]

Dry run: Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/blimp_linux_dbg...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[bcwhite, 2016-11-24 20:25:44]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-24 20:25:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2016-11-24 20:27:36]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-11-24 20:59:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-24 20:59:17]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[bcwhite, 2016-12-01 14:52:35]

bcwhite@chromium.org changed reviewers:
+ asvitkine@chromium.org, wittman@chromium.org

[bcwhite, 2016-12-01 15:07:37]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 15:09:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-01 16:01:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 16:01:57]

Dry run: This issue passed the CQ dry run.

[Mike Wittman, 2016-12-01 16:40:33]

To avoid unnecessary complexity in the server-side handling of the phases, I'd
like to have support in place for recording the process phases during GPU
process collection before landing this CL.

https://codereview.chromium.org/2530043002/diff/60001/components/metrics/prot...
File components/metrics/proto/execution_context.proto (right):

https://codereview.chromium.org/2530043002/diff/60001/components/metrics/prot...
components/metrics/proto/execution_context.proto:57: // Based on histogram
Startup.BrowserMainRunnerImplInitializeStep1Time.
This should be based on Startup.BrowserMessageLoopStartTime, which is the most
commonly-used metric for this purpose.

https://codereview.chromium.org/2530043002/diff/60001/content/public/browser/...
File content/public/browser/browser_main_parts.h (right):

https://codereview.chromium.org/2530043002/diff/60001/content/public/browser/...
content/public/browser/browser_main_parts.h:100: virtual void PostShutdown() {}
Internal interfaces in Chrome are generally not added until they're needed, so
this function can be removed.

[bcwhite, 2016-12-01 17:46:06]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 17:46:41]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2016-12-01 17:50:54]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-01 17:51:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2016-12-01 17:59:12]

Patchset #4 (id:80001) has been deleted

[bcwhite, 2016-12-01 17:59:54]

> To avoid unnecessary complexity in the server-side 
> handling of the phases, I'd
> like to have support in place for recording the process
> phases during GPU
> process collection before landing this CL.

Do you mean you have another CL to go in first or you'd like to add something
else to this one?

https://codereview.chromium.org/2530043002/diff/60001/components/metrics/prot...
File components/metrics/proto/execution_context.proto (right):

https://codereview.chromium.org/2530043002/diff/60001/components/metrics/prot...
components/metrics/proto/execution_context.proto:57: // Based on histogram
Startup.BrowserMainRunnerImplInitializeStep1Time.
On 2016/12/01 16:40:33, Mike Wittman wrote:
> This should be based on Startup.BrowserMessageLoopStartTime, which is the most
> commonly-used metric for this purpose.

Done.

https://codereview.chromium.org/2530043002/diff/60001/content/public/browser/...
File content/public/browser/browser_main_parts.h (right):

https://codereview.chromium.org/2530043002/diff/60001/content/public/browser/...
content/public/browser/browser_main_parts.h:100: virtual void PostShutdown() {}
On 2016/12/01 16:40:33, Mike Wittman wrote:
> Internal interfaces in Chrome are generally not added until they're needed, so
> this function can be removed.

Done.

[Mike Wittman, 2016-12-01 18:46:50]

On 2016/12/01 17:59:54, bcwhite wrote:
> > To avoid unnecessary complexity in the server-side 
> > handling of the phases, I'd
> > like to have support in place for recording the process
> > phases during GPU
> > process collection before landing this CL.
> 
> Do you mean you have another CL to go in first or you'd like to add something
> else to this one?

Another CL. To correlate the histograms with data across processes, the phases
will need to be shared from the browser process to supported child processes so
they can be recorded during collection in those processes. Presumably this will
be non-trivial, although it potentially could piggy-back on lawrencewu@'s work
setting up a metrics shared memory segment between browser and child processes.

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:607:
metrics::CallStackProfileMetricsProvider::MAIN_LOOP_START);
This line should be as close as possible to where we get the time, but
unfortunately that's is in the caller and not proximate to the histogram
recording below.

To keep these three things together I'd suggest making the
base::TimeTicks::Now() call at the start of this function, then setting the
phase.

[commit-bot: I haz the power, 2016-12-01 18:51:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-01 18:51:26]

Dry run: This issue passed the CQ dry run.

[Alexei Svitkine (slow), 2016-12-01 19:49:13]

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:607:
metrics::CallStackProfileMetricsProvider::MAIN_LOOP_START);
On 2016/12/01 18:46:50, Mike Wittman wrote:
> This line should be as close as possible to where we get the time, but
> unfortunately that's is in the caller and not proximate to the histogram
> recording below.
> 
> To keep these three things together I'd suggest making the
> base::TimeTicks::Now() call at the start of this function, then setting the
> phase.

Right, I think we need to clearly define what SetProcessPhase() means - because
its name implies you should call it at the *start* of a phase, but the way this
CL is doing it, is you're calling it at the *end* of a phase - i.e. you're
calling it when the histogram is logged - which is at the end of the time.

I agree that logging the *end* of a phase is nice because then you can keep the
code to set it can live right next to the histogram - as is being done here.
However, then the API should be renamed to make that clear.

[Mike Wittman, 2016-12-01 23:00:48]

On 2016/12/01 18:46:50, Mike Wittman wrote:
> On 2016/12/01 17:59:54, bcwhite wrote:
> > > To avoid unnecessary complexity in the server-side 
> > > handling of the phases, I'd
> > > like to have support in place for recording the process
> > > phases during GPU
> > > process collection before landing this CL.
> > 
> > Do you mean you have another CL to go in first or you'd like to add
something
> > else to this one?
> 
> Another CL. To correlate the histograms with data across processes, the phases
> will need to be shared from the browser process to supported child processes
so
> they can be recorded during collection in those processes. Presumably this
will
> be non-trivial, although it potentially could piggy-back on lawrencewu@'s work
> setting up a metrics shared memory segment between browser and child
processes.

Just to clarify, I don't have a CL implementing this. I was hoping you could
tackle that change. :)

[bcwhite, 2016-12-08 22:33:05]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 22:33:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 23:55:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 23:56:00]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2016-12-09 22:46:40]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-09 22:47:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-10 01:58:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-10 01:58:22]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2016-12-16 02:10:59]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-16 02:12:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 05:53:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 05:53:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bcwhite, 2017-01-05 15:09:49]

On 2016/12/01 23:00:48, Mike Wittman wrote:
> On 2016/12/01 18:46:50, Mike Wittman wrote:
> > On 2016/12/01 17:59:54, bcwhite wrote:
> > > > To avoid unnecessary complexity in the server-side 
> > > > handling of the phases, I'd
> > > > like to have support in place for recording the process
> > > > phases during GPU
> > > > process collection before landing this CL.
> > > 
> > > Do you mean you have another CL to go in first or you'd like to add
> something
> > > else to this one?
> > 
> > Another CL. To correlate the histograms with data across processes, the
phases
> > will need to be shared from the browser process to supported child processes
> so
> > they can be recorded during collection in those processes. Presumably this
> will
> > be non-trivial, although it potentially could piggy-back on lawrencewu@'s
work
> > setting up a metrics shared memory segment between browser and child
> processes.
> 
> Just to clarify, I don't have a CL implementing this. I was hoping you could
> tackle that change. :)

Oh, just saw this last comment.  The best way would be to take the shared memory
segment used for passing fieldtrial info and make it a more generic
"configuration" thing.  Then those bit-fields could be stored in there.  Sadly,
the intern who did that has left so it'll take some to do that.

Is it necessary to wait?  Wouldn't this be of some benefit even without
cross-process support?

[Mike Wittman, 2017-01-05 20:39:10]

On 2017/01/05 15:09:49, bcwhite wrote:
> On 2016/12/01 23:00:48, Mike Wittman wrote:
> > On 2016/12/01 18:46:50, Mike Wittman wrote:
> > > On 2016/12/01 17:59:54, bcwhite wrote:
> > > > > To avoid unnecessary complexity in the server-side 
> > > > > handling of the phases, I'd
> > > > > like to have support in place for recording the process
> > > > > phases during GPU
> > > > > process collection before landing this CL.
> > > > 
> > > > Do you mean you have another CL to go in first or you'd like to add
> > something
> > > > else to this one?
> > > 
> > > Another CL. To correlate the histograms with data across processes, the
> phases
> > > will need to be shared from the browser process to supported child
processes
> > so
> > > they can be recorded during collection in those processes. Presumably this
> > will
> > > be non-trivial, although it potentially could piggy-back on lawrencewu@'s
> work
> > > setting up a metrics shared memory segment between browser and child
> > processes.
> > 
> > Just to clarify, I don't have a CL implementing this. I was hoping you could
> > tackle that change. :)
> 
> Oh, just saw this last comment.  The best way would be to take the shared
memory
> segment used for passing fieldtrial info and make it a more generic
> "configuration" thing.  Then those bit-fields could be stored in there. 
Sadly,
> the intern who did that has left so it'll take some to do that.
> 
> Is it necessary to wait?  Wouldn't this be of some benefit even without
> cross-process support?

After fleshing out a design for the server-side support for this functionality,
I think it should be OK to move ahead with this with just browser process
support. It'll add some server complexity, but we should be able to live with
that as long as support for other processes is added before too long.

LGTM

[bcwhite, 2017-01-05 20:52:55]

Patchset #5 (id:120001) has been deleted

[bcwhite, 2017-01-05 20:53:05]

Patchset #5 (id:140001) has been deleted

[Alexei Svitkine (slow), 2017-01-05 20:54:12]

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:607:
metrics::CallStackProfileMetricsProvider::MAIN_LOOP_START);
On 2016/12/01 19:49:12, Alexei Svitkine (slow) wrote:
> On 2016/12/01 18:46:50, Mike Wittman wrote:
> > This line should be as close as possible to where we get the time, but
> > unfortunately that's is in the caller and not proximate to the histogram
> > recording below.
> > 
> > To keep these three things together I'd suggest making the
> > base::TimeTicks::Now() call at the start of this function, then setting the
> > phase.
> 
> Right, I think we need to clearly define what SetProcessPhase() means -
because
> its name implies you should call it at the *start* of a phase, but the way
this
> CL is doing it, is you're calling it at the *end* of a phase - i.e. you're
> calling it when the histogram is logged - which is at the end of the time.
> 
> I agree that logging the *end* of a phase is nice because then you can keep
the
> code to set it can live right next to the histogram - as is being done here.
> However, then the API should be renamed to make that clear.

Ping on this comment.

[bcwhite, 2017-01-06 14:17:12]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 14:17:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-06 16:17:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 16:17:50]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bcwhite, 2017-01-06 16:50:56]

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:607:
metrics::CallStackProfileMetricsProvider::MAIN_LOOP_START);
> Right, I think we need to clearly define what SetProcessPhase() means -
because
> its name implies you should call it at the *start* of a phase, but the way
this
> CL is doing it, is you're calling it at the *end* of a phase - i.e. you're
> calling it when the histogram is logged - which is at the end of the time.

In this case, the name indicates that the "main loop has started".  Perhaps
simply changing the name to MAIN_LOOP_STARTED would be sufficient?

[bcwhite, 2017-01-06 19:07:30]

Patchset #5 (id:160001) has been deleted

[Alexei Svitkine (slow), 2017-01-09 19:12:18]

lgtm % comment that suggests renaming the API

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:607:
metrics::CallStackProfileMetricsProvider::MAIN_LOOP_START);
On 2017/01/06 16:50:56, bcwhite wrote:
> > Right, I think we need to clearly define what SetProcessPhase() means -
> because
> > its name implies you should call it at the *start* of a phase, but the way
> this
> > CL is doing it, is you're calling it at the *end* of a phase - i.e. you're
> > calling it when the histogram is logged - which is at the end of the time.
> 
> In this case, the name indicates that the "main loop has started".  Perhaps
> simply changing the name to MAIN_LOOP_STARTED would be sufficient?

I guess I'm less concerned with the name and more that I would like to make the
API & comment more explicit by what is meant.

e.g. change SetProcessPhase to something like SetLastCompletedProcessPhase() -
to make it clear that a call to this function implies the phase ended and not
started. Otherwise, its name makes me think you're setting the current phase
that's happening right now.

[bcwhite, 2017-01-10 17:56:54]

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:607:
metrics::CallStackProfileMetricsProvider::MAIN_LOOP_START);
> > In this case, the name indicates that the "main loop has started".  Perhaps
> > simply changing the name to MAIN_LOOP_STARTED would be sufficient?
> 
> I guess I'm less concerned with the name and more that I would like to make
the
> API & comment more explicit by what is meant.
> 
> e.g. change SetProcessPhase to something like SetLastCompletedProcessPhase() -
> to make it clear that a call to this function implies the phase ended and not
> started. Otherwise, its name makes me think you're setting the current phase
> that's happening right now.

That's not what it does, though.  Perhaps "SetProcessMilestone" would be a
better name because it's not specifically beginning or ending of a phase.  For
example, the "shutdown" phase indicates that shutdown in starting.

[Alexei Svitkine (slow), 2017-01-10 18:05:41]

SetProcessMilestone SGTM

On Tue, Jan 10, 2017 at 12:56 PM, <bcwhite@chromium.org> wrote:

>
> https://codereview.chromium.org/2530043002/diff/100001/
> components/startup_metric_utils/browser/startup_metric_utils.cc
> File components/startup_metric_utils/browser/startup_metric_utils.cc
> (right):
>
> https://codereview.chromium.org/2530043002/diff/100001/
> components/startup_metric_utils/browser/startup_metric_utils.cc#newcode607
> components/startup_metric_utils/browser/startup_metric_utils.cc:607:
> metrics::CallStackProfileMetricsProvider::MAIN_LOOP_START);
> > > In this case, the name indicates that the "main loop has started".
> Perhaps
> > > simply changing the name to MAIN_LOOP_STARTED would be sufficient?
> >
> > I guess I'm less concerned with the name and more that I would like to
> make the
> > API & comment more explicit by what is meant.
> >
> > e.g. change SetProcessPhase to something like
> SetLastCompletedProcessPhase() -
> > to make it clear that a call to this function implies the phase ended
> and not
> > started. Otherwise, its name makes me think you're setting the current
> phase
> > that's happening right now.
>
> That's not what it does, though. Perhaps "SetProcessMilestone" would be
> a better name because it's not specifically beginning or ending of a
> phase. For example, the "shutdown" phase indicates that shutdown in
> starting.
>
> https://codereview.chromium.org/2530043002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2017-01-10 18:31:54]

>
> SetProcessMilestone SGTM
>

Okay...  Do I change it in all the protobufs, too?

-- Brian



>
> On Tue, Jan 10, 2017 at 12:56 PM, <bcwhite@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/2530043002/diff/100001/compo
>> nents/startup_metric_utils/browser/startup_metric_utils.cc
>> File components/startup_metric_utils/browser/startup_metric_utils.cc
>> (right):
>>
>> https://codereview.chromium.org/2530043002/diff/100001/compo
>> nents/startup_metric_utils/browser/startup_metric_utils.cc#newcode607
>> components/startup_metric_utils/browser/startup_metric_utils.cc:607:
>> metrics::CallStackProfileMetricsProvider::MAIN_LOOP_START);
>> > > In this case, the name indicates that the "main loop has started".
>> Perhaps
>> > > simply changing the name to MAIN_LOOP_STARTED would be sufficient?
>> >
>> > I guess I'm less concerned with the name and more that I would like to
>> make the
>> > API & comment more explicit by what is meant.
>> >
>> > e.g. change SetProcessPhase to something like
>> SetLastCompletedProcessPhase() -
>> > to make it clear that a call to this function implies the phase ended
>> and not
>> > started. Otherwise, its name makes me think you're setting the current
>> phase
>> > that's happening right now.
>>
>> That's not what it does, though. Perhaps "SetProcessMilestone" would be
>> a better name because it's not specifically beginning or ending of a
>> phase. For example, the "shutdown" phase indicates that shutdown in
>> starting.
>>
>> https://codereview.chromium.org/2530043002/
>>
>
>


-- 
  Brian
  bcwhite@google.com
-----------------------------------------------------------------------------------------



*Treat someone as they are and they will remain that way.Treat someone as
they can be and they will become that way.*

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Alexei Svitkine (slow), 2017-01-10 19:10:25]

Let's just do it in the API since that was my concern. Maybe it could be
named SetProcessPhaseMilestone to bridge the two?

On Tue, Jan 10, 2017 at 1:31 PM, Brian White <bcwhite@google.com> wrote:

> SetProcessMilestone SGTM
>>
>
> Okay...  Do I change it in all the protobufs, too?
>
> -- Brian
>
>
>
>>
>> On Tue, Jan 10, 2017 at 12:56 PM, <bcwhite@chromium.org> wrote:
>>
>>>
>>> https://codereview.chromium.org/2530043002/diff/100001/compo
>>> nents/startup_metric_utils/browser/startup_metric_utils.cc
>>> File components/startup_metric_utils/browser/startup_metric_utils.cc
>>> (right):
>>>
>>> https://codereview.chromium.org/2530043002/diff/100001/compo
>>> nents/startup_metric_utils/browser/startup_metric_utils.cc#newcode607
>>> components/startup_metric_utils/browser/startup_metric_utils.cc:607:
>>> metrics::CallStackProfileMetricsProvider::MAIN_LOOP_START);
>>> > > In this case, the name indicates that the "main loop has started".
>>> Perhaps
>>> > > simply changing the name to MAIN_LOOP_STARTED would be sufficient?
>>> >
>>> > I guess I'm less concerned with the name and more that I would like to
>>> make the
>>> > API & comment more explicit by what is meant.
>>> >
>>> > e.g. change SetProcessPhase to something like
>>> SetLastCompletedProcessPhase() -
>>> > to make it clear that a call to this function implies the phase ended
>>> and not
>>> > started. Otherwise, its name makes me think you're setting the current
>>> phase
>>> > that's happening right now.
>>>
>>> That's not what it does, though. Perhaps "SetProcessMilestone" would be
>>> a better name because it's not specifically beginning or ending of a
>>> phase. For example, the "shutdown" phase indicates that shutdown in
>>> starting.
>>>
>>> https://codereview.chromium.org/2530043002/
>>>
>>
>>
>
>
> --
>   Brian
>   bcwhite@google.com
> ------------------------------------------------------------
> -----------------------------
>
>
>
> *Treat someone as they are and they will remain that way.Treat someone as
> they can be and they will become that way.*
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mike Wittman, 2017-01-10 20:10:36]

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:607:
metrics::CallStackProfileMetricsProvider::MAIN_LOOP_START);
On 2016/12/01 18:46:50, Mike Wittman wrote:
> This line should be as close as possible to where we get the time, but
> unfortunately that's is in the caller and not proximate to the histogram
> recording below.
> 
> To keep these three things together I'd suggest making the
> base::TimeTicks::Now() call at the start of this function, then setting the
> phase.

Just noticed that this comment still remains to be addressed. Please make this
change before submitting.

[bcwhite, 2017-01-11 15:30:02]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 15:30:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-01-11 16:15:48]

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:607:
metrics::CallStackProfileMetricsProvider::MAIN_LOOP_START);
On 2017/01/10 20:10:36, Mike Wittman wrote:
> On 2016/12/01 18:46:50, Mike Wittman wrote:
> > This line should be as close as possible to where we get the time, but
> > unfortunately that's is in the caller and not proximate to the histogram
> > recording below.
> > 
> > To keep these three things together I'd suggest making the
> > base::TimeTicks::Now() call at the start of this function, then setting the
> > phase.
> 
> Just noticed that this comment still remains to be addressed. Please make this
> change before submitting.

If I understand you correctly, this amounts to removing the |ticks| parameter
and getting it directly at the start of the method, then immediately setting the
phase.

[Mike Wittman, 2017-01-11 16:24:47]

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:607:
metrics::CallStackProfileMetricsProvider::MAIN_LOOP_START);
On 2017/01/11 16:15:48, bcwhite wrote:
> On 2017/01/10 20:10:36, Mike Wittman wrote:
> > On 2016/12/01 18:46:50, Mike Wittman wrote:
> > > This line should be as close as possible to where we get the time, but
> > > unfortunately that's is in the caller and not proximate to the histogram
> > > recording below.
> > > 
> > > To keep these three things together I'd suggest making the
> > > base::TimeTicks::Now() call at the start of this function, then setting
the
> > > phase.
> > 
> > Just noticed that this comment still remains to be addressed. Please make
this
> > change before submitting.
> 
> If I understand you correctly, this amounts to removing the |ticks| parameter
> and getting it directly at the start of the method, then immediately setting
the
> phase.

Yes, and moving the histogram recording immediately after that.

[bcwhite, 2017-01-11 16:26:39]

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:607:
metrics::CallStackProfileMetricsProvider::MAIN_LOOP_START);
On 2017/01/11 16:24:47, Mike Wittman wrote:
> On 2017/01/11 16:15:48, bcwhite wrote:
> > On 2017/01/10 20:10:36, Mike Wittman wrote:
> > > On 2016/12/01 18:46:50, Mike Wittman wrote:
> > > > This line should be as close as possible to where we get the time, but
> > > > unfortunately that's is in the caller and not proximate to the histogram
> > > > recording below.
> > > > 
> > > > To keep these three things together I'd suggest making the
> > > > base::TimeTicks::Now() call at the start of this function, then setting
> the
> > > > phase.
> > > 
> > > Just noticed that this comment still remains to be addressed. Please make
> this
> > > change before submitting.
> > 
> > If I understand you correctly, this amounts to removing the |ticks|
parameter
> > and getting it directly at the start of the method, then immediately setting
> the
> > phase.
> 
> Yes, and moving the histogram recording immediately after that.

Looking further, every other method also takes a time parameter so I think we
should keep the parameter for consistency sake.  But I'll move the histogram
set.

[Mike Wittman, 2017-01-11 16:30:18]

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:607:
metrics::CallStackProfileMetricsProvider::MAIN_LOOP_START);
On 2017/01/11 16:26:39, bcwhite wrote:
> On 2017/01/11 16:24:47, Mike Wittman wrote:
> > On 2017/01/11 16:15:48, bcwhite wrote:
> > > On 2017/01/10 20:10:36, Mike Wittman wrote:
> > > > On 2016/12/01 18:46:50, Mike Wittman wrote:
> > > > > This line should be as close as possible to where we get the time, but
> > > > > unfortunately that's is in the caller and not proximate to the
histogram
> > > > > recording below.
> > > > > 
> > > > > To keep these three things together I'd suggest making the
> > > > > base::TimeTicks::Now() call at the start of this function, then
setting
> > the
> > > > > phase.
> > > > 
> > > > Just noticed that this comment still remains to be addressed. Please
make
> > this
> > > > change before submitting.
> > > 
> > > If I understand you correctly, this amounts to removing the |ticks|
> parameter
> > > and getting it directly at the start of the method, then immediately
setting
> > the
> > > phase.
> > 
> > Yes, and moving the histogram recording immediately after that.
> 
> Looking further, every other method also takes a time parameter so I think we
> should keep the parameter for consistency sake.  But I'll move the histogram
> set.

Sounds good to me.

[bcwhite, 2017-01-11 16:34:15]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 16:34:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-01-11 16:53:29]

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2530043002/diff/100001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:607:
metrics::CallStackProfileMetricsProvider::MAIN_LOOP_START);
On 2017/01/11 16:30:18, Mike Wittman wrote:
> On 2017/01/11 16:26:39, bcwhite wrote:
> > On 2017/01/11 16:24:47, Mike Wittman wrote:
> > > On 2017/01/11 16:15:48, bcwhite wrote:
> > > > On 2017/01/10 20:10:36, Mike Wittman wrote:
> > > > > On 2016/12/01 18:46:50, Mike Wittman wrote:
> > > > > > This line should be as close as possible to where we get the time,
but
> > > > > > unfortunately that's is in the caller and not proximate to the
> histogram
> > > > > > recording below.
> > > > > > 
> > > > > > To keep these three things together I'd suggest making the
> > > > > > base::TimeTicks::Now() call at the start of this function, then
> setting
> > > the
> > > > > > phase.
> > > > > 
> > > > > Just noticed that this comment still remains to be addressed. Please
> make
> > > this
> > > > > change before submitting.
> > > > 
> > > > If I understand you correctly, this amounts to removing the |ticks|
> > parameter
> > > > and getting it directly at the start of the method, then immediately
> setting
> > > the
> > > > phase.
> > > 
> > > Yes, and moving the histogram recording immediately after that.
> > 
> > Looking further, every other method also takes a time parameter so I think
we
> > should keep the parameter for consistency sake.  But I'll move the histogram
> > set.
> 
> Sounds good to me.

Done (order) and Done (naming).

[commit-bot: I haz the power, 2017-01-11 17:27:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 17:27:57]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2017-01-11 20:32:39]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2017-01-11 20:32:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
wittman@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/2530043002/#ps220001
(title: "move MainLoopStart milestone and histogram to top of utility method")

[bcwhite, 2017-01-11 20:32:41]

The CQ bit was unchecked by bcwhite@chromium.org

[bcwhite, 2017-01-11 20:35:28]

bcwhite@chromium.org changed reviewers:
+ gab@chromium.org, jochen@chromium.org

[bcwhite, 2017-01-11 20:35:29]

gab@chromium.org: Please review changes in 
components/startup_metric_utils/*

jochen@chromium.org: Please review changes in 
content/*
chrome/*

[jochen (gone - plz use gerrit), 2017-01-12 08:35:02]

lgtm

https://codereview.chromium.org/2530043002/diff/220001/content/public/browser...
File content/public/browser/browser_main_parts.h (right):

https://codereview.chromium.org/2530043002/diff/220001/content/public/browser...
content/public/browser/browser_main_parts.h:96: virtual void PreShutdown() {}
nit. can you move that up to before PostDestroyThreads, so the methods are in
the order of expected calls

[gab, 2017-01-12 16:30:05]

gab@chromium.org changed reviewers:
+ fdoray@chromium.org

[gab, 2017-01-12 16:30:06]

Punting to fdoray who's been around that code a lot more than I lately.

[fdoray, 2017-01-12 16:56:56]

https://codereview.chromium.org/2530043002/diff/220001/components/startup_met...
File components/startup_metric_utils/browser/BUILD.gn (right):

https://codereview.chromium.org/2530043002/diff/220001/components/startup_met...
components/startup_metric_utils/browser/BUILD.gn:43:
"//components/metrics:metrics",
not needed

https://codereview.chromium.org/2530043002/diff/220001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (left):

https://codereview.chromium.org/2530043002/diff/220001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:595:
RecordSameVersionStartupCount(pref_service);
RecordSameVersionStartupCount() and RecordHardFaultHistogram(); must be called
before Startup.BrowserMessageLoopStartTime is recorded because they set global
variables used by the
UMA_HISTOGRAM_AND_TRACE_WITH_TEMPERATURE_AND_SAME_VERSION_COUNT macro.

Could add this before RecordSameVersionStartupCount():

// Keep RecordSameVersionStartupCount() and RecordHardFaultHistogram() first
// as many other histograms depend on them setting 
// |g_startups_with_current_version| and |g_startup_temperature|.

[bcwhite, 2017-01-12 17:00:24]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 17:00:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-01-12 17:43:40]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 17:44:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-01-12 17:45:41]

Patchset #8 (id:240001) has been deleted

[bcwhite, 2017-01-12 17:47:03]

https://codereview.chromium.org/2530043002/diff/220001/components/startup_met...
File components/startup_metric_utils/browser/BUILD.gn (right):

https://codereview.chromium.org/2530043002/diff/220001/components/startup_met...
components/startup_metric_utils/browser/BUILD.gn:43:
"//components/metrics:metrics",
On 2017/01/12 16:56:56, fdoray wrote:
> not needed

Done.

https://codereview.chromium.org/2530043002/diff/220001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (left):

https://codereview.chromium.org/2530043002/diff/220001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:595:
RecordSameVersionStartupCount(pref_service);
On 2017/01/12 16:56:56, fdoray wrote:
> RecordSameVersionStartupCount() and RecordHardFaultHistogram(); must be called
> before Startup.BrowserMessageLoopStartTime is recorded because they set global
> variables used by the
> UMA_HISTOGRAM_AND_TRACE_WITH_TEMPERATURE_AND_SAME_VERSION_COUNT macro.
> 
> Could add this before RecordSameVersionStartupCount():
> 
> // Keep RecordSameVersionStartupCount() and RecordHardFaultHistogram() first
> // as many other histograms depend on them setting 
> // |g_startups_with_current_version| and |g_startup_temperature|.

Done.

https://codereview.chromium.org/2530043002/diff/220001/content/public/browser...
File content/public/browser/browser_main_parts.h (right):

https://codereview.chromium.org/2530043002/diff/220001/content/public/browser...
content/public/browser/browser_main_parts.h:96: virtual void PreShutdown() {}
On 2017/01/12 08:35:02, jochen wrote:
> nit. can you move that up to before PostDestroyThreads, so the methods are in
> the order of expected calls

Done.

[fdoray, 2017-01-12 17:59:36]

lgtm w/ comment

https://codereview.chromium.org/2530043002/diff/260001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2530043002/diff/260001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:599: // depend
on it setting |g_startup_temperature|.
on them setting |g_startup_temperature| and |g_startups_with_current_version|

(both are important)

[bcwhite, 2017-01-12 18:05:46]

The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-12 18:06:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bcwhite, 2017-01-12 18:25:25]

https://codereview.chromium.org/2530043002/diff/260001/components/startup_met...
File components/startup_metric_utils/browser/startup_metric_utils.cc (right):

https://codereview.chromium.org/2530043002/diff/260001/components/startup_met...
components/startup_metric_utils/browser/startup_metric_utils.cc:599: // depend
on it setting |g_startup_temperature|.
On 2017/01/12 17:59:36, fdoray wrote:
> on them setting |g_startup_temperature| and |g_startups_with_current_version|
> 
> (both are important)

Done.

[gab, 2017-01-12 19:28:15]

Description was changed from

==========
Set process phases in the StackSamplingProfiler.

BUG=657012
==========

to

==========
Set process phases in the StackSamplingProfiler.

BUG=657012
==========

[gab, 2017-01-12 19:28:15]

gab@chromium.org changed reviewers:
- gab@chromium.org

[commit-bot: I haz the power, 2017-01-12 19:51:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 19:51:07]

Dry run: This issue passed the CQ dry run.

[bcwhite, 2017-01-12 21:04:25]

The CQ bit was checked by bcwhite@chromium.org

[bcwhite, 2017-01-12 21:04:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
wittman@chromium.org, jochen@chromium.org, asvitkine@chromium.org,
fdoray@chromium.org
Link to the patchset: https://codereview.chromium.org/2530043002/#ps280001
(title: "improved comment")

[commit-bot: I haz the power, 2017-01-12 21:04:59]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 21:14:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-12 21:14:29]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[bcwhite, 2017-01-12 21:18:54]

bcwhite@chromium.org changed reviewers:
+ jam@chromium.org

[bcwhite, 2017-01-12 21:18:55]

jam: Please review changes in 

components/metrics/public/*  (minor name change)

[jam, 2017-01-12 22:14:36]

On 2017/01/12 21:18:55, bcwhite wrote:
> jam: Please review changes in 
> 
> components/metrics/public/*  (minor name change)

the presumbit failure is because you need an ipc security reviewer (I'm not one)

[bcwhite, 2017-01-12 22:16:29]

On 2017/01/12 22:14:36, jam wrote:
> On 2017/01/12 21:18:55, bcwhite wrote:
> > jam: Please review changes in 
> > 
> > components/metrics/public/*  (minor name change)
> 
> the presumbit failure is because you need an ipc security reviewer (I'm not
one)

Oops!  I looked at ipc/OWNERS by mistake.

[bcwhite, 2017-01-12 22:17:11]

bcwhite@chromium.org changed reviewers:
+ palmer@chromium.org
- jam@chromium.org

[bcwhite, 2017-01-12 22:17:11]

palmer: Please review changes in 

components/metrics/public/*  (minor name change)

[palmer, 2017-01-12 22:40:58]

lgtm

[bcwhite, 2017-01-12 22:47:55]

The CQ bit was checked by bcwhite@chromium.org

[commit-bot: I haz the power, 2017-01-12 22:48:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 23:55:08]

CQ is committing da patch.

Bot data: {"patchset_id": 280001, "attempt_start_ts": 1484261275104190,
"parent_rev": "b5c9bcd8091d35691e61a939eb2aa8201baf57c8", "commit_rev":
"da097d30d0e99243c59c75d336a761b0ec5e8def"}

[commit-bot: I haz the power, 2017-01-12 23:55:48]

Description was changed from

==========
Set process phases in the StackSamplingProfiler.

BUG=657012
==========

to

==========
Set process phases in the StackSamplingProfiler.

BUG=657012

Review-Url: https://codereview.chromium.org/2530043002
Cr-Commit-Position: refs/heads/master@{#443394}
Committed:
https://chromium.googlesource.com/chromium/src/+/da097d30d0e99243c59c75d336a7...
==========

[commit-bot: I haz the power, 2017-01-12 23:55:51]

Committed patchset #9 (id:280001) as
https://chromium.googlesource.com/chromium/src/+/da097d30d0e99243c59c75d336a7...