Make stack sampling profiler sample beyond startup.

components/metrics/call_stack_profile_metrics_provider.cc

Index: components/metrics/call_stack_profile_metrics_provider.cc
diff --git a/components/metrics/call_stack_profile_metrics_provider.cc b/components/metrics/call_stack_profile_metrics_provider.cc
index 0df175bde1e46d34a7ec03881094839bed7d4915..2f3d81f48511860da870c71bf5b3c68a669129ad 100644
--- a/components/metrics/call_stack_profile_metrics_provider.cc
+++ b/components/metrics/call_stack_profile_metrics_provider.cc
@@ -19,7 +19,7 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/singleton.h"
-#include "base/metrics/field_trial.h"
+#include "base/metrics/field_trial_params.h"
 #include "base/metrics/metrics_hashes.h"
 #include "base/profiler/stack_sampling_profiler.h"
 #include "base/single_thread_task_runner.h"
@@ -34,6 +34,10 @@ namespace metrics {
 
 namespace {
 
+// Interval for periodic (post-startup) sampling, when enabled.
+constexpr base::TimeDelta kPeriodicSamplingInterval =
+    base::TimeDelta::FromSeconds(1);
+
 // Provide a mapping from the C++ "enum" definition of various process mile-
 // stones to the equivalent protobuf "enum" definition. This table-lookup
 // conversion allows for the implementation to evolve and still be compatible
@@ -56,8 +60,7 @@ const ProcessPhase
 // with them.
 struct ProfilesState {
   ProfilesState(const CallStackProfileParams& params,
-                StackSamplingProfiler::CallStackProfiles profiles,
-                base::TimeTicks start_timestamp);
+                StackSamplingProfiler::CallStackProfiles profiles);
   ProfilesState(ProfilesState&&);
   ProfilesState& operator=(ProfilesState&&);
 
@@ -68,22 +71,13 @@ struct ProfilesState {
   // The call stack profiles collected by the profiler.
   StackSamplingProfiler::CallStackProfiles profiles;
 
-  // The time at which the CallStackProfileMetricsProvider became aware of the
-  // request for profiling. In particular, this is when callback was requested
-  // via CallStackProfileMetricsProvider::GetProfilerCallback(). Used to
-  // determine if collection was disabled during the collection of the profile.
-  base::TimeTicks start_timestamp;
-
  private:
   DISALLOW_COPY_AND_ASSIGN(ProfilesState);
 };
 
 ProfilesState::ProfilesState(const CallStackProfileParams& params,
-                             StackSamplingProfiler::CallStackProfiles profiles,
-                             base::TimeTicks start_timestamp)
-    : params(params),
-      profiles(std::move(profiles)),
-      start_timestamp(start_timestamp) {}
+                             StackSamplingProfiler::CallStackProfiles profiles)
+    : params(params), profiles(std::move(profiles)) {}
 
 ProfilesState::ProfilesState(ProfilesState&&) = default;
 
@@ -180,7 +174,7 @@ void PendingProfiles::CollectProfilesIfCollectionEnabled(
   // since the start of collection for this profile.
   if (!collection_enabled_ ||
       (!last_collection_disable_time_.is_null() &&
-       last_collection_disable_time_ >= profiles.start_timestamp)) {
+       last_collection_disable_time_ >= profiles.params.start_timestamp)) {
     return;
   }
 
@@ -208,17 +202,37 @@ PendingProfiles::~PendingProfiles() {}
 
 // Will be invoked on either the main thread or the profiler's thread. Provides
 // the profiles to PendingProfiles to append, if the collecting state allows.
-void ReceiveCompletedProfilesImpl(
-    const CallStackProfileParams& params,
-    base::TimeTicks start_timestamp,
+base::Optional<StackSamplingProfiler::SamplingParams>
+ReceiveCompletedProfilesImpl(
+    CallStackProfileParams* params,

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

We can revert to passing this by const reference again.

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

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.

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

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.

     StackSamplingProfiler::CallStackProfiles profiles) {
   PendingProfiles::GetInstance()->CollectProfilesIfCollectionEnabled(
-      ProfilesState(params, std::move(profiles), start_timestamp));
+      ProfilesState(*params, std::move(profiles)));
+
+  // Now, schedule periodic sampling every 1s, if enabled by trial.
+  if (CallStackProfileMetricsProvider::IsPeriodicSamplingEnabled() &&

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

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.

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

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

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

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

+      params->process == CallStackProfileParams::BROWSER_PROCESS &&
+      params->thread == CallStackProfileParams::UI_THREAD) {
+    params->trigger = metrics::CallStackProfileParams::PERIODIC_COLLECTION;
+    params->start_timestamp = base::TimeTicks::Now();
+
+    StackSamplingProfiler::SamplingParams sampling_params;
+    sampling_params.initial_delay = kPeriodicSamplingInterval;
+    sampling_params.bursts = 1;
+    sampling_params.samples_per_burst = 1;
+    // Below are unused:
+    sampling_params.burst_interval = base::TimeDelta::FromMilliseconds(0);
+    sampling_params.sampling_interval = base::TimeDelta::FromMilliseconds(0);
+    return sampling_params;
+  }
+  return base::Optional<StackSamplingProfiler::SamplingParams>();
 }
 
 // Invoked on an arbitrary thread. Ignores the provided profiles.
-void IgnoreCompletedProfiles(
-    StackSamplingProfiler::CallStackProfiles profiles) {}
+base::Optional<StackSamplingProfiler::SamplingParams> IgnoreCompletedProfiles(
+    StackSamplingProfiler::CallStackProfiles profiles) {
+  return base::Optional<StackSamplingProfiler::SamplingParams>();
+}
 
 // Functions to encode protobufs ----------------------------------------------
 
@@ -280,6 +294,7 @@ void CopyAnnotationsToProto(uint32_t new_milestones,
 void CopyProfileToProto(
     const StackSamplingProfiler::CallStackProfile& profile,
     CallStackProfileParams::SampleOrderingSpec ordering_spec,
+    int64_t sampling_period_ms,
     CallStackProfile* proto_profile) {
   if (profile.samples.empty())
     return;
@@ -330,8 +345,7 @@ void CopyProfileToProto(
 
   proto_profile->set_profile_duration_ms(
       profile.profile_duration.InMilliseconds());
-  proto_profile->set_sampling_period_ms(
-      profile.sampling_period.InMilliseconds());
+  proto_profile->set_sampling_period_ms(sampling_period_ms);
 }
 
 // Translates CallStackProfileParams's process to the corresponding
@@ -405,6 +419,8 @@ SampledProfile::TriggerEvent ToSampledProfileTriggerEvent(
       return SampledProfile::JANKY_TASK;
     case CallStackProfileParams::THREAD_HUNG:
       return SampledProfile::THREAD_HUNG;
+    case CallStackProfileParams::PERIODIC_COLLECTION:
+      return SampledProfile::PERIODIC_COLLECTION;

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

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), 2017/07/07 16:18:48]

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, 2017/07/07 18:15:17]

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), 2017/07/07 19:04:14]

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

   }
   NOTREACHED();
   return SampledProfile::UNKNOWN_TRIGGER_EVENT;
@@ -414,10 +430,8 @@ SampledProfile::TriggerEvent ToSampledProfileTriggerEvent(
 
 // CallStackProfileMetricsProvider --------------------------------------------
 
-const char CallStackProfileMetricsProvider::kFieldTrialName[] =
-    "StackProfiling";
-const char CallStackProfileMetricsProvider::kReportProfilesGroupName[] =
-    "Report profiles";
+const base::Feature CallStackProfileMetricsProvider::kEnableReporting = {
+    "SamplingProfilerReporting", base::FEATURE_DISABLED_BY_DEFAULT};
 
 CallStackProfileMetricsProvider::CallStackProfileMetricsProvider() {
 }
@@ -428,23 +442,33 @@ CallStackProfileMetricsProvider::~CallStackProfileMetricsProvider() {
 // This function can be invoked on an abitrary thread.
 StackSamplingProfiler::CompletedCallback
 CallStackProfileMetricsProvider::GetProfilerCallback(
-    const CallStackProfileParams& params) {
+    CallStackProfileParams* params) {
   // Ignore the profiles if the collection is disabled. If the collection state
   // changes while collecting, this will be detected by the callback and
   // profiles will be ignored at that point.
   if (!PendingProfiles::GetInstance()->IsCollectionEnabled())
     return base::Bind(&IgnoreCompletedProfiles);
 
-  return base::Bind(&ReceiveCompletedProfilesImpl, params,
-                    base::TimeTicks::Now());
+  params->start_timestamp = base::TimeTicks::Now();
+  return base::Bind(&ReceiveCompletedProfilesImpl, params);
 }
 
 // static
 void CallStackProfileMetricsProvider::ReceiveCompletedProfiles(
-    const CallStackProfileParams& params,
-    base::TimeTicks start_timestamp,
-    StackSamplingProfiler::CallStackProfiles profiles) {
-  ReceiveCompletedProfilesImpl(params, start_timestamp, std::move(profiles));
+    CallStackProfileParams* params,
+    base::StackSamplingProfiler::CallStackProfiles profiles) {
+  ReceiveCompletedProfilesImpl(params, std::move(profiles));
+}
+
+// static
+bool CallStackProfileMetricsProvider::IsPeriodicSamplingEnabled() {
+  // Ensure FeatureList has been initialized before calling into an API that
+  // calls base::FeatureList::IsEnabled() internally. While extremely unlikely,
+  // it is possible that the profiler callback and therefore this function get
+  // called before FeatureList initialization (e.g. if machine was suspended).
+  return base::FeatureList::GetInstance() != nullptr &&
+         base::GetFieldTrialParamByFeatureAsBool(kEnableReporting, "periodic",
+                                                 false);
 }
 
 void CallStackProfileMetricsProvider::OnRecordingEnabled() {
@@ -462,17 +486,27 @@ void CallStackProfileMetricsProvider::ProvideGeneralMetrics(
 
   DCHECK(IsReportingEnabledByFieldTrial() || pending_profiles.empty());
 
+  // TODO(asvitkine): For post-startup periodic samples, this is currently
+  // wasteful as each sample is reported in its own profile. We should attempt
+  // to merge profiles to save bandwidth.
   for (const ProfilesState& profiles_state : pending_profiles) {
     for (const StackSamplingProfiler::CallStackProfile& profile :
              profiles_state.profiles) {
       SampledProfile* sampled_profile = uma_proto->add_sampled_profile();
-      sampled_profile->set_process(ToExecutionContextProcess(
-          profiles_state.params.process));
-      sampled_profile->set_thread(ToExecutionContextThread(
-          profiles_state.params.thread));
-      sampled_profile->set_trigger_event(ToSampledProfileTriggerEvent(
-          profiles_state.params.trigger));
+      sampled_profile->set_process(
+          ToExecutionContextProcess(profiles_state.params.process));
+      sampled_profile->set_thread(
+          ToExecutionContextThread(profiles_state.params.thread));
+      sampled_profile->set_trigger_event(
+          ToSampledProfileTriggerEvent(profiles_state.params.trigger));
+
+      int64_t sampling_period_ms = profile.sampling_period.InMilliseconds();
+      if (sampled_profile->trigger_event() ==
+          SampledProfile::PERIODIC_COLLECTION) {
+        sampling_period_ms += kPeriodicSamplingInterval.InMilliseconds();
+      }
       CopyProfileToProto(profile, profiles_state.params.ordering_spec,
+                         sampling_period_ms,
                          sampled_profile->mutable_call_stack_profile());
     }
   }
@@ -485,10 +519,7 @@ void CallStackProfileMetricsProvider::ResetStaticStateForTesting() {
 
 // static
 bool CallStackProfileMetricsProvider::IsReportingEnabledByFieldTrial() {
-  const std::string group_name = base::FieldTrialList::FindFullName(
-      CallStackProfileMetricsProvider::kFieldTrialName);
-  return group_name ==
-      CallStackProfileMetricsProvider::kReportProfilesGroupName;
+  return base::FeatureList::IsEnabled(kEnableReporting);
 }
 
 }  // namespace metrics