Make stack sampling profiler sample beyond startup.

chrome/common/stack_sampling_configuration.h

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_COMMON_STACK_SAMPLING_CONFIGURATION_H_
 #define CHROME_COMMON_STACK_SAMPLING_CONFIGURATION_H_
 
 #include <string>
 
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/profiler/stack_sampling_profiler.h"
+#include "components/metrics/call_stack_profile_metrics_provider.h"
 
 namespace base {
 class CommandLine;
 }  // namespace base
 
 // StackSamplingConfiguration chooses a configuration for the enable state of
 // the stack sampling profiler across all processes. This configuration is
 // determined once at browser process startup. Configurations for child
 // processes are communicated via command line arguments.
 class StackSamplingConfiguration {
  public:
   StackSamplingConfiguration();
 
   // Get the stack sampling params to use for this process.
   base::StackSamplingProfiler::SamplingParams
-      GetSamplingParamsForCurrentProcess() const;
+  GetSamplingParamsForCurrentProcess() const;
+
+  // Returns the profiler callback to use for this process.
+  base::StackSamplingProfiler::CompletedCallback
+  GetProfilerCallbackForCurrentProcess();

[Mike Wittman, 2017/06/21 14:49:06]

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), 2017/06/29 20:51:04]

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, 2017/07/01 02:44:06]

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.

[Alexei Svitkine (slow), 2017/07/06 15:51:09]

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?

[Mike Wittman, 2017/07/06 17:25:52]

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.

[Alexei Svitkine (slow), 2017/07/06 19:36:25]

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.

[Mike Wittman, 2017/07/06 22:14:01]

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.

[Alexei Svitkine (slow), 2017/07/07 16:18:48]

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.

[Mike Wittman, 2017/07/07 18:15:17]

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.

[Alexei Svitkine (slow), 2017/07/07 19:04:14]

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.

 
   // Returns true if the profiler should be started for the current process.
   bool IsProfilerEnabledForCurrentProcess() const;
 
   // Get the synthetic field trial configuration. Returns true if a synthetic
   // field trial should be registered. This should only be called from the
   // browser process. When run at startup, the profiler must use a synthetic
   // field trial since it runs before the metrics field trials are initialized.
   bool GetSyntheticFieldTrial(std::string* trial_name,
                               std::string* group_name) const;
@@ skipping 35 matching lines @@
       const std::vector<Variation>& variations);
 
   // Generates sampling profiler configurations for all processes.
   static ProfileConfiguration GenerateConfiguration();
 
   // In the browser process this represents the configuration to use across all
   // Chrome processes. In the child processes it is always
   // PROFILE_FROM_COMMAND_LINE.
   const ProfileConfiguration configuration_;
 
+  // The current profiling params. Not const since these may be changed when
+  // transitioning from start-up profiling to periodic profiling.
+  metrics::CallStackProfileParams profile_params_;
+
   DISALLOW_COPY_AND_ASSIGN(StackSamplingConfiguration);
 };
 
 #endif  // CHROME_COMMON_STACK_SAMPLING_CONFIGURATION_H_