Issue 2927593002: Make stack sampling profiler sample beyond startup.

Alexei Svitkine (slow)

Description was changed from ========== Make stack sampling profiler sample beyond startup. BUG=... ========== to ...

3 years, 6 months ago (2017-06-20 20:48:12 UTC) #1

Alexei Svitkine (slow)

Description was changed from ========== Make stack sampling profiler sample beyond startup. This expands the ...

3 years, 6 months ago (2017-06-20 20:48:25 UTC) #2

Alexei Svitkine (slow)

Description was changed from ========== Make stack sampling profiler sample beyond startup. This expands the ...

3 years, 6 months ago (2017-06-20 20:52:00 UTC) #3

Alexei Svitkine (slow)

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

3 years, 6 months ago (2017-06-20 20:52:51 UTC) #7

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-20 20:53:22 UTC) #8

Alexei Svitkine (slow)

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

3 years, 6 months ago (2017-06-20 20:57:38 UTC) #9

Alexei Svitkine (slow)

Hey Mike, This is finally ready-ish for review. There's still some small issues that I ...

3 years, 6 months ago (2017-06-20 20:57:39 UTC) #10

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-20 20:58:03 UTC) #11

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm64_dbg_recipe/builds/292871) android_compile_dbg on ...

3 years, 6 months ago (2017-06-20 20:58:04 UTC) #12

Alexei Svitkine (slow)

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

3 years, 6 months ago (2017-06-20 21:05:27 UTC) #13

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-20 21:06:05 UTC) #14

Alexei Svitkine (slow)

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

3 years, 6 months ago (2017-06-20 21:12:39 UTC) #15

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-20 21:13:33 UTC) #16

commit-bot: I haz the power

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

3 years, 6 months ago (2017-06-20 22:45:24 UTC) #17

commit-bot: I haz the power

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_rel_ng/builds/483734)

3 years, 6 months ago (2017-06-20 22:45:25 UTC) #18

Mike Wittman

On 2017/06/20 20:57:39, Alexei Svitkine (very slow) wrote: > Hey Mike, > > This is ...

3 years, 6 months ago (2017-06-21 14:49:06 UTC) #19

On 2017/06/20 20:57:39, Alexei Svitkine (very slow) wrote:
> Hey Mike,
> 
> This is finally ready-ish for review. There's still some small issues that I
> comment on below and I've not added the start timestamp either (as we
discussed
> in person for being able to control for startup bias).
> 
> But otherwise I believe this should be working and the other parts could
> potentially be added in follow-up CLs or later patchsets on this CL.
> 
> PTAL.

A few high-level comments before getting into details:

Chaining additional collections via the completed callback seems kind of
awkward, but I'm having trouble thinking of an alternative that works within the
constraints of the profiler execution. The inability to assume that the message
loop has started execution and the need to ensure collection termination before
thread exit especially makes this challenging.

This will be fairly wasteful of bandwidth because we'll be repeating largely the
same module information with every profile, plus there will be no coalescing of
stacks across profiles. I'd guess this could take an order of magnitude more
bandwidth per-sample than we're currently using for startup profiling. This is
probably OK for just validating the approach but I think we'll need to address
this before enabling permanently.

This needs some changes to support non-browser processes. In particular, the
logic for configuring the beyond-startup profiling probably needs to live
outside of CallStackProfileMetricsProvider. I think we should make the effort do
this in this CL since it will make the relationships more explicit and the code
cleaner.

https://codereview.chromium.org/2927593002/diff/60001/components/metrics/call...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/2927593002/diff/60001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.cc:344:
profile.sampling_period.InMilliseconds());
On 2017/06/20 20:57:39, Alexei Svitkine (very slow) wrote:
> Note: I need to double check whether sampling period and profile duration are
> set correctly.
> 
> iirc I think profiler duration wasn't being set correctly, but haven't looked
> recently.

The sampling period is being set correctly, but the profile duration is not
currently. It's set to last_sample_time - first_sample_time but should be 
last_sample_time - first_sample_time + 1 * sampling_period to account for the
time period represented by the last sample.

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
File chrome/common/stack_sampling_configuration.h (right):

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
chrome/common/stack_sampling_configuration.h:33:
GetProfilerCallbackForCurrentProcess();
If we have a unified function for getting the callback across processes, then it
will also need to support the GPU process callback that's currently configured
in ChromeContentGpuClient.

Alexei Svitkine (slow)

On 2017/06/21 14:49:06, Mike Wittman wrote: > A few high-level comments before getting into details: ...

3 years, 5 months ago (2017-06-29 20:51:04 UTC) #20

On 2017/06/21 14:49:06, Mike Wittman wrote:
> A few high-level comments before getting into details:
> 
> Chaining additional collections via the completed callback seems kind of
> awkward, but I'm having trouble thinking of an alternative that works within
the
> constraints of the profiler execution. The inability to assume that the
message
> loop has started execution and the need to ensure collection termination
before
> thread exit especially makes this challenging.

One alternative would be to make an interface in base/ that the higher level
could implement - which could provide params, have a function that replaces the
callback, etc. I think this is possibly the cleaner approach but doesn't
fundamentally change the model - and possibly adds more edge cases (i.e. if we
separate the functions for providing data and asking if we should continue
sampling, then now we have to worry about timing thereof - or them being called
at awkward times, etc.). Also, that doesn't (by itself) solve the multi-process
case where things are async - unless we make the interface async itself and that
complicates things more. I think it's better to keep the multi-process async
stuff limited to where it's needed (i.e. so base doesn't need to know about it).

So I'm leaning in keeping it simple for now (with the callback), but to keep in
mind that we may want to switch to the interface approach if we need to add more
functionality later.

Also one option to simplify things somewhat is instead of having both the bool
return and out param, we could instead have the return value be the new params
(with some enum in the params indicating "not to sample" anymore). This would be
a bit less efficient since return value will now be bigger and we'd need to copy
it - but the advantage is semantics are cleaner (i.e. less error prone since the
caller needs to fully provide the new params instead of possibly relying on some
fields already being set.) WDYT?

> 
> This will be fairly wasteful of bandwidth because we'll be repeating largely
the
> same module information with every profile, plus there will be no coalescing
of
> stacks across profiles. I'd guess this could take an order of magnitude more
> bandwidth per-sample than we're currently using for startup profiling. This is
> probably OK for just validating the approach but I think we'll need to address
> this before enabling permanently.

Agreed. Actually meant to add a TODO about that, but forgot before sending.
Added now.

https://codereview.chromium.org/2927593002/diff/60001/components/metrics/call...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/2927593002/diff/60001/components/metrics/call...
components/metrics/call_stack_profile_metrics_provider.cc:344:
profile.sampling_period.InMilliseconds());
On 2017/06/21 14:49:06, Mike Wittman wrote:
> On 2017/06/20 20:57:39, Alexei Svitkine (very slow) wrote:
> > Note: I need to double check whether sampling period and profile duration
are
> > set correctly.
> > 
> > iirc I think profiler duration wasn't being set correctly, but haven't
looked
> > recently.
> 
> The sampling period is being set correctly, but the profile duration is not
> currently. It's set to last_sample_time - first_sample_time but should be 
> last_sample_time - first_sample_time + 1 * sampling_period to account for the
> time period represented by the last sample.

Done.

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
File chrome/common/stack_sampling_configuration.h (right):

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
chrome/common/stack_sampling_configuration.h:33:
GetProfilerCallbackForCurrentProcess();
On 2017/06/21 14:49:06, Mike Wittman wrote:
> If we have a unified function for getting the callback across processes, then
it
> will also need to support the GPU process callback that's currently configured
> in ChromeContentGpuClient.

Did you have some thoughts on how to organize this?

Right now my CL has the browser configuration here. Were you thinking that
GetProfilerCallbackForCurrentProcess() would also work for GPU process - e.g. by
checking process type information from some global? Or were you thinking there
could be separate functions - i.e. ForBrowserProcess() / ForGpuProcess(), etc?

Or something else?

Mike Wittman

On 2017/06/29 20:51:04, Alexei Svitkine (slow) wrote: > On 2017/06/21 14:49:06, Mike Wittman wrote: > ...

3 years, 5 months ago (2017-07-01 02:44:06 UTC) #21

On 2017/06/29 20:51:04, Alexei Svitkine (slow) wrote:
> On 2017/06/21 14:49:06, Mike Wittman wrote:
> > A few high-level comments before getting into details:
> > 
> > Chaining additional collections via the completed callback seems kind of
> > awkward, but I'm having trouble thinking of an alternative that works within
> the
> > constraints of the profiler execution. The inability to assume that the
> message
> > loop has started execution and the need to ensure collection termination
> before
> > thread exit especially makes this challenging.
> 
> One alternative would be to make an interface in base/ that the higher level
> could implement - which could provide params, have a function that replaces
the
> callback, etc. I think this is possibly the cleaner approach but doesn't
> fundamentally change the model - and possibly adds more edge cases (i.e. if we
> separate the functions for providing data and asking if we should continue
> sampling, then now we have to worry about timing thereof - or them being
called
> at awkward times, etc.). Also, that doesn't (by itself) solve the
multi-process
> case where things are async - unless we make the interface async itself and
that
> complicates things more. I think it's better to keep the multi-process async
> stuff limited to where it's needed (i.e. so base doesn't need to know about
it).
> 
> So I'm leaning in keeping it simple for now (with the callback), but to keep
in
> mind that we may want to switch to the interface approach if we need to add
more
> functionality later.
> 
> Also one option to simplify things somewhat is instead of having both the bool
> return and out param, we could instead have the return value be the new params
> (with some enum in the params indicating "not to sample" anymore). This would
be
> a bit less efficient since return value will now be bigger and we'd need to
copy
> it - but the advantage is semantics are cleaner (i.e. less error prone since
the
> caller needs to fully provide the new params instead of possibly relying on
some
> fields already being set.) WDYT?

My ideal approach would be to create a new profiler object for the new
profiling, but I don't see a way to do that that doesn't involve subtle thread
synchronization between the profiler and main threads, and makes things even
more complicated.

So the latter approach sounds pretty good to me. I agree that we should avoid
implementing an interface since that encourages storage of state, which also
makes things more complicated as it's supporting an inherently async operation.

How about returning a base::Optional<SamplingParams> from the callback to
implement this? It still requires at least one copy but I think is more explicit
about what's going on and forces consideration of what state is being reused.

> > This will be fairly wasteful of bandwidth because we'll be repeating largely
> the
> > same module information with every profile, plus there will be no coalescing
> of
> > stacks across profiles. I'd guess this could take an order of magnitude more
> > bandwidth per-sample than we're currently using for startup profiling. This
is
> > probably OK for just validating the approach but I think we'll need to
address
> > this before enabling permanently.
> 
> Agreed. Actually meant to add a TODO about that, but forgot before sending.
> Added now.

Keep in mind also that we issue one symbolization query per profile server side,
so this will drastically increase our QPS and likely push us over quota.

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
File chrome/common/stack_sampling_configuration.h (right):

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
chrome/common/stack_sampling_configuration.h:33:
GetProfilerCallbackForCurrentProcess();
On 2017/06/29 20:51:04, Alexei Svitkine (slow) wrote:
> On 2017/06/21 14:49:06, Mike Wittman wrote:
> > If we have a unified function for getting the callback across processes,
then
> it
> > will also need to support the GPU process callback that's currently
configured
> > in ChromeContentGpuClient.
> 
> Did you have some thoughts on how to organize this?
> 
> Right now my CL has the browser configuration here. Were you thinking that
> GetProfilerCallbackForCurrentProcess() would also work for GPU process - e.g.
by
> checking process type information from some global? Or were you thinking there
> could be separate functions - i.e. ForBrowserProcess() / ForGpuProcess(), etc?
> 
> Or something else?

I'd recommend creating a function here like
base::StackSamplingProfiler::CompletedCallback
ConfigureCallbackForFollowOnSampling(const
base::StackSamplingProfiler::CompletedCallback& callback);

which returns a callback that wraps the passed callback and makes the decision
about whether to continue profiling, returning SamplingParams accordingly.

This can then be invoked by both the browser and GPU process code, each passing
their own specific callback which knows how to move the profiles where they need
to go.

It probably makes sense to move the start_timestamp handling into this wrapper
callback as well.

https://codereview.chromium.org/2927593002/diff/140001/base/profiler/stack_sa...
File base/profiler/stack_sampling_profiler.cc (right):

https://codereview.chromium.org/2927593002/diff/140001/base/profiler/stack_sa...
base/profiler/stack_sampling_profiler.cc:631: collection->next_sample_time =
I'd prefer to create a new CollectionContext and set its state from the current
one, to make clear which state is being reused between collections. You'll
likely need to preserve the collection_id so that the Stop operation continues
to work.

https://codereview.chromium.org/2927593002/diff/140001/components/metrics/cal...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/2927593002/diff/140001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:469: return
base::GetFieldTrialParamByFeatureAsBool(kEnableReporting, "periodic",
Will this function execute properly and return something sensible if invoked
before the metrics service is initialized?

https://codereview.chromium.org/2927593002/diff/140001/components/metrics/cal...
File components/metrics/call_stack_profile_params.h (right):

https://codereview.chromium.org/2927593002/diff/140001/components/metrics/cal...
components/metrics/call_stack_profile_params.h:91: base::TimeTicks
start_timestamp;
Do we need to move this from a callback param to inside CallStackProfileParams
if we use the wrapper callback approach? I'm concerned that this makes it very
easy to miss the extra step required to set this value after construction,
whereas providing as a callback parameter is checked by the compiler.

Alexei Svitkine (slow)

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

3 years, 5 months ago (2017-07-05 20:39:19 UTC) #22

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-05 20:39:42 UTC) #23

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-05 22:40:36 UTC) #24

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_TIMED_OUT, build has not ...

3 years, 5 months ago (2017-07-05 22:40:37 UTC) #25

Alexei Svitkine (slow)

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sampling_configuration.h File chrome/common/stack_sampling_configuration.h (right): https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sampling_configuration.h#newcode33 chrome/common/stack_sampling_configuration.h:33: GetProfilerCallbackForCurrentProcess(); On 2017/07/01 02:44:06, Mike Wittman wrote: > On ...

3 years, 5 months ago (2017-07-06 15:51:09 UTC) #26

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
File chrome/common/stack_sampling_configuration.h (right):

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
chrome/common/stack_sampling_configuration.h:33:
GetProfilerCallbackForCurrentProcess();
On 2017/07/01 02:44:06, Mike Wittman wrote:
> On 2017/06/29 20:51:04, Alexei Svitkine (slow) wrote:
> > On 2017/06/21 14:49:06, Mike Wittman wrote:
> > > If we have a unified function for getting the callback across processes,
> then
> > it
> > > will also need to support the GPU process callback that's currently
> configured
> > > in ChromeContentGpuClient.
> > 
> > Did you have some thoughts on how to organize this?
> > 
> > Right now my CL has the browser configuration here. Were you thinking that
> > GetProfilerCallbackForCurrentProcess() would also work for GPU process -
e.g.
> by
> > checking process type information from some global? Or were you thinking
there
> > could be separate functions - i.e. ForBrowserProcess() / ForGpuProcess(),
etc?
> > 
> > Or something else?
> 
> I'd recommend creating a function here like
> base::StackSamplingProfiler::CompletedCallback
> ConfigureCallbackForFollowOnSampling(const
> base::StackSamplingProfiler::CompletedCallback& callback);
> 
> which returns a callback that wraps the passed callback and makes the decision
> about whether to continue profiling, returning SamplingParams accordingly.
> 
> This can then be invoked by both the browser and GPU process code, each
passing
> their own specific callback which knows how to move the profiles where they
need
> to go.
> 
> It probably makes sense to move the start_timestamp handling into this wrapper
> callback as well.

So if we go with this approach, then we need to change where we check for
whether periodic sampling is enabled:
  1. It doesn't make sense to check at the time of configuration, because that's
too early before field trials are configured by variations service.
  2. So then, if we continue to do it in the callback, we need to decide where
the base::Feature lives that we query. It can't move to chrome/common because
the provider still needs to query it. It can be exposed from the provider, but
then it's kind of weird that this chrome/common code includes the header for the
provider - which otherwise is a browser-side object. It will still work (since
feature state does get synced cross-process), but it will be a bit weird. We
could move it to base, but then it's weird to have a feature in base that is not
queried from base. We could add it to some other place in components/metrics...
WDYT?

https://codereview.chromium.org/2927593002/diff/140001/base/profiler/stack_sa...
File base/profiler/stack_sampling_profiler.cc (right):

https://codereview.chromium.org/2927593002/diff/140001/base/profiler/stack_sa...
base/profiler/stack_sampling_profiler.cc:631: collection->next_sample_time =
On 2017/07/01 02:44:06, Mike Wittman wrote:
> I'd prefer to create a new CollectionContext and set its state from the
current
> one, to make clear which state is being reused between collections. You'll
> likely need to preserve the collection_id so that the Stop operation continues
> to work.

Done.

https://codereview.chromium.org/2927593002/diff/140001/components/metrics/cal...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/2927593002/diff/140001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:469: return
base::GetFieldTrialParamByFeatureAsBool(kEnableReporting, "periodic",
On 2017/07/01 02:44:06, Mike Wittman wrote:
> Will this function execute properly and return something sensible if invoked
> before the metrics service is initialized?

It doesn't care about metrics service being initialized.

However, it does care about VariationsService::CreateTrialsFromSeed having
already been called. Which happens during single-threaded startup shortly after
reading the local state file. That's when base::FeatureList is initialized and
it's forbidden to call into base::FeatureList::IsEnabled() before then (which
the called function does).

I guess the concern is if we get the callback call before that point? It should
be very unlikely ... but I guess not impossible - e.g. if machine got suspended
or something during Chrome startup.

OK, given the above, added a check that feature list has been initialized with a
comment.

Mike Wittman

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sampling_configuration.h File chrome/common/stack_sampling_configuration.h (right): https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sampling_configuration.h#newcode33 chrome/common/stack_sampling_configuration.h:33: GetProfilerCallbackForCurrentProcess(); On 2017/07/06 15:51:09, Alexei Svitkine (slow) wrote: > ...

3 years, 5 months ago (2017-07-06 17:25:53 UTC) #27

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
File chrome/common/stack_sampling_configuration.h (right):

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
chrome/common/stack_sampling_configuration.h:33:
GetProfilerCallbackForCurrentProcess();
On 2017/07/06 15:51:09, Alexei Svitkine (slow) wrote:
> On 2017/07/01 02:44:06, Mike Wittman wrote:
> > On 2017/06/29 20:51:04, Alexei Svitkine (slow) wrote:
> > > On 2017/06/21 14:49:06, Mike Wittman wrote:
> > > > If we have a unified function for getting the callback across processes,
> > then
> > > it
> > > > will also need to support the GPU process callback that's currently
> > configured
> > > > in ChromeContentGpuClient.
> > > 
> > > Did you have some thoughts on how to organize this?
> > > 
> > > Right now my CL has the browser configuration here. Were you thinking that
> > > GetProfilerCallbackForCurrentProcess() would also work for GPU process -
> e.g.
> > by
> > > checking process type information from some global? Or were you thinking
> there
> > > could be separate functions - i.e. ForBrowserProcess() / ForGpuProcess(),
> etc?
> > > 
> > > Or something else?
> > 
> > I'd recommend creating a function here like
> > base::StackSamplingProfiler::CompletedCallback
> > ConfigureCallbackForFollowOnSampling(const
> > base::StackSamplingProfiler::CompletedCallback& callback);
> > 
> > which returns a callback that wraps the passed callback and makes the
decision
> > about whether to continue profiling, returning SamplingParams accordingly.
> > 
> > This can then be invoked by both the browser and GPU process code, each
> passing
> > their own specific callback which knows how to move the profiles where they
> need
> > to go.
> > 
> > It probably makes sense to move the start_timestamp handling into this
wrapper
> > callback as well.
> 
> So if we go with this approach, then we need to change where we check for
> whether periodic sampling is enabled:
>   1. It doesn't make sense to check at the time of configuration, because
that's
> too early before field trials are configured by variations service.
>   2. So then, if we continue to do it in the callback, we need to decide where
> the base::Feature lives that we query. It can't move to chrome/common because
> the provider still needs to query it. It can be exposed from the provider, but
> then it's kind of weird that this chrome/common code includes the header for
the
> provider - which otherwise is a browser-side object. It will still work (since
> feature state does get synced cross-process), but it will be a bit weird. We
> could move it to base, but then it's weird to have a feature in base that is
not
> queried from base. We could add it to some other place in
components/metrics...
> WDYT?

Good point. Perhaps we should move both the wrapper-callback-generator and the
feature into a separate place in components/metrics? This would be consistent
with existing dependencies since both the browser and non-browser sampling
configurations already get their callbacks from components/metrics.

https://codereview.chromium.org/2927593002/diff/140001/components/metrics/cal...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/2927593002/diff/140001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:469: return
base::GetFieldTrialParamByFeatureAsBool(kEnableReporting, "periodic",
On 2017/07/06 15:51:09, Alexei Svitkine (slow) wrote:
> On 2017/07/01 02:44:06, Mike Wittman wrote:
> > Will this function execute properly and return something sensible if invoked
> > before the metrics service is initialized?
> 
> It doesn't care about metrics service being initialized.
> 
> However, it does care about VariationsService::CreateTrialsFromSeed having
> already been called. Which happens during single-threaded startup shortly
after
> reading the local state file. That's when base::FeatureList is initialized and
> it's forbidden to call into base::FeatureList::IsEnabled() before then (which
> the called function does).
> 
> I guess the concern is if we get the callback call before that point? It
should
> be very unlikely ... but I guess not impossible - e.g. if machine got
suspended
> or something during Chrome startup.
> 
> OK, given the above, added a check that feature list has been initialized with
a
> comment.

It should be pretty rare, but I've seen cases where startup hangs for the better
part of the 30 seconds in one function (e.g. due to waiting for files to load
from a network filesystem). So I also would not be surprised if it happens from
time to time.

https://codereview.chromium.org/2927593002/diff/240001/base/profiler/stack_sa...
File base/profiler/stack_sampling_profiler.cc (right):

https://codereview.chromium.org/2927593002/diff/240001/base/profiler/stack_sa...
base/profiler/stack_sampling_profiler.cc:158: const int collection_id;
We probably should rename this to something like "profiler_id" since it's
effectively identifying the requester of the profiling across multiple
collections at this point.

https://codereview.chromium.org/2927593002/diff/240001/base/profiler/stack_sa...
base/profiler/stack_sampling_profiler.cc:181: static StaticAtomicSequenceNumber
next_collection_id_;
nit: drop trailing underscore

https://codereview.chromium.org/2927593002/diff/240001/base/profiler/stack_sa...
base/profiler/stack_sampling_profiler.cc:644: new_collection->next_sample_time =
Can we reuse the logic in AddCollectionTask for restarting the collection
(exclusive of the add events increment)? This code is pretty complex, so the
fewer ways we have of doing something the better.

https://codereview.chromium.org/2927593002/diff/240001/components/metrics/chi...
File components/metrics/child_call_stack_profile_collector.cc (right):

https://codereview.chromium.org/2927593002/diff/240001/components/metrics/chi...
components/metrics/child_call_stack_profile_collector.cc:24: 
nit: remove

Alexei Svitkine (slow)

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sampling_configuration.h File chrome/common/stack_sampling_configuration.h (right): https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sampling_configuration.h#newcode33 chrome/common/stack_sampling_configuration.h:33: GetProfilerCallbackForCurrentProcess(); On 2017/07/06 17:25:52, Mike Wittman wrote: > On ...

3 years, 5 months ago (2017-07-06 19:36:26 UTC) #28

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
File chrome/common/stack_sampling_configuration.h (right):

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
chrome/common/stack_sampling_configuration.h:33:
GetProfilerCallbackForCurrentProcess();
On 2017/07/06 17:25:52, Mike Wittman wrote:
> On 2017/07/06 15:51:09, Alexei Svitkine (slow) wrote:
> > On 2017/07/01 02:44:06, Mike Wittman wrote:
> > > On 2017/06/29 20:51:04, Alexei Svitkine (slow) wrote:
> > > > On 2017/06/21 14:49:06, Mike Wittman wrote:
> > > > > If we have a unified function for getting the callback across
processes,
> > > then
> > > > it
> > > > > will also need to support the GPU process callback that's currently
> > > configured
> > > > > in ChromeContentGpuClient.
> > > > 
> > > > Did you have some thoughts on how to organize this?
> > > > 
> > > > Right now my CL has the browser configuration here. Were you thinking
that
> > > > GetProfilerCallbackForCurrentProcess() would also work for GPU process -
> > e.g.
> > > by
> > > > checking process type information from some global? Or were you thinking
> > there
> > > > could be separate functions - i.e. ForBrowserProcess() /
ForGpuProcess(),
> > etc?
> > > > 
> > > > Or something else?
> > > 
> > > I'd recommend creating a function here like
> > > base::StackSamplingProfiler::CompletedCallback
> > > ConfigureCallbackForFollowOnSampling(const
> > > base::StackSamplingProfiler::CompletedCallback& callback);
> > > 
> > > which returns a callback that wraps the passed callback and makes the
> decision
> > > about whether to continue profiling, returning SamplingParams accordingly.
> > > 
> > > This can then be invoked by both the browser and GPU process code, each
> > passing
> > > their own specific callback which knows how to move the profiles where
they
> > need
> > > to go.
> > > 
> > > It probably makes sense to move the start_timestamp handling into this
> wrapper
> > > callback as well.
> > 
> > So if we go with this approach, then we need to change where we check for
> > whether periodic sampling is enabled:
> >   1. It doesn't make sense to check at the time of configuration, because
> that's
> > too early before field trials are configured by variations service.
> >   2. So then, if we continue to do it in the callback, we need to decide
where
> > the base::Feature lives that we query. It can't move to chrome/common
because
> > the provider still needs to query it. It can be exposed from the provider,
but
> > then it's kind of weird that this chrome/common code includes the header for
> the
> > provider - which otherwise is a browser-side object. It will still work
(since
> > feature state does get synced cross-process), but it will be a bit weird. We
> > could move it to base, but then it's weird to have a feature in base that is
> not
> > queried from base. We could add it to some other place in
> components/metrics...
> > WDYT?
> 
> Good point. Perhaps we should move both the wrapper-callback-generator and the
> feature into a separate place in components/metrics? This would be consistent
> with existing dependencies since both the browser and non-browser sampling
> configurations already get their callbacks from components/metrics.

It gets a bit messy:

- We'll need to also expose kPeriodicSamplingInterval in the new header file.
- As implemented currently, we also need to modify CallStackProfileParams - to
set trigger & timestamp. These are come from the configuration object and are
passed to CallStackProfileMetricsProvider::GetProfilerCallback() right now. We
would need to pass them through as well.

My suggestion is we leave this for a later CL when we want to add support for
periodic sampling for child processes - which requires additional work anyway.
No need to do it for now to get initial coverage of browser process. I can add a
TODO about it.

https://codereview.chromium.org/2927593002/diff/240001/base/profiler/stack_sa...
File base/profiler/stack_sampling_profiler.cc (right):

https://codereview.chromium.org/2927593002/diff/240001/base/profiler/stack_sa...
base/profiler/stack_sampling_profiler.cc:158: const int collection_id;
On 2017/07/06 17:25:52, Mike Wittman wrote:
> We probably should rename this to something like "profiler_id" since it's
> effectively identifying the requester of the profiling across multiple
> collections at this point.

Done.

https://codereview.chromium.org/2927593002/diff/240001/base/profiler/stack_sa...
base/profiler/stack_sampling_profiler.cc:181: static StaticAtomicSequenceNumber
next_collection_id_;
On 2017/07/06 17:25:53, Mike Wittman wrote:
> nit: drop trailing underscore

Done.

https://codereview.chromium.org/2927593002/diff/240001/base/profiler/stack_sa...
base/profiler/stack_sampling_profiler.cc:644: new_collection->next_sample_time =
On 2017/07/06 17:25:52, Mike Wittman wrote:
> Can we reuse the logic in AddCollectionTask for restarting the collection
> (exclusive of the add events increment)? This code is pretty complex, so the
> fewer ways we have of doing something the better.

The way it's structured right now, it re-uses the logic for scheduling the next
sample (i.e. line 651 is shared between schedule to take next sample normally or
when periodic starts). It's pretty straight forward because the two work
basically the same (via next_sample_time).

The functional difference when using AddCollectionTask() is that
|thread_execution_state_add_events_| will be incremented, but I don't think it
makes a difference in this case?

We can't use it outright because it uses active_collections_.insert() which
won't replace an element. And if we made it replace it (e.g. by using
active_collections_[profiler_id] = syntax), it will hide away the invalidation
of |collection|. It will also be a bit less efficient since we'd need to do the
map lookup again.

So my preference is to keep it as-is so that the code is shared with the "next
sample case" here rather than "starting collection task". Another option is to
make a helper function wrapping the PostDelayedTask that both places could use.

https://codereview.chromium.org/2927593002/diff/240001/components/metrics/chi...
File components/metrics/child_call_stack_profile_collector.cc (right):

https://codereview.chromium.org/2927593002/diff/240001/components/metrics/chi...
components/metrics/child_call_stack_profile_collector.cc:24: 
On 2017/07/06 17:25:53, Mike Wittman wrote:
> nit: remove

Done.

Mike Wittman

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sampling_configuration.h File chrome/common/stack_sampling_configuration.h (right): https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sampling_configuration.h#newcode33 chrome/common/stack_sampling_configuration.h:33: GetProfilerCallbackForCurrentProcess(); On 2017/07/06 19:36:25, Alexei Svitkine (slow) wrote: > ...

3 years, 5 months ago (2017-07-06 22:14:02 UTC) #29

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
File chrome/common/stack_sampling_configuration.h (right):

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
chrome/common/stack_sampling_configuration.h:33:
GetProfilerCallbackForCurrentProcess();
On 2017/07/06 19:36:25, Alexei Svitkine (slow) wrote:
> On 2017/07/06 17:25:52, Mike Wittman wrote:
> > On 2017/07/06 15:51:09, Alexei Svitkine (slow) wrote:
> > > On 2017/07/01 02:44:06, Mike Wittman wrote:
> > > > On 2017/06/29 20:51:04, Alexei Svitkine (slow) wrote:
> > > > > On 2017/06/21 14:49:06, Mike Wittman wrote:
> > > > > > If we have a unified function for getting the callback across
> processes,
> > > > then
> > > > > it
> > > > > > will also need to support the GPU process callback that's currently
> > > > configured
> > > > > > in ChromeContentGpuClient.
> > > > > 
> > > > > Did you have some thoughts on how to organize this?
> > > > > 
> > > > > Right now my CL has the browser configuration here. Were you thinking
> that
> > > > > GetProfilerCallbackForCurrentProcess() would also work for GPU process
-
> > > e.g.
> > > > by
> > > > > checking process type information from some global? Or were you
thinking
> > > there
> > > > > could be separate functions - i.e. ForBrowserProcess() /
> ForGpuProcess(),
> > > etc?
> > > > > 
> > > > > Or something else?
> > > > 
> > > > I'd recommend creating a function here like
> > > > base::StackSamplingProfiler::CompletedCallback
> > > > ConfigureCallbackForFollowOnSampling(const
> > > > base::StackSamplingProfiler::CompletedCallback& callback);
> > > > 
> > > > which returns a callback that wraps the passed callback and makes the
> > decision
> > > > about whether to continue profiling, returning SamplingParams
accordingly.
> > > > 
> > > > This can then be invoked by both the browser and GPU process code, each
> > > passing
> > > > their own specific callback which knows how to move the profiles where
> they
> > > need
> > > > to go.
> > > > 
> > > > It probably makes sense to move the start_timestamp handling into this
> > wrapper
> > > > callback as well.
> > > 
> > > So if we go with this approach, then we need to change where we check for
> > > whether periodic sampling is enabled:
> > >   1. It doesn't make sense to check at the time of configuration, because
> > that's
> > > too early before field trials are configured by variations service.
> > >   2. So then, if we continue to do it in the callback, we need to decide
> where
> > > the base::Feature lives that we query. It can't move to chrome/common
> because
> > > the provider still needs to query it. It can be exposed from the provider,
> but
> > > then it's kind of weird that this chrome/common code includes the header
for
> > the
> > > provider - which otherwise is a browser-side object. It will still work
> (since
> > > feature state does get synced cross-process), but it will be a bit weird.
We
> > > could move it to base, but then it's weird to have a feature in base that
is
> > not
> > > queried from base. We could add it to some other place in
> > components/metrics...
> > > WDYT?
> > 
> > Good point. Perhaps we should move both the wrapper-callback-generator and
the
> > feature into a separate place in components/metrics? This would be
consistent
> > with existing dependencies since both the browser and non-browser sampling
> > configurations already get their callbacks from components/metrics.
> 
> It gets a bit messy:
> 
> - We'll need to also expose kPeriodicSamplingInterval in the new header file.
> - As implemented currently, we also need to modify CallStackProfileParams - to
> set trigger & timestamp. These are come from the configuration object and are
> passed to CallStackProfileMetricsProvider::GetProfilerCallback() right now. We
> would need to pass them through as well.
> 
> My suggestion is we leave this for a later CL when we want to add support for
> periodic sampling for child processes - which requires additional work anyway.
> No need to do it for now to get initial coverage of browser process. I can add
a
> TODO about it.

Leaving for a later CL sgtm if it's going to be involved to support non-browser
processes.

In that case I think we can revert the changes to StackSamplingConfiguration,
and let ChromeBrowserMainParts continue to set its own parameters.

https://codereview.chromium.org/2927593002/diff/240001/base/profiler/stack_sa...
File base/profiler/stack_sampling_profiler.cc (right):

https://codereview.chromium.org/2927593002/diff/240001/base/profiler/stack_sa...
base/profiler/stack_sampling_profiler.cc:644: new_collection->next_sample_time =
On 2017/07/06 19:36:25, Alexei Svitkine (slow) wrote:
> On 2017/07/06 17:25:52, Mike Wittman wrote:
> > Can we reuse the logic in AddCollectionTask for restarting the collection
> > (exclusive of the add events increment)? This code is pretty complex, so the
> > fewer ways we have of doing something the better.
> 
> The way it's structured right now, it re-uses the logic for scheduling the
next
> sample (i.e. line 651 is shared between schedule to take next sample normally
or
> when periodic starts). It's pretty straight forward because the two work
> basically the same (via next_sample_time).
> 
> The functional difference when using AddCollectionTask() is that
> |thread_execution_state_add_events_| will be incremented, but I don't think it
> makes a difference in this case?
> 
> We can't use it outright because it uses active_collections_.insert() which
> won't replace an element. And if we made it replace it (e.g. by using
> active_collections_[profiler_id] = syntax), it will hide away the invalidation
> of |collection|. It will also be a bit less efficient since we'd need to do
the
> map lookup again.
> 
> So my preference is to keep it as-is so that the code is shared with the "next
> sample case" here rather than "starting collection task". Another option is to
> make a helper function wrapping the PostDelayedTask that both places could
use.

I was thinking that we could remove the old collection then call
AddCollectionTask() on the new collection. I think that will result in
duplicating code to erase the old collection though (vs. the current
implementation which has duplicated code to handle the initial delay). Neither
is obviously better, so sticking with the current implementation sgtm.

https://codereview.chromium.org/2927593002/diff/260001/components/metrics/cal...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/2927593002/diff/260001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:207:
CallStackProfileParams* params,
We can revert to passing this by const reference again.

https://codereview.chromium.org/2927593002/diff/260001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:213: if
(CallStackProfileMetricsProvider::IsPeriodicSamplingEnabled() &&
If we're not supporting non-browser processes in this CL, then this logic can be
moved to ReceiveCompletedProfiles, which is only invoked by the browser process.

https://codereview.chromium.org/2927593002/diff/260001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:423: return
SampledProfile::PERIODIC_COLLECTION;
This CL seems to be missing the proto change to add PERIODIC_COLLECTION. (And
the corresponding change server-side needs to be approved first as well.)

Alexei Svitkine (slow)

(No changes since before, just discussion for now.) https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sampling_configuration.h File chrome/common/stack_sampling_configuration.h (right): https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sampling_configuration.h#newcode33 chrome/common/stack_sampling_configuration.h:33: GetProfilerCallbackForCurrentProcess(); ...

3 years, 5 months ago (2017-07-07 16:18:49 UTC) #30

(No changes since before, just discussion for now.)

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
File chrome/common/stack_sampling_configuration.h (right):

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
chrome/common/stack_sampling_configuration.h:33:
GetProfilerCallbackForCurrentProcess();
On 2017/07/06 22:14:01, Mike Wittman wrote:
> On 2017/07/06 19:36:25, Alexei Svitkine (slow) wrote:
> > On 2017/07/06 17:25:52, Mike Wittman wrote:
> > > On 2017/07/06 15:51:09, Alexei Svitkine (slow) wrote:
> > > > On 2017/07/01 02:44:06, Mike Wittman wrote:
> > > > > On 2017/06/29 20:51:04, Alexei Svitkine (slow) wrote:
> > > > > > On 2017/06/21 14:49:06, Mike Wittman wrote:
> > > > > > > If we have a unified function for getting the callback across
> > processes,
> > > > > then
> > > > > > it
> > > > > > > will also need to support the GPU process callback that's
currently
> > > > > configured
> > > > > > > in ChromeContentGpuClient.
> > > > > > 
> > > > > > Did you have some thoughts on how to organize this?
> > > > > > 
> > > > > > Right now my CL has the browser configuration here. Were you
thinking
> > that
> > > > > > GetProfilerCallbackForCurrentProcess() would also work for GPU
process
> -
> > > > e.g.
> > > > > by
> > > > > > checking process type information from some global? Or were you
> thinking
> > > > there
> > > > > > could be separate functions - i.e. ForBrowserProcess() /
> > ForGpuProcess(),
> > > > etc?
> > > > > > 
> > > > > > Or something else?
> > > > > 
> > > > > I'd recommend creating a function here like
> > > > > base::StackSamplingProfiler::CompletedCallback
> > > > > ConfigureCallbackForFollowOnSampling(const
> > > > > base::StackSamplingProfiler::CompletedCallback& callback);
> > > > > 
> > > > > which returns a callback that wraps the passed callback and makes the
> > > decision
> > > > > about whether to continue profiling, returning SamplingParams
> accordingly.
> > > > > 
> > > > > This can then be invoked by both the browser and GPU process code,
each
> > > > passing
> > > > > their own specific callback which knows how to move the profiles where
> > they
> > > > need
> > > > > to go.
> > > > > 
> > > > > It probably makes sense to move the start_timestamp handling into this
> > > wrapper
> > > > > callback as well.
> > > > 
> > > > So if we go with this approach, then we need to change where we check
for
> > > > whether periodic sampling is enabled:
> > > >   1. It doesn't make sense to check at the time of configuration,
because
> > > that's
> > > > too early before field trials are configured by variations service.
> > > >   2. So then, if we continue to do it in the callback, we need to decide
> > where
> > > > the base::Feature lives that we query. It can't move to chrome/common
> > because
> > > > the provider still needs to query it. It can be exposed from the
provider,
> > but
> > > > then it's kind of weird that this chrome/common code includes the header
> for
> > > the
> > > > provider - which otherwise is a browser-side object. It will still work
> > (since
> > > > feature state does get synced cross-process), but it will be a bit
weird.
> We
> > > > could move it to base, but then it's weird to have a feature in base
that
> is
> > > not
> > > > queried from base. We could add it to some other place in
> > > components/metrics...
> > > > WDYT?
> > > 
> > > Good point. Perhaps we should move both the wrapper-callback-generator and
> the
> > > feature into a separate place in components/metrics? This would be
> consistent
> > > with existing dependencies since both the browser and non-browser sampling
> > > configurations already get their callbacks from components/metrics.
> > 
> > It gets a bit messy:
> > 
> > - We'll need to also expose kPeriodicSamplingInterval in the new header
file.
> > - As implemented currently, we also need to modify CallStackProfileParams -
to
> > set trigger & timestamp. These are come from the configuration object and
are
> > passed to CallStackProfileMetricsProvider::GetProfilerCallback() right now.
We
> > would need to pass them through as well.
> > 
> > My suggestion is we leave this for a later CL when we want to add support
for
> > periodic sampling for child processes - which requires additional work
anyway.
> > No need to do it for now to get initial coverage of browser process. I can
add
> a
> > TODO about it.
> 
> Leaving for a later CL sgtm if it's going to be involved to support
non-browser
> processes.
> 
> In that case I think we can revert the changes to StackSamplingConfiguration,
> and let ChromeBrowserMainParts continue to set its own parameters.

The changes to StackSamplingConfiguration are needed because we need a place to
store |profile_params_| which are no longer const and get changed by the
callback. The alternative would be for
metrics::CallStackProfileMetricsProvider::GetProfilerCallback(params) to store
the passed-in params somewhere (beyond just binding to the callback) so that
they can be modified.

That would mean introducing more global state in a less centralized location
than StackSamplingConfiguration, which seems not ideal.

I understand it's not great to have browser-specific params live in
StackSamplingConfiguration as is done in this CL, but we can add a TODO comment
about that there and it can be cleaned up in a later CL when we support periodic
sampling in subprocesses too.

https://codereview.chromium.org/2927593002/diff/260001/components/metrics/cal...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/2927593002/diff/260001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:207:
CallStackProfileParams* params,
On 2017/07/06 22:14:02, Mike Wittman wrote:
> We can revert to passing this by const reference again.

No, these are the CallStackProfileParams - which we are modifying (see line
216). SamplingParams are no longer being modified, but they're also no longer
being passed.

https://codereview.chromium.org/2927593002/diff/260001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:213: if
(CallStackProfileMetricsProvider::IsPeriodicSamplingEnabled() &&
On 2017/07/06 22:14:02, Mike Wittman wrote:
> If we're not supporting non-browser processes in this CL, then this logic can
be
> moved to ReceiveCompletedProfiles, which is only invoked by the browser
process.

Er, that's not correct. Browser process uses
CallStackProfileMetricsProvider::GetProfilerCallback() which returns a callback
to this (*Impl) function. It doesn't go through ReceiveCompletedProfiles.

It's the child process mojo-collected ones that go through
ReceiveCompletedProfiles().

https://codereview.chromium.org/2927593002/diff/260001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:423: return
SampledProfile::PERIODIC_COLLECTION;
On 2017/07/06 22:14:02, Mike Wittman wrote:
> This CL seems to be missing the proto change to add PERIODIC_COLLECTION. (And
> the corresponding change server-side needs to be approved first as well.)

PERIODIC_COLLECTION is already in the proto - I guess because CWP had added it.
(The comment doesn't fully match what we're doing here - since it talks about a
random interval - but I think we can just relax the comment a bit - which I can
do in a separate CL so as not to block this one on logs.)

Mike Wittman

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sampling_configuration.h File chrome/common/stack_sampling_configuration.h (right): https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sampling_configuration.h#newcode33 chrome/common/stack_sampling_configuration.h:33: GetProfilerCallbackForCurrentProcess(); On 2017/07/07 16:18:48, Alexei Svitkine (slow) wrote: > ...

3 years, 5 months ago (2017-07-07 18:15:17 UTC) #31

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
File chrome/common/stack_sampling_configuration.h (right):

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
chrome/common/stack_sampling_configuration.h:33:
GetProfilerCallbackForCurrentProcess();
On 2017/07/07 16:18:48, Alexei Svitkine (slow) wrote:
> On 2017/07/06 22:14:01, Mike Wittman wrote:
> > On 2017/07/06 19:36:25, Alexei Svitkine (slow) wrote:
> > > On 2017/07/06 17:25:52, Mike Wittman wrote:
> > > > On 2017/07/06 15:51:09, Alexei Svitkine (slow) wrote:
> > > > > On 2017/07/01 02:44:06, Mike Wittman wrote:
> > > > > > On 2017/06/29 20:51:04, Alexei Svitkine (slow) wrote:
> > > > > > > On 2017/06/21 14:49:06, Mike Wittman wrote:
> > > > > > > > If we have a unified function for getting the callback across
> > > processes,
> > > > > > then
> > > > > > > it
> > > > > > > > will also need to support the GPU process callback that's
> currently
> > > > > > configured
> > > > > > > > in ChromeContentGpuClient.
> > > > > > > 
> > > > > > > Did you have some thoughts on how to organize this?
> > > > > > > 
> > > > > > > Right now my CL has the browser configuration here. Were you
> thinking
> > > that
> > > > > > > GetProfilerCallbackForCurrentProcess() would also work for GPU
> process
> > -
> > > > > e.g.
> > > > > > by
> > > > > > > checking process type information from some global? Or were you
> > thinking
> > > > > there
> > > > > > > could be separate functions - i.e. ForBrowserProcess() /
> > > ForGpuProcess(),
> > > > > etc?
> > > > > > > 
> > > > > > > Or something else?
> > > > > > 
> > > > > > I'd recommend creating a function here like
> > > > > > base::StackSamplingProfiler::CompletedCallback
> > > > > > ConfigureCallbackForFollowOnSampling(const
> > > > > > base::StackSamplingProfiler::CompletedCallback& callback);
> > > > > > 
> > > > > > which returns a callback that wraps the passed callback and makes
the
> > > > decision
> > > > > > about whether to continue profiling, returning SamplingParams
> > accordingly.
> > > > > > 
> > > > > > This can then be invoked by both the browser and GPU process code,
> each
> > > > > passing
> > > > > > their own specific callback which knows how to move the profiles
where
> > > they
> > > > > need
> > > > > > to go.
> > > > > > 
> > > > > > It probably makes sense to move the start_timestamp handling into
this
> > > > wrapper
> > > > > > callback as well.
> > > > > 
> > > > > So if we go with this approach, then we need to change where we check
> for
> > > > > whether periodic sampling is enabled:
> > > > >   1. It doesn't make sense to check at the time of configuration,
> because
> > > > that's
> > > > > too early before field trials are configured by variations service.
> > > > >   2. So then, if we continue to do it in the callback, we need to
decide
> > > where
> > > > > the base::Feature lives that we query. It can't move to chrome/common
> > > because
> > > > > the provider still needs to query it. It can be exposed from the
> provider,
> > > but
> > > > > then it's kind of weird that this chrome/common code includes the
header
> > for
> > > > the
> > > > > provider - which otherwise is a browser-side object. It will still
work
> > > (since
> > > > > feature state does get synced cross-process), but it will be a bit
> weird.
> > We
> > > > > could move it to base, but then it's weird to have a feature in base
> that
> > is
> > > > not
> > > > > queried from base. We could add it to some other place in
> > > > components/metrics...
> > > > > WDYT?
> > > > 
> > > > Good point. Perhaps we should move both the wrapper-callback-generator
and
> > the
> > > > feature into a separate place in components/metrics? This would be
> > consistent
> > > > with existing dependencies since both the browser and non-browser
sampling
> > > > configurations already get their callbacks from components/metrics.
> > > 
> > > It gets a bit messy:
> > > 
> > > - We'll need to also expose kPeriodicSamplingInterval in the new header
> file.
> > > - As implemented currently, we also need to modify CallStackProfileParams
-
> to
> > > set trigger & timestamp. These are come from the configuration object and
> are
> > > passed to CallStackProfileMetricsProvider::GetProfilerCallback() right
now.
> We
> > > would need to pass them through as well.
> > > 
> > > My suggestion is we leave this for a later CL when we want to add support
> for
> > > periodic sampling for child processes - which requires additional work
> anyway.
> > > No need to do it for now to get initial coverage of browser process. I can
> add
> > a
> > > TODO about it.
> > 
> > Leaving for a later CL sgtm if it's going to be involved to support
> non-browser
> > processes.
> > 
> > In that case I think we can revert the changes to
StackSamplingConfiguration,
> > and let ChromeBrowserMainParts continue to set its own parameters.
> 
> The changes to StackSamplingConfiguration are needed because we need a place
to
> store |profile_params_| which are no longer const and get changed by the
> callback. The alternative would be for
> metrics::CallStackProfileMetricsProvider::GetProfilerCallback(params) to store
> the passed-in params somewhere (beyond just binding to the callback) so that
> they can be modified.
> 
> That would mean introducing more global state in a less centralized location
> than StackSamplingConfiguration, which seems not ideal.

How about storing the CallStackProfileParams alongside the StackSamplingProfiler
in ChromeBrowserMainParts? That's the only user of the continuous profiling in
this CL so I think it makes sense that the supporting state should live there.

It sounds like the changes to support non-browser processes will look
considerably different than this code, so having the params here also doesn't
get us much closer to a general solution but does makes it harder to understand
the non-browser process configuration.

https://codereview.chromium.org/2927593002/diff/260001/components/metrics/cal...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/2927593002/diff/260001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:207:
CallStackProfileParams* params,
On 2017/07/07 16:18:48, Alexei Svitkine (slow) wrote:
> On 2017/07/06 22:14:02, Mike Wittman wrote:
> > We can revert to passing this by const reference again.
> 
> No, these are the CallStackProfileParams - which we are modifying (see line
> 216). SamplingParams are no longer being modified, but they're also no longer
> being passed.

That's true. I think we could avoid this modification and the need to store an
instance of the CallStackProfileParams by returning a new callback (that binds
the new CallStackProfileParams) along with the SamplingParams from the callback.
But this would require a recursive callback type and it's not obvious how that
would be represented. We can leave any changes here to the follow-on CL as well.

https://codereview.chromium.org/2927593002/diff/260001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:213: if
(CallStackProfileMetricsProvider::IsPeriodicSamplingEnabled() &&
On 2017/07/07 16:18:48, Alexei Svitkine (slow) wrote:
> On 2017/07/06 22:14:02, Mike Wittman wrote:
> > If we're not supporting non-browser processes in this CL, then this logic
can
> be
> > moved to ReceiveCompletedProfiles, which is only invoked by the browser
> process.
> 
> Er, that's not correct. Browser process uses
> CallStackProfileMetricsProvider::GetProfilerCallback() which returns a
callback
> to this (*Impl) function. It doesn't go through ReceiveCompletedProfiles.
> 
> It's the child process mojo-collected ones that go through
> ReceiveCompletedProfiles().

Right. Apparently I shouldn't be relying on my memory of this code. :)

https://codereview.chromium.org/2927593002/diff/260001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:423: return
SampledProfile::PERIODIC_COLLECTION;
On 2017/07/07 16:18:48, Alexei Svitkine (slow) wrote:
> On 2017/07/06 22:14:02, Mike Wittman wrote:
> > This CL seems to be missing the proto change to add PERIODIC_COLLECTION.
(And
> > the corresponding change server-side needs to be approved first as well.)
> 
> PERIODIC_COLLECTION is already in the proto - I guess because CWP had added
it.
> (The comment doesn't fully match what we're doing here - since it talks about
a
> random interval - but I think we can just relax the comment a bit - which I
can
> do in a separate CL so as not to block this one on logs.)

Ah, didn't consider that this was reusing a CWP enum.

To support this we'll need to update the mechanism for filtering the sampling
profiler data into its own logs source (the IsSamplingProfilerEvent server
function). Currently it's based on trigger value but if we reuse the trigger
then I think we'll need a different mechanism for distinguishing the CWP data
from the sampling profiler data.

Alexei Svitkine (slow)

Patchset #13 (id:300001) has been deleted

3 years, 5 months ago (2017-07-07 19:01:16 UTC) #32

Alexei Svitkine (slow)

Description was changed from ========== Make stack sampling profiler sample beyond startup. This expands the ...

3 years, 5 months ago (2017-07-07 19:04:00 UTC) #33

Alexei Svitkine (slow)

PTAL https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sampling_configuration.h File chrome/common/stack_sampling_configuration.h (right): https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sampling_configuration.h#newcode33 chrome/common/stack_sampling_configuration.h:33: GetProfilerCallbackForCurrentProcess(); On 2017/07/07 18:15:17, Mike Wittman wrote: > ...

3 years, 5 months ago (2017-07-07 19:04:14 UTC) #34

PTAL

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
File chrome/common/stack_sampling_configuration.h (right):

https://codereview.chromium.org/2927593002/diff/100001/chrome/common/stack_sa...
chrome/common/stack_sampling_configuration.h:33:
GetProfilerCallbackForCurrentProcess();
On 2017/07/07 18:15:17, Mike Wittman wrote:
> On 2017/07/07 16:18:48, Alexei Svitkine (slow) wrote:
> > On 2017/07/06 22:14:01, Mike Wittman wrote:
> > > On 2017/07/06 19:36:25, Alexei Svitkine (slow) wrote:
> > > > On 2017/07/06 17:25:52, Mike Wittman wrote:
> > > > > On 2017/07/06 15:51:09, Alexei Svitkine (slow) wrote:
> > > > > > On 2017/07/01 02:44:06, Mike Wittman wrote:
> > > > > > > On 2017/06/29 20:51:04, Alexei Svitkine (slow) wrote:
> > > > > > > > On 2017/06/21 14:49:06, Mike Wittman wrote:
> > > > > > > > > If we have a unified function for getting the callback across
> > > > processes,
> > > > > > > then
> > > > > > > > it
> > > > > > > > > will also need to support the GPU process callback that's
> > currently
> > > > > > > configured
> > > > > > > > > in ChromeContentGpuClient.
> > > > > > > > 
> > > > > > > > Did you have some thoughts on how to organize this?
> > > > > > > > 
> > > > > > > > Right now my CL has the browser configuration here. Were you
> > thinking
> > > > that
> > > > > > > > GetProfilerCallbackForCurrentProcess() would also work for GPU
> > process
> > > -
> > > > > > e.g.
> > > > > > > by
> > > > > > > > checking process type information from some global? Or were you
> > > thinking
> > > > > > there
> > > > > > > > could be separate functions - i.e. ForBrowserProcess() /
> > > > ForGpuProcess(),
> > > > > > etc?
> > > > > > > > 
> > > > > > > > Or something else?
> > > > > > > 
> > > > > > > I'd recommend creating a function here like
> > > > > > > base::StackSamplingProfiler::CompletedCallback
> > > > > > > ConfigureCallbackForFollowOnSampling(const
> > > > > > > base::StackSamplingProfiler::CompletedCallback& callback);
> > > > > > > 
> > > > > > > which returns a callback that wraps the passed callback and makes
> the
> > > > > decision
> > > > > > > about whether to continue profiling, returning SamplingParams
> > > accordingly.
> > > > > > > 
> > > > > > > This can then be invoked by both the browser and GPU process code,
> > each
> > > > > > passing
> > > > > > > their own specific callback which knows how to move the profiles
> where
> > > > they
> > > > > > need
> > > > > > > to go.
> > > > > > > 
> > > > > > > It probably makes sense to move the start_timestamp handling into
> this
> > > > > wrapper
> > > > > > > callback as well.
> > > > > > 
> > > > > > So if we go with this approach, then we need to change where we
check
> > for
> > > > > > whether periodic sampling is enabled:
> > > > > >   1. It doesn't make sense to check at the time of configuration,
> > because
> > > > > that's
> > > > > > too early before field trials are configured by variations service.
> > > > > >   2. So then, if we continue to do it in the callback, we need to
> decide
> > > > where
> > > > > > the base::Feature lives that we query. It can't move to
chrome/common
> > > > because
> > > > > > the provider still needs to query it. It can be exposed from the
> > provider,
> > > > but
> > > > > > then it's kind of weird that this chrome/common code includes the
> header
> > > for
> > > > > the
> > > > > > provider - which otherwise is a browser-side object. It will still
> work
> > > > (since
> > > > > > feature state does get synced cross-process), but it will be a bit
> > weird.
> > > We
> > > > > > could move it to base, but then it's weird to have a feature in base
> > that
> > > is
> > > > > not
> > > > > > queried from base. We could add it to some other place in
> > > > > components/metrics...
> > > > > > WDYT?
> > > > > 
> > > > > Good point. Perhaps we should move both the wrapper-callback-generator
> and
> > > the
> > > > > feature into a separate place in components/metrics? This would be
> > > consistent
> > > > > with existing dependencies since both the browser and non-browser
> sampling
> > > > > configurations already get their callbacks from components/metrics.
> > > > 
> > > > It gets a bit messy:
> > > > 
> > > > - We'll need to also expose kPeriodicSamplingInterval in the new header
> > file.
> > > > - As implemented currently, we also need to modify
CallStackProfileParams
> -
> > to
> > > > set trigger & timestamp. These are come from the configuration object
and
> > are
> > > > passed to CallStackProfileMetricsProvider::GetProfilerCallback() right
> now.
> > We
> > > > would need to pass them through as well.
> > > > 
> > > > My suggestion is we leave this for a later CL when we want to add
support
> > for
> > > > periodic sampling for child processes - which requires additional work
> > anyway.
> > > > No need to do it for now to get initial coverage of browser process. I
can
> > add
> > > a
> > > > TODO about it.
> > > 
> > > Leaving for a later CL sgtm if it's going to be involved to support
> > non-browser
> > > processes.
> > > 
> > > In that case I think we can revert the changes to
> StackSamplingConfiguration,
> > > and let ChromeBrowserMainParts continue to set its own parameters.
> > 
> > The changes to StackSamplingConfiguration are needed because we need a place
> to
> > store |profile_params_| which are no longer const and get changed by the
> > callback. The alternative would be for
> > metrics::CallStackProfileMetricsProvider::GetProfilerCallback(params) to
store
> > the passed-in params somewhere (beyond just binding to the callback) so that
> > they can be modified.
> > 
> > That would mean introducing more global state in a less centralized location
> > than StackSamplingConfiguration, which seems not ideal.
> 
> How about storing the CallStackProfileParams alongside the
StackSamplingProfiler
> in ChromeBrowserMainParts? That's the only user of the continuous profiling in
> this CL so I think it makes sense that the supporting state should live there.
> 
> It sounds like the changes to support non-browser processes will look
> considerably different than this code, so having the params here also doesn't
> get us much closer to a general solution but does makes it harder to
understand
> the non-browser process configuration.

Done. I still think it would be nice to move some of these implementation
details out of ChromeBrowserMain, but that can be done at some point in the
future.

https://codereview.chromium.org/2927593002/diff/260001/components/metrics/cal...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/2927593002/diff/260001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:423: return
SampledProfile::PERIODIC_COLLECTION;
On 2017/07/07 18:15:17, Mike Wittman wrote:
> On 2017/07/07 16:18:48, Alexei Svitkine (slow) wrote:
> > On 2017/07/06 22:14:02, Mike Wittman wrote:
> > > This CL seems to be missing the proto change to add PERIODIC_COLLECTION.
> (And
> > > the corresponding change server-side needs to be approved first as well.)
> > 
> > PERIODIC_COLLECTION is already in the proto - I guess because CWP had added
> it.
> > (The comment doesn't fully match what we're doing here - since it talks
about
> a
> > random interval - but I think we can just relax the comment a bit - which I
> can
> > do in a separate CL so as not to block this one on logs.)
> 
> Ah, didn't consider that this was reusing a CWP enum.
> 
> To support this we'll need to update the mechanism for filtering the sampling
> profiler data into its own logs source (the IsSamplingProfilerEvent server
> function). Currently it's based on trigger value but if we reuse the trigger
> then I think we'll need a different mechanism for distinguishing the CWP data
> from the sampling profiler data.

Thanks for the heads up! Will look at doing that before enabling this new
functionality from the experiment.

Mike Wittman

https://codereview.chromium.org/2927593002/diff/320001/base/profiler/stack_sampling_profiler.cc File base/profiler/stack_sampling_profiler.cc (left): https://codereview.chromium.org/2927593002/diff/320001/base/profiler/stack_sampling_profiler.cc#oldcode500 base/profiler/stack_sampling_profiler.cc:500: size_t count = active_collections_.erase(collection->collection_id); The test code depends on ...

3 years, 5 months ago (2017-07-07 21:01:11 UTC) #35

https://codereview.chromium.org/2927593002/diff/320001/base/profiler/stack_sa...
File base/profiler/stack_sampling_profiler.cc (left):

https://codereview.chromium.org/2927593002/diff/320001/base/profiler/stack_sa...
base/profiler/stack_sampling_profiler.cc:500: size_t count =
active_collections_.erase(collection->collection_id);
The test code depends on the finished event below being signaled _after_ the
collection is erased, to be able to test some aspects of the behavior
non-racily, so we actually need to preserve this ordering in the case where no
further collection takes place.

https://codereview.chromium.org/2927593002/diff/320001/components/metrics/cal...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/2927593002/diff/320001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:507:
sampling_period_ms += kPeriodicSamplingInterval.InMilliseconds();
Sorry, overlooked this change earlier. We actually need to add
1*sampling_interval to the profile_duration when it is set in
StackSamplingProfiler to account for the duration of the last sample (this
should only affect the non-continuous case, actually).

In the continuous case, we'll need to use sampling_period_ms in the proto to
represent the inter-profile interval. This code is pretty much doing that,
except that we can assign directly from
kPeriodicSamplingInterval.InMilliseconds() rather than adding the value.

And also in the continuous case, we probably should use the proto's
profile_duration_ms to represent the time since startup. I suspect we may want
to do time-dependent analyses using that value, either to separately consider
execution from short-running users vs. long-running users, or to consider
execution during specific time periods post-startup. Without it, I suspect the
samples from long-running users will tend to dominate the results and possibly
wash out other interesting effects.

Alexei Svitkine (slow)

Description was changed from ========== Make stack sampling profiler sample beyond startup. This expands the ...

3 years, 5 months ago (2017-07-10 22:04:40 UTC) #36

Alexei Svitkine (slow)

https://codereview.chromium.org/2927593002/diff/320001/base/profiler/stack_sampling_profiler.cc File base/profiler/stack_sampling_profiler.cc (left): https://codereview.chromium.org/2927593002/diff/320001/base/profiler/stack_sampling_profiler.cc#oldcode500 base/profiler/stack_sampling_profiler.cc:500: size_t count = active_collections_.erase(collection->collection_id); On 2017/07/07 21:01:11, Mike Wittman ...

3 years, 5 months ago (2017-07-10 22:05:37 UTC) #37

https://codereview.chromium.org/2927593002/diff/320001/base/profiler/stack_sa...
File base/profiler/stack_sampling_profiler.cc (left):

https://codereview.chromium.org/2927593002/diff/320001/base/profiler/stack_sa...
base/profiler/stack_sampling_profiler.cc:500: size_t count =
active_collections_.erase(collection->collection_id);
On 2017/07/07 21:01:11, Mike Wittman wrote:
> The test code depends on the finished event below being signaled _after_ the
> collection is erased, to be able to test some aspects of the behavior
> non-racily, so we actually need to preserve this ordering in the case where no
> further collection takes place.

Ah, this wasn't obvious. Change to still do it here when there are no new params
and expanded comments, including mentioning the ordering w.r.t. so that tests
aren't flaky.

https://codereview.chromium.org/2927593002/diff/320001/components/metrics/cal...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/2927593002/diff/320001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:507:
sampling_period_ms += kPeriodicSamplingInterval.InMilliseconds();
On 2017/07/07 21:01:11, Mike Wittman wrote:
> Sorry, overlooked this change earlier. We actually need to add
> 1*sampling_interval to the profile_duration when it is set in
> StackSamplingProfiler to account for the duration of the last sample (this
> should only affect the non-continuous case, actually).
> 
> In the continuous case, we'll need to use sampling_period_ms in the proto to
> represent the inter-profile interval. This code is pretty much doing that,
> except that we can assign directly from
> kPeriodicSamplingInterval.InMilliseconds() rather than adding the value.
> 
> And also in the continuous case, we probably should use the proto's
> profile_duration_ms to represent the time since startup. I suspect we may want
> to do time-dependent analyses using that value, either to separately consider
> execution from short-running users vs. long-running users, or to consider
> execution during specific time periods post-startup. Without it, I suspect the
> samples from long-running users will tend to dominate the results and possibly
> wash out other interesting effects.

Done. Moved the logic to CollectProfilesIfCollectionEnabled() now (since we need
uptime at the time of receiving the profile), so this loop body is reverted to
how it was before.

Alexei Svitkine (slow)

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

3 years, 5 months ago (2017-07-10 22:05:45 UTC) #38

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-10 22:06:05 UTC) #39

Mike Wittman

https://codereview.chromium.org/2927593002/diff/320001/base/profiler/stack_sampling_profiler.cc File base/profiler/stack_sampling_profiler.cc (left): https://codereview.chromium.org/2927593002/diff/320001/base/profiler/stack_sampling_profiler.cc#oldcode500 base/profiler/stack_sampling_profiler.cc:500: size_t count = active_collections_.erase(collection->collection_id); On 2017/07/10 22:05:37, Alexei Svitkine ...

3 years, 5 months ago (2017-07-10 22:31:25 UTC) #40

https://codereview.chromium.org/2927593002/diff/320001/base/profiler/stack_sa...
File base/profiler/stack_sampling_profiler.cc (left):

https://codereview.chromium.org/2927593002/diff/320001/base/profiler/stack_sa...
base/profiler/stack_sampling_profiler.cc:500: size_t count =
active_collections_.erase(collection->collection_id);
On 2017/07/10 22:05:37, Alexei Svitkine (slow) wrote:
> On 2017/07/07 21:01:11, Mike Wittman wrote:
> > The test code depends on the finished event below being signaled _after_ the
> > collection is erased, to be able to test some aspects of the behavior
> > non-racily, so we actually need to preserve this ordering in the case where
no
> > further collection takes place.
> 
> Ah, this wasn't obvious. Change to still do it here when there are no new
params
> and expanded comments, including mentioning the ordering w.r.t. so that tests
> aren't flaky.

The callback is also used as a synchronization point for white box testing, so
the the erase should stay before that as well (should have explicitly mentioned
this).

https://codereview.chromium.org/2927593002/diff/360001/components/metrics/cal...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/2927593002/diff/360001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:69:
DCHECK(process_creation_time.is_null());
I think this should be DCHECK(!process_creation_time.is_null()) ?

https://codereview.chromium.org/2927593002/diff/360001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:203:
profiles.profiles[0].profile_duration = GetUptime();
Can we add a test for the PERIODIC_COLLECTION behavior?

https://codereview.chromium.org/2927593002/diff/360001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:206:
profile.profile_duration += profile.sampling_period;
This should go in StackSamplingProfiler; the handling of the profiling duration
there is conceptually incorrect as currently implemented. Lines 492 and 544
should be the relevant places.

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-10 22:39:50 UTC) #41

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/254818)

3 years, 5 months ago (2017-07-10 22:39:51 UTC) #42

Alexei Svitkine (slow)

https://codereview.chromium.org/2927593002/diff/320001/base/profiler/stack_sampling_profiler.cc File base/profiler/stack_sampling_profiler.cc (left): https://codereview.chromium.org/2927593002/diff/320001/base/profiler/stack_sampling_profiler.cc#oldcode500 base/profiler/stack_sampling_profiler.cc:500: size_t count = active_collections_.erase(collection->collection_id); On 2017/07/10 22:31:25, Mike Wittman ...

3 years, 5 months ago (2017-07-11 18:27:05 UTC) #43

https://codereview.chromium.org/2927593002/diff/320001/base/profiler/stack_sa...
File base/profiler/stack_sampling_profiler.cc (left):

https://codereview.chromium.org/2927593002/diff/320001/base/profiler/stack_sa...
base/profiler/stack_sampling_profiler.cc:500: size_t count =
active_collections_.erase(collection->collection_id);
On 2017/07/10 22:31:25, Mike Wittman wrote:
> On 2017/07/10 22:05:37, Alexei Svitkine (slow) wrote:
> > On 2017/07/07 21:01:11, Mike Wittman wrote:
> > > The test code depends on the finished event below being signaled _after_
the
> > > collection is erased, to be able to test some aspects of the behavior
> > > non-racily, so we actually need to preserve this ordering in the case
where
> no
> > > further collection takes place.
> > 
> > Ah, this wasn't obvious. Change to still do it here when there are no new
> params
> > and expanded comments, including mentioning the ordering w.r.t. so that
tests
> > aren't flaky.
> 
> The callback is also used as a synchronization point for white box testing, so
> the the erase should stay before that as well (should have explicitly
mentioned
> this).

I see. So this required reorganizing things a bit - because then we have to
erase it before knowing the return value of the callback. So, instead I moved
this to the caller (as we need to keep collection alive to create the new
collection from its params.) With this approach, I've now re-used
AddCollectionTask() per your previous suggestion.

https://codereview.chromium.org/2927593002/diff/360001/components/metrics/cal...
File components/metrics/call_stack_profile_metrics_provider.cc (right):

https://codereview.chromium.org/2927593002/diff/360001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:69:
DCHECK(process_creation_time.is_null());
On 2017/07/10 22:31:25, Mike Wittman wrote:
> I think this should be DCHECK(!process_creation_time.is_null()) ?

Done.

https://codereview.chromium.org/2927593002/diff/360001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:203:
profiles.profiles[0].profile_duration = GetUptime();
On 2017/07/10 22:31:25, Mike Wittman wrote:
> Can we add a test for the PERIODIC_COLLECTION behavior?

Will take a look at adding. Haven't done so yet.

https://codereview.chromium.org/2927593002/diff/360001/components/metrics/cal...
components/metrics/call_stack_profile_metrics_provider.cc:206:
profile.profile_duration += profile.sampling_period;
On 2017/07/10 22:31:25, Mike Wittman wrote:
> This should go in StackSamplingProfiler; the handling of the profiling
duration
> there is conceptually incorrect as currently implemented. Lines 492 and 544
> should be the relevant places.

Done.

Alexei Svitkine (slow)

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

3 years, 5 months ago (2017-07-11 18:36:18 UTC) #44

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-11 18:36:39 UTC) #45

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-11 19:10:25 UTC) #46

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/255670)

3 years, 5 months ago (2017-07-11 19:10:26 UTC) #47

Mike Wittman

This looks good apart from the metrics provider test. https://codereview.chromium.org/2927593002/diff/400001/base/profiler/stack_sampling_profiler.cc File base/profiler/stack_sampling_profiler.cc (right): https://codereview.chromium.org/2927593002/diff/400001/base/profiler/stack_sampling_profiler.cc#newcode486 base/profiler/stack_sampling_profiler.cc:486: ...

3 years, 5 months ago (2017-07-11 21:41:52 UTC) #48

Mike Wittman

One other thought: should we stop collecting stacks after some time period so that we're ...

3 years, 5 months ago (2017-07-12 16:09:42 UTC) #49

Alexei Svitkine (slow)

I agree we may want something like this in the future. But I think in ...

3 years, 5 months ago (2017-07-12 17:17:03 UTC) #50

Alexei Svitkine (slow)

PTAL. Test added. I moved the GetUptime() helper to an internal namespace exposed from .h ...

3 years, 5 months ago (2017-07-12 20:06:21 UTC) #51

Alexei Svitkine (slow)

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

3 years, 5 months ago (2017-07-12 20:09:16 UTC) #52

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-12 20:09:42 UTC) #53

Mike Wittman

On 2017/07/12 20:06:21, Alexei Svitkine (slow) wrote: > PTAL. > > Test added. I moved ...

3 years, 5 months ago (2017-07-12 20:16:52 UTC) #54

Alexei Svitkine (slow)

Description was changed from ========== Make stack sampling profiler sample beyond startup. This expands the ...

3 years, 5 months ago (2017-07-12 20:59:12 UTC) #55

Alexei Svitkine (slow)

asvitkine@chromium.org changed reviewers: + sky@chromium.org

3 years, 5 months ago (2017-07-12 20:59:43 UTC) #56

Alexei Svitkine (slow)

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

3 years, 5 months ago (2017-07-12 20:59:49 UTC) #58

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-12 21:00:14 UTC) #59

Alexei Svitkine (slow)

asvitkine@chromium.org changed reviewers: + rsesek@chromium.org

3 years, 5 months ago (2017-07-12 21:01:27 UTC) #60

Alexei Svitkine (slow)

+rsesek from ipc/SECURITY_OWNERS for components/metrics/public/*

3 years, 5 months ago (2017-07-12 21:01:28 UTC) #61

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-12 21:10:25 UTC) #63

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-12 21:10:28 UTC) #64

sky

https://codereview.chromium.org/2927593002/diff/460001/chrome/browser/chrome_browser_main.cc File chrome/browser/chrome_browser_main.cc (right): https://codereview.chromium.org/2927593002/diff/460001/chrome/browser/chrome_browser_main.cc#newcode643 chrome/browser/chrome_browser_main.cc:643: &sampling_profiler_params_)), It's rather error prone to have callers be ...

3 years, 5 months ago (2017-07-12 21:42:58 UTC) #65

Alexei Svitkine (slow)

Moved the params to call_stack_profile_metrics_provider.cc and made some things constexpr to avoid a static initializer. ...

3 years, 5 months ago (2017-07-13 17:58:22 UTC) #66

Mike Wittman

https://codereview.chromium.org/2927593002/diff/480001/components/metrics/call_stack_profile_metrics_provider.h File components/metrics/call_stack_profile_metrics_provider.h (right): https://codereview.chromium.org/2927593002/diff/480001/components/metrics/call_stack_profile_metrics_provider.h#newcode53 components/metrics/call_stack_profile_metrics_provider.h:53: static base::StackSamplingProfiler::CompletedCallback GetProfilerCallback( This can be made private or ...

3 years, 5 months ago (2017-07-13 18:10:29 UTC) #67

Alexei Svitkine (slow)

https://codereview.chromium.org/2927593002/diff/480001/components/metrics/call_stack_profile_metrics_provider.h File components/metrics/call_stack_profile_metrics_provider.h (right): https://codereview.chromium.org/2927593002/diff/480001/components/metrics/call_stack_profile_metrics_provider.h#newcode53 components/metrics/call_stack_profile_metrics_provider.h:53: static base::StackSamplingProfiler::CompletedCallback GetProfilerCallback( On 2017/07/13 18:10:29, Mike Wittman wrote: ...

3 years, 5 months ago (2017-07-13 18:36:15 UTC) #68

Alexei Svitkine (slow)

asvitkine@chromium.org changed reviewers: + thakis@chromium.org

3 years, 5 months ago (2017-07-13 18:37:44 UTC) #69

Alexei Svitkine (slow)

+thakis for base/ OWNERS (constexpr additions to time.h)

3 years, 5 months ago (2017-07-13 18:37:48 UTC) #70

Alexei Svitkine (slow)

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

3 years, 5 months ago (2017-07-13 20:37:15 UTC) #73

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-13 20:37:50 UTC) #74

sky

chrome/browser LGTM (thanks for moving ownership to the actual object).

3 years, 5 months ago (2017-07-13 20:38:55 UTC) #75

Alexei Svitkine (slow)

The CQ bit was unchecked by asvitkine@chromium.org

3 years, 5 months ago (2017-07-13 20:40:26 UTC) #76

Alexei Svitkine (slow)

The CQ bit was checked by asvitkine@chromium.org

3 years, 5 months ago (2017-07-13 20:40:33 UTC) #77

Alexei Svitkine (slow)

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org Link to the ...

3 years, 5 months ago (2017-07-13 20:40:34 UTC) #78

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-13 20:40:45 UTC) #79

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-13 21:53:02 UTC) #80

commit-bot: I haz the power

Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/258086)

3 years, 5 months ago (2017-07-13 21:53:08 UTC) #81

Alexei Svitkine (slow)

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

3 years, 5 months ago (2017-07-14 14:51:41 UTC) #82

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-14 14:51:50 UTC) #83

Alexei Svitkine (slow)

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

3 years, 5 months ago (2017-07-14 15:20:03 UTC) #84

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-14 15:20:11 UTC) #85

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-14 16:48:53 UTC) #86

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/500587)

3 years, 5 months ago (2017-07-14 16:48:54 UTC) #87

Alexei Svitkine (slow)

Hmm, StackSamplingProfilerTest.RescheduledByCallback (the new test I'm adding) fails on mac_chromium_rel_ng - but it passes for ...

3 years, 5 months ago (2017-07-14 17:05:02 UTC) #88

Robert Sesek

On 2017/07/14 17:05:02, Alexei Svitkine (slow) wrote: > Hmm, StackSamplingProfilerTest.RescheduledByCallback (the new test I'm adding) ...

3 years, 5 months ago (2017-07-14 17:07:54 UTC) #89

Alexei Svitkine (slow)

Ah, awesome! Thanks for that tidbit, looks like in this case: dcheck_always_on = true ffmpeg_branding ...

3 years, 5 months ago (2017-07-14 17:14:22 UTC) #90

Alexei Svitkine (slow)

And, unfortunately, still can't repro locally. I guess I'll do some try bot debugging with ...

3 years, 5 months ago (2017-07-14 17:31:03 UTC) #91

Robert Sesek

On 2017/07/14 17:31:03, Alexei Svitkine (slow) wrote: > And, unfortunately, still can't repro locally. I ...

3 years, 5 months ago (2017-07-14 17:59:07 UTC) #92

Alexei Svitkine (slow)

So logs from the bot indicates the second WaitableEvent::Signal() call doesn't cause the test's sampling_completed.Wait() ...

3 years, 5 months ago (2017-07-14 19:13:00 UTC) #93

So logs from the bot indicates the second WaitableEvent::Signal() call
doesn't cause the test's sampling_completed.Wait() to wake.

Wonder if WaitableEvent has limitations on 10.9 we're running into. Or a
bug?

Test output from the bot:

[42118:771:0714/112932.095861:8223312726831:ERROR:stack_sampling_profiler_unittest.cc(1057)]
RescheduledByCallback start
[42118:771:0714/112932.096290:8223313151913:ERROR:stack_sampling_profiler_unittest.cc(1072)]
RescheduledByCallback call start
[42118:771:0714/112932.096542:8223313402665:ERROR:stack_sampling_profiler_unittest.cc(1074)]
RescheduledByCallback call wait1
[42118:13059:0714/112932.099682:8223316545059:ERROR:stack_sampling_profiler.cc(637)]
Collection finished
[42118:13059:0714/112932.099737:8223316596038:ERROR:stack_sampling_profiler.cc(487)]
FinishCollection call
[42118:13059:0714/112932.099757:8223316615920:ERROR:stack_sampling_profiler_unittest.cc(352)]
SaveProfilesAndReschedule
[42118:13059:0714/112932.099786:8223316644497:ERROR:stack_sampling_profiler_unittest.cc(370)]
Returning new params
[42118:13059:0714/112932.099802:8223316660277:ERROR:stack_sampling_profiler.cc(508)]
FinishCollection before signal, with new_params=1
[42118:13059:0714/112932.099825:8223316683555:ERROR:stack_sampling_profiler.cc(655)]
Scheduling new collection
[42118:13059:0714/112932.203425:8223420292016:ERROR:stack_sampling_profiler.cc(637)]
Collection finished
[42118:13059:0714/112932.203501:8223420363900:ERROR:stack_sampling_profiler.cc(487)]
FinishCollection call
[42118:13059:0714/112932.203537:8223420399079:ERROR:stack_sampling_profiler_unittest.cc(352)]
SaveProfilesAndReschedule
[42118:13059:0714/112932.203589:8223420451551:ERROR:stack_sampling_profiler_unittest.cc(359)]
Returning empty params
[42118:13059:0714/112932.203623:8223420484978:ERROR:stack_sampling_profiler.cc(508)]
FinishCollection before signal, with new_params=0

So we run all the code, including the second callback that doesn't
reschedule things, and we call Signal() from the profiler object, but the
test code that's waiting for it is never woken up...

On Fri, Jul 14, 2017 at 1:59 PM, <rsesek@chromium.org> wrote:

> On 2017/07/14 17:31:03, Alexei Svitkine (slow) wrote:
> > And, unfortunately, still can't repro locally. I guess I'll do some try
> bot
> > debugging with LOG messages. :/
>
> Looks like base_unittests ran on 10.9. Maybe an OS-specific issue? You can
> grab
> VMs from go/mac-litterbox <https://goto.google.com/mac-litterbox>.
>
> https://codereview.chromium.org/2927593002/
>

-- 
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)

Looks like I wasn't calling Reset() on the WaitableEvent which is required with the params ...

3 years, 5 months ago (2017-07-14 19:18:36 UTC) #94

Looks like I wasn't calling Reset() on the WaitableEvent which is required
with the params we were using. It's surprising that it worked on other
platforms without it, though.

On Fri, Jul 14, 2017 at 3:12 PM, Alexei Svitkine <asvitkine@chromium.org>
wrote:

> So logs from the bot indicates the second WaitableEvent::Signal() call
> doesn't cause the test's sampling_completed.Wait() to wake.
>
> Wonder if WaitableEvent has limitations on 10.9 we're running into. Or a
> bug?
>
> Test output from the bot:
>
> [42118:771:0714/112932.095861:8223312726831:ERROR:stack_
> sampling_profiler_unittest.cc(1057)] RescheduledByCallback start
> [42118:771:0714/112932.096290:8223313151913:ERROR:stack_
> sampling_profiler_unittest.cc(1072)] RescheduledByCallback call start
> [42118:771:0714/112932.096542:8223313402665:ERROR:stack_
> sampling_profiler_unittest.cc(1074)] RescheduledByCallback call wait1
> [42118:13059:0714/112932.099682:8223316545059:ERROR:
> stack_sampling_profiler.cc(637)] Collection finished
> [42118:13059:0714/112932.099737:8223316596038:ERROR:
> stack_sampling_profiler.cc(487)] FinishCollection call
> [42118:13059:0714/112932.099757:8223316615920:ERROR:
> stack_sampling_profiler_unittest.cc(352)] SaveProfilesAndReschedule
> [42118:13059:0714/112932.099786:8223316644497:ERROR:
> stack_sampling_profiler_unittest.cc(370)] Returning new params
> [42118:13059:0714/112932.099802:8223316660277:ERROR:
> stack_sampling_profiler.cc(508)] FinishCollection before signal, with
> new_params=1
> [42118:13059:0714/112932.099825:8223316683555:ERROR:
> stack_sampling_profiler.cc(655)] Scheduling new collection
> [42118:13059:0714/112932.203425:8223420292016:ERROR:
> stack_sampling_profiler.cc(637)] Collection finished
> [42118:13059:0714/112932.203501:8223420363900:ERROR:
> stack_sampling_profiler.cc(487)] FinishCollection call
> [42118:13059:0714/112932.203537:8223420399079:ERROR:
> stack_sampling_profiler_unittest.cc(352)] SaveProfilesAndReschedule
> [42118:13059:0714/112932.203589:8223420451551:ERROR:
> stack_sampling_profiler_unittest.cc(359)] Returning empty params
> [42118:13059:0714/112932.203623:8223420484978:ERROR:
> stack_sampling_profiler.cc(508)] FinishCollection before signal, with
> new_params=0
>
> So we run all the code, including the second callback that doesn't
> reschedule things, and we call Signal() from the profiler object, but the
> test code that's waiting for it is never woken up...
>
> On Fri, Jul 14, 2017 at 1:59 PM, <rsesek@chromium.org> wrote:
>
>> On 2017/07/14 17:31:03, Alexei Svitkine (slow) wrote:
>> > And, unfortunately, still can't repro locally. I guess I'll do some try
>> bot
>> > debugging with LOG messages. :/
>>
>> Looks like base_unittests ran on 10.9. Maybe an OS-specific issue? You
>> can grab
>> VMs from go/mac-litterbox <https://goto.google.com/mac-litterbox>.
>>
>> https://codereview.chromium.org/2927593002/
>>
>
>

-- 
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)

Nevermind, we are calling Reset() on it ... from SaveProfilesAndReschedule(). Hmm. On Fri, Jul 14, ...

3 years, 5 months ago (2017-07-14 19:20:51 UTC) #95

Nevermind, we are calling Reset() on it ...
from SaveProfilesAndReschedule(). Hmm.

On Fri, Jul 14, 2017 at 3:17 PM, Alexei Svitkine <asvitkine@chromium.org>
wrote:

> Looks like I wasn't calling Reset() on the WaitableEvent which is required
> with the params we were using. It's surprising that it worked on other
> platforms without it, though.
>
> On Fri, Jul 14, 2017 at 3:12 PM, Alexei Svitkine <asvitkine@chromium.org>
> wrote:
>
>> So logs from the bot indicates the second WaitableEvent::Signal() call
>> doesn't cause the test's sampling_completed.Wait() to wake.
>>
>> Wonder if WaitableEvent has limitations on 10.9 we're running into. Or a
>> bug?
>>
>> Test output from the bot:
>>
>> [42118:771:0714/112932.095861:8223312726831:ERROR:stack_samp
>> ling_profiler_unittest.cc(1057)] RescheduledByCallback start
>> [42118:771:0714/112932.096290:8223313151913:ERROR:stack_samp
>> ling_profiler_unittest.cc(1072)] RescheduledByCallback call start
>> [42118:771:0714/112932.096542:8223313402665:ERROR:stack_samp
>> ling_profiler_unittest.cc(1074)] RescheduledByCallback call wait1
>>
[42118:13059:0714/112932.099682:8223316545059:ERROR:stack_sampling_profiler.cc(637)]
>> Collection finished
>>
[42118:13059:0714/112932.099737:8223316596038:ERROR:stack_sampling_profiler.cc(487)]
>> FinishCollection call
>> [42118:13059:0714/112932.099757:8223316615920:ERROR:stack_
>> sampling_profiler_unittest.cc(352)] SaveProfilesAndReschedule
>> [42118:13059:0714/112932.099786:8223316644497:ERROR:stack_
>> sampling_profiler_unittest.cc(370)] Returning new params
>>
[42118:13059:0714/112932.099802:8223316660277:ERROR:stack_sampling_profiler.cc(508)]
>> FinishCollection before signal, with new_params=1
>>
[42118:13059:0714/112932.099825:8223316683555:ERROR:stack_sampling_profiler.cc(655)]
>> Scheduling new collection
>>
[42118:13059:0714/112932.203425:8223420292016:ERROR:stack_sampling_profiler.cc(637)]
>> Collection finished
>>
[42118:13059:0714/112932.203501:8223420363900:ERROR:stack_sampling_profiler.cc(487)]
>> FinishCollection call
>> [42118:13059:0714/112932.203537:8223420399079:ERROR:stack_
>> sampling_profiler_unittest.cc(352)] SaveProfilesAndReschedule
>> [42118:13059:0714/112932.203589:8223420451551:ERROR:stack_
>> sampling_profiler_unittest.cc(359)] Returning empty params
>>
[42118:13059:0714/112932.203623:8223420484978:ERROR:stack_sampling_profiler.cc(508)]
>> FinishCollection before signal, with new_params=0
>>
>> So we run all the code, including the second callback that doesn't
>> reschedule things, and we call Signal() from the profiler object, but the
>> test code that's waiting for it is never woken up...
>>
>> On Fri, Jul 14, 2017 at 1:59 PM, <rsesek@chromium.org> wrote:
>>
>>> On 2017/07/14 17:31:03, Alexei Svitkine (slow) wrote:
>>> > And, unfortunately, still can't repro locally. I guess I'll do some
>>> try bot
>>> > debugging with LOG messages. :/
>>>
>>> Looks like base_unittests ran on 10.9. Maybe an OS-specific issue? You
>>> can grab
>>> VMs from go/mac-litterbox <https://goto.google.com/mac-litterbox>.
>>>
>>> https://codereview.chromium.org/2927593002/
>>>
>>
>>
>

-- 
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.

Robert Sesek

https://codereview.chromium.org/2927593002/diff/560001/base/profiler/stack_sampling_profiler_unittest.cc File base/profiler/stack_sampling_profiler_unittest.cc (right): https://codereview.chromium.org/2927593002/diff/560001/base/profiler/stack_sampling_profiler_unittest.cc#newcode356 base/profiler/stack_sampling_profiler_unittest.cc:356: event->Reset(); I've just rewritten WaitableEvent on Mac and it's ...

3 years, 5 months ago (2017-07-14 19:22:39 UTC) #96

Alexei Svitkine (slow)

On 2017/07/14 19:22:39, Robert Sesek wrote: > https://codereview.chromium.org/2927593002/diff/560001/base/profiler/stack_sampling_profiler_unittest.cc > File base/profiler/stack_sampling_profiler_unittest.cc (right): > > https://codereview.chromium.org/2927593002/diff/560001/base/profiler/stack_sampling_profiler_unittest.cc#newcode356 ...

3 years, 5 months ago (2017-07-14 19:27:38 UTC) #97

Robert Sesek

On 2017/07/14 19:27:38, Alexei Svitkine (slow) wrote: > On 2017/07/14 19:22:39, Robert Sesek wrote: > ...

3 years, 5 months ago (2017-07-14 19:36:05 UTC) #98

Alexei Svitkine (slow)

It's pretty unfortunate that it only repros on a specific platform (10.9) - for which ...

3 years, 5 months ago (2017-07-14 19:40:03 UTC) #99

Robert Sesek

On 2017/07/14 19:40:03, Alexei Svitkine (slow) wrote: > It's pretty unfortunate that it only repros ...

3 years, 5 months ago (2017-07-14 19:46:02 UTC) #100

Alexei Svitkine (slow)

Okay, maybe file a crbug so that you don't forget. Thanks for looking into it! ...

3 years, 5 months ago (2017-07-14 19:50:12 UTC) #101

Alexei Svitkine (slow)

The CQ bit was checked by asvitkine@chromium.org

3 years, 5 months ago (2017-07-14 20:20:32 UTC) #102

Alexei Svitkine (slow)

The patchset sent to the CQ was uploaded after l-g-t-m from wittman@chromium.org, rsesek@chromium.org, thakis@chromium.org, sky@chromium.org ...

3 years, 5 months ago (2017-07-14 20:20:33 UTC) #103

commit-bot: I haz the power

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

3 years, 5 months ago (2017-07-14 20:20:43 UTC) #104

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 600001, "attempt_start_ts": 1500063632687390, "parent_rev": "191d11489cb9f83e1e1575a29abf3904e7efa87b", "commit_rev": "2d8b08c72cac89549268a67996c387323895ce22"}

3 years, 5 months ago (2017-07-14 22:16:03 UTC) #105

commit-bot: I haz the power

Description was changed from ========== Make stack sampling profiler sample beyond startup. This expands the ...

3 years, 5 months ago (2017-07-14 22:16:24 UTC) #106

Message was sent while issue was closed.

Description was changed from

==========
Make stack sampling profiler sample beyond startup.

This expands the base sampling profiler API to make the client callback to be
able
to return new sampling params if another profiling collection should be started.

Additionally, converts sampling profiler reporting to use a base::Feature rather
than a field trial directly and adds a base::Feature param that specifies
whether
this new functionality should be used to do periodic sampling (at 1s intervals).
This
new functionality is not on by default, but can be enabled via a field trial.

The new periodic samples will be added to UMA logs as they're received and
will have PERIODIC_COLLECTION trigger specified in the proto.

Adds a test for the new base profiler functionality.

Also, updates profile_duration for non-periodic profiles to include the
sampling_period, as that was previously not accounted for.

Also fixes some existing lint warnings.

BUG=735182
==========

to

==========
Make stack sampling profiler sample beyond startup.

This expands the base sampling profiler API to make the client callback to be
able
to return new sampling params if another profiling collection should be started.

Additionally, converts sampling profiler reporting to use a base::Feature rather
than a field trial directly and adds a base::Feature param that specifies
whether
this new functionality should be used to do periodic sampling (at 1s intervals).
This
new functionality is not on by default, but can be enabled via a field trial.

The new periodic samples will be added to UMA logs as they're received and
will have PERIODIC_COLLECTION trigger specified in the proto.

Adds a test for the new base profiler functionality.

Also, updates profile_duration for non-periodic profiles to include the
sampling_period, as that was previously not accounted for.

Also fixes some existing lint warnings.

BUG=735182

Review-Url: https://codereview.chromium.org/2927593002
Cr-Commit-Position: refs/heads/master@{#486906}
Committed:
https://chromium.googlesource.com/chromium/src/+/2d8b08c72cac89549268a67996c3...
==========

commit-bot: I haz the power

3 years, 5 months ago (2017-07-14 22:16:27 UTC) #107

Message was sent while issue was closed.

Committed patchset #27 (id:600001) as
https://chromium.googlesource.com/chromium/src/+/2d8b08c72cac89549268a67996c3...

Issue 2927593002: Make stack sampling profiler sample beyond startup. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Fix gn check also revert local change from testing. #

Patch Set 3 : rebase + PROFILER_TEST_F #

Patch Set 4 : rebase #

Patch Set 5 : Address some comments. #

Patch Set 6 : rebase #

Patch Set 7 : fix compile #

Patch Set 8 : Use base::Optional for callback return value. #

Patch Set 9 : Address comments. #

Patch Set 10 : Address comments. #

Patch Set 11 : more comments #

Patch Set 12 : rebase #

Patch Set 13 : Add TODO. Move sampling_profiler_params_ to chrome_browser_main. #

Patch Set 14 : rebase #

Patch Set 15 : Address comments. #

Patch Set 16 : Address more comments. #

Patch Set 17 : Address more comments. #

Patch Set 18 : Add test + some other comments. #

Patch Set 19 : Remove change to chrome/common/DEPS #

Patch Set 20 : fix some lint warnings #

Patch Set 21 : Move sampling params to call_stack_profile_metrics_provider.cc; make some things constexpr to avoid… #

Patch Set 22 : Make function internal. #

Patch Set 23 : Fix checks to exclude iOS for uptime. #

Patch Set 24 : Reduce delay in base unit test. #

Patch Set 25 : Temporary LOG statements for debugging. #

Patch Set 26 : Use WaitableEvent::ResetPolicy::AUTOMATIC in unit test. (Debug logging not yet removed.) #

Patch Set 27 : Remove debug log statements. #

Messages