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}

base/profiler/stack_sampling_profiler.h

@@ -16,6 +16,7 @@
 #include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/strings/string16.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/platform_thread.h"
@@ -215,13 +216,18 @@ class BASE_EXPORT StackSamplingProfiler {
   // are move-only. Other threads, including the UI thread, may block on
   // callback completion so this should run as quickly as possible.
   //
+  // After collection completion, the callback may instruct the profiler to do
+  // additional collection(s) by returning a SamplingParams object to indicate
+  // collection should be started again.
+  //
   // IMPORTANT NOTE: The callback is invoked on a thread the profiler
   // constructs, rather than on the thread used to construct the profiler and
   // set the callback, and thus the callback must be callable on any thread. For
   // threads with message loops that create StackSamplingProfilers, posting a
   // task to the message loop with the moved (i.e. std::move) profiles is the
   // thread-safe callback implementation.
-  using CompletedCallback = Callback<void(CallStackProfiles)>;
+  using CompletedCallback =
+      Callback<Optional<SamplingParams>(CallStackProfiles)>;
 
   // Creates a profiler for the CURRENT thread that sends completed profiles
   // to |callback|. An optional |test_delegate| can be supplied by tests.
@@ -299,9 +305,9 @@ class BASE_EXPORT StackSamplingProfiler {
   // and later passed to the sampling thread when profiling is started.
   std::unique_ptr<NativeStackSampler> native_sampler_;
 
-  // An ID uniquely identifying this collection to the sampling thread. This
+  // An ID uniquely identifying this profiler to the sampling thread. This
   // will be an internal "null" value when no collection has been started.
-  int collection_id_;
+  int profiler_id_;
 
   // Stored until it can be passed to the NativeStackSampler created in Start().
   NativeStackSamplerTestDelegate* const test_delegate_;

base/profiler/stack_sampling_profiler_unittest.cc

@@ -5,6 +5,7 @@
 #include <stddef.h>
 #include <stdint.h>
 
+#include <algorithm>
 #include <cstdlib>
 #include <memory>
 #include <utility>
@@ -323,19 +324,46 @@ void SynchronousUnloadNativeLibrary(NativeLibrary library) {
 }
 
 // Called on the profiler thread when complete, to collect profiles.
-void SaveProfiles(CallStackProfiles* profiles,
-                  CallStackProfiles pending_profiles) {
+Optional<StackSamplingProfiler::SamplingParams> SaveProfiles(
+    CallStackProfiles* profiles,
+    CallStackProfiles pending_profiles) {
   *profiles = std::move(pending_profiles);
+  return Optional<StackSamplingProfiler::SamplingParams>();
 }
 
 // Called on the profiler thread when complete. Collects profiles produced by
 // the profiler, and signals an event to allow the main thread to know that that
 // the profiler is done.
-void SaveProfilesAndSignalEvent(CallStackProfiles* profiles,
-                                WaitableEvent* event,
-                                CallStackProfiles pending_profiles) {
+Optional<StackSamplingProfiler::SamplingParams> SaveProfilesAndSignalEvent(
+    CallStackProfiles* profiles,
+    WaitableEvent* event,
+    CallStackProfiles pending_profiles) {
   *profiles = std::move(pending_profiles);
   event->Signal();
+  return Optional<StackSamplingProfiler::SamplingParams>();
+}
+
+// Similar to SaveProfilesAndSignalEvent(), but will schedule a second
+// collection after the first call back.
+Optional<StackSamplingProfiler::SamplingParams> SaveProfilesAndReschedule(
+    std::vector<CallStackProfiles>* profiles,
+    WaitableEvent* event,
+    CallStackProfiles pending_profiles) {
+  profiles->push_back(std::move(pending_profiles));
+
+  event->Signal();
+
+  if (profiles->size() == 2)
+    return Optional<StackSamplingProfiler::SamplingParams>();
+
+  StackSamplingProfiler::SamplingParams sampling_params;
+  sampling_params.initial_delay = base::TimeDelta::FromMilliseconds(100);
+  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;
 }
 
 // Executes the function with the target thread running and executing within
@@ -1020,6 +1048,34 @@ PROFILER_TEST_F(StackSamplingProfilerTest, CanRunMultipleTimes) {
   });
 }
 
+PROFILER_TEST_F(StackSamplingProfilerTest, RescheduledByCallback) {
+  WithTargetThread([](PlatformThreadId target_thread_id) {
+    SamplingParams params;
+    params.sampling_interval = TimeDelta::FromMilliseconds(0);
+    params.samples_per_burst = 1;
+
+    std::vector<CallStackProfiles> profiles;
+    WaitableEvent sampling_completed(WaitableEvent::ResetPolicy::AUTOMATIC,
+                                     WaitableEvent::InitialState::NOT_SIGNALED);
+    const StackSamplingProfiler::CompletedCallback callback =
+        Bind(&SaveProfilesAndReschedule, Unretained(&profiles),
+             Unretained(&sampling_completed));
+    StackSamplingProfiler profiler(target_thread_id, params, callback);
+
+    // Start once and wait for it to be completed.
+    profiler.Start();
+    sampling_completed.Wait();
+    ASSERT_EQ(1u, profiles.size());
+    ASSERT_EQ(1u, profiles[0].size());
+
+    // Now, wait for the second callback call.
+    sampling_completed.Wait();
+    profiler.Stop();
+    ASSERT_EQ(2u, profiles.size());
+    ASSERT_EQ(1u, profiles[1].size());
+  });
+}
+
 // Checks that the different profilers may be run.
 PROFILER_TEST_F(StackSamplingProfilerTest, CanRunMultipleProfilers) {
   SamplingParams params;

base/time/time.h

@@ -410,7 +410,7 @@ class TimeBase {
   static TimeClass FromInternalValue(int64_t us) { return TimeClass(us); }
 
  protected:
-  explicit TimeBase(int64_t us) : us_(us) {}
+  constexpr explicit TimeBase(int64_t us) : us_(us) {}
 
   // Time value in a microsecond timebase.
   int64_t us_;
@@ -740,8 +740,7 @@ class BASE_EXPORT TimeTicks : public time_internal::TimeBase<TimeTicks> {
     WIN_ROLLOVER_PROTECTED_TIME_GET_TIME
   };
 
-  TimeTicks() : TimeBase(0) {
-  }
+  constexpr TimeTicks() : TimeBase(0) {}
 
   // Platform-dependent tick count representing "right now." When
   // IsHighResolution() returns false, the resolution of the clock could be

chrome/browser/chrome_browser_main.cc

@@ -630,16 +630,11 @@ ChromeBrowserMainParts::ChromeBrowserMainParts(
       result_code_(content::RESULT_CODE_NORMAL_EXIT),
       startup_watcher_(new StartupTimeBomb()),
       shutdown_watcher_(new ShutdownWatcherHelper()),
-      sampling_profiler_(
-          base::PlatformThread::CurrentId(),
-          StackSamplingConfiguration::Get()->
-              GetSamplingParamsForCurrentProcess(),
-          metrics::CallStackProfileMetricsProvider::GetProfilerCallback(
-              metrics::CallStackProfileParams(
-                  metrics::CallStackProfileParams::BROWSER_PROCESS,
-                  metrics::CallStackProfileParams::UI_THREAD,
-                  metrics::CallStackProfileParams::PROCESS_STARTUP,
-                  metrics::CallStackProfileParams::MAY_SHUFFLE))),
+      sampling_profiler_(base::PlatformThread::CurrentId(),
+                         StackSamplingConfiguration::Get()
+                             ->GetSamplingParamsForCurrentProcess(),
+                         metrics::CallStackProfileMetricsProvider::
+                             GetProfilerCallbackForBrowserProcessStartup()),
       profile_(NULL),
       run_message_loop_(true),
       local_state_(NULL) {

chrome/browser/chrome_browser_main.h

@@ -19,6 +19,7 @@
 #include "chrome/browser/process_singleton.h"
 #include "chrome/browser/ui/startup/startup_browser_creator.h"
 #include "chrome/common/stack_sampling_configuration.h"
+#include "components/metrics/call_stack_profile_params.h"
 #include "content/public/browser/browser_main_parts.h"
 #include "content/public/common/main_function_params.h"
 

components/metrics/BUILD.gn

@@ -271,9 +271,12 @@ static_library("single_sample_metrics") {
 
 source_set("call_stack_profile_params") {
   sources = [
-    "call_stack_profile_params.cc",
     "call_stack_profile_params.h",
   ]
+
+  deps = [
+    "//base:base",
+  ]
 }
 
 source_set("call_stacks") {

components/metrics/call_stack_profile_collector.cc

@@ -5,6 +5,7 @@
 #include "components/metrics/call_stack_profile_collector.h"
 
 #include <utility>
+#include <vector>
 
 #include "base/memory/ptr_util.h"
 #include "components/metrics/call_stack_profile_metrics_provider.h"
@@ -36,8 +37,10 @@ void CallStackProfileCollector::Collect(
   if (params.process != expected_process_)
     return;
 
+  CallStackProfileParams params_copy = params;
+  params_copy.start_timestamp = start_timestamp;
   CallStackProfileMetricsProvider::ReceiveCompletedProfiles(
-      params, start_timestamp, std::move(profiles));
+      &params_copy, std::move(profiles));
 }
 
 }  // namespace metrics

components/metrics/call_stack_profile_metrics_provider.h

@@ -7,6 +7,7 @@
 
 #include <vector>
 
+#include "base/feature_list.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/profiler/stack_sampling_profiler.h"
@@ -14,8 +15,24 @@
 #include "components/metrics/metrics_provider.h"
 
 namespace metrics {
+
 class ChromeUserMetricsExtension;
 
+// Internal to expose functions for testing.
+namespace internal {
+
+// Returns the process uptime as a TimeDelta.
+base::TimeDelta GetUptime();
+
+// Get a callback for use with StackSamplingProfiler that provides completed
+// profiles to this object. The callback should be immediately passed to the
+// StackSamplingProfiler, and should not be reused between
+// StackSamplingProfilers. This function may be called on any thread.
+base::StackSamplingProfiler::CompletedCallback GetProfilerCallback(
+    CallStackProfileParams* params);
+
+}  // namespace internal
+
 // Performs metrics logging for the stack sampling profiler.
 class CallStackProfileMetricsProvider : public MetricsProvider {
  public:
@@ -36,32 +53,31 @@ class CallStackProfileMetricsProvider : public MetricsProvider {
   CallStackProfileMetricsProvider();
   ~CallStackProfileMetricsProvider() override;
 
-  // Get a callback for use with StackSamplingProfiler that provides completed
-  // profiles to this object. The callback should be immediately passed to the
-  // StackSamplingProfiler, and should not be reused between
-  // StackSamplingProfilers. This function may be called on any thread.
-  static base::StackSamplingProfiler::CompletedCallback GetProfilerCallback(
-      const CallStackProfileParams& params);
+  // Returns a callback for use with StackSamplingProfiler that sets up
+  // parameters for browser process startup sampling. The callback should be
+  // immediately passed to the StackSamplingProfiler, and should not be reused.
+  static base::StackSamplingProfiler::CompletedCallback
+  GetProfilerCallbackForBrowserProcessStartup();
 
   // Provides completed stack profiles to the metrics provider. Intended for use
   // when receiving profiles over IPC. In-process StackSamplingProfiler users
-  // should use GetProfilerCallback() instead. |profiles| is not const& because
-  // it must be passed with std::move.
+  // should instead use a variant of GetProfilerCallback*(). |profiles| is not
+  // const& because it must be passed with std::move.
   static void ReceiveCompletedProfiles(
-      const CallStackProfileParams& params,
-      base::TimeTicks start_timestamp,
+      CallStackProfileParams* params,
       base::StackSamplingProfiler::CallStackProfiles profiles);
 
+  // Whether periodic sampling is enabled via a trial.
+  static bool IsPeriodicSamplingEnabled();
+
   // MetricsProvider:
   void OnRecordingEnabled() override;
   void OnRecordingDisabled() override;
   void ProvideGeneralMetrics(ChromeUserMetricsExtension* uma_proto) override;
 
  protected:
-  // Finch field trial and group for reporting profiles. Provided here for test
-  // use.
-  static const char kFieldTrialName[];
-  static const char kReportProfilesGroupName[];
+  // base::Feature for reporting profiles. Provided here for test use.
+  static const base::Feature kEnableReporting;
 
   // Reset the static state to the defaults after startup.
   static void ResetStaticStateForTesting();

components/metrics/call_stack_profile_params.cc

-// 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.
-
-#include "components/metrics/call_stack_profile_params.h"
-
-namespace metrics {
-
-CallStackProfileParams::CallStackProfileParams()
-    : CallStackProfileParams(UNKNOWN_PROCESS, UNKNOWN_THREAD, UNKNOWN) {}
-
-CallStackProfileParams::CallStackProfileParams(Process process, Thread thread,
-                                               Trigger trigger)
-    : CallStackProfileParams(process, thread, trigger, MAY_SHUFFLE) {}
-
-CallStackProfileParams::CallStackProfileParams(Process process, Thread thread,
-                                               Trigger trigger,
-                                               SampleOrderingSpec ordering_spec)
-    : process(process), thread(thread), trigger(trigger),
-      ordering_spec(ordering_spec) {}
-
-}  // namespace metrics

components/metrics/call_stack_profile_params.h

@@ -5,6 +5,8 @@
 #ifndef COMPONENTS_METRICS_CALL_STACK_PROFILE_PARAMS_H_
 #define COMPONENTS_METRICS_CALL_STACK_PROFILE_PARAMS_H_
 
+#include "base/time/time.h"
+
 namespace metrics {
 
 // Parameters to pass back to the metrics provider.
@@ -49,7 +51,8 @@ struct CallStackProfileParams {
     PROCESS_STARTUP,
     JANKY_TASK,
     THREAD_HUNG,
-    TRIGGER_LAST = THREAD_HUNG
+    PERIODIC_COLLECTION,
+    TRIGGER_LAST = PERIODIC_COLLECTION
   };
 
   // Allows the caller to specify whether sample ordering is
@@ -64,10 +67,20 @@ struct CallStackProfileParams {
 
   // The default constructor is required for mojo and should not be used
   // otherwise. A valid trigger should always be specified.
-  CallStackProfileParams();
-  CallStackProfileParams(Process process, Thread thread, Trigger trigger);
-  CallStackProfileParams(Process process, Thread thread, Trigger trigger,
-                         SampleOrderingSpec ordering_spec);
+  constexpr CallStackProfileParams()
+      : CallStackProfileParams(UNKNOWN_PROCESS, UNKNOWN_THREAD, UNKNOWN) {}
+  constexpr CallStackProfileParams(Process process,
+                                   Thread thread,
+                                   Trigger trigger)
+      : CallStackProfileParams(process, thread, trigger, MAY_SHUFFLE) {}
+  constexpr CallStackProfileParams(Process process,
+                                   Thread thread,
+                                   Trigger trigger,
+                                   SampleOrderingSpec ordering_spec)
+      : process(process),
+        thread(thread),
+        trigger(trigger),
+        ordering_spec(ordering_spec) {}
 
   // The collection process.
   Process process;
@@ -80,6 +93,12 @@ struct CallStackProfileParams {
 
   // Whether to preserve sample ordering.
   SampleOrderingSpec ordering_spec;
+
+  // 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;
 };
 
 }  // namespace metrics

components/metrics/child_call_stack_profile_collector.cc

@@ -65,7 +65,19 @@ void ChildCallStackProfileCollector::SetParentProfileCollector(
   profiles_.clear();
 }
 
-void ChildCallStackProfileCollector::Collect(
+base::Optional<base::StackSamplingProfiler::SamplingParams>
+ChildCallStackProfileCollector::Collect(
+    const CallStackProfileParams& params,
+    base::TimeTicks start_timestamp,
+    std::vector<CallStackProfile> profiles) {
+  // Impl function is used as it needs to PostTask() to itself on a different
+  // thread - which only works with a void return value.
+  CollectImpl(params, start_timestamp, std::move(profiles));
+  // Empty return value indicates that collection should not be re-started.
+  return base::Optional<base::StackSamplingProfiler::SamplingParams>();
+}
+
+void ChildCallStackProfileCollector::CollectImpl(
     const CallStackProfileParams& params,
     base::TimeTicks start_timestamp,
     std::vector<CallStackProfile> profiles) {
@@ -76,13 +88,11 @@ void ChildCallStackProfileCollector::Collect(
       (!base::ThreadTaskRunnerHandle::IsSet() ||
        base::ThreadTaskRunnerHandle::Get() != task_runner_)) {
     // Post back to the thread that owns the the parent interface.
-    task_runner_->PostTask(FROM_HERE, base::Bind(
-        &ChildCallStackProfileCollector::Collect,
-        // This class has lazy instance lifetime.
-        base::Unretained(this),
-        params,
-        start_timestamp,
-        base::Passed(std::move(profiles))));
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&ChildCallStackProfileCollector::CollectImpl,
+                              // This class has lazy instance lifetime.
+                              base::Unretained(this), params, start_timestamp,
+                              base::Passed(std::move(profiles))));
     return;
   }
 

components/metrics/child_call_stack_profile_collector.h

@@ -5,6 +5,8 @@
 #ifndef COMPONENTS_METRICS_CHILD_CALL_STACK_PROFILE_COLLECTOR_H_
 #define COMPONENTS_METRICS_CHILD_CALL_STACK_PROFILE_COLLECTOR_H_
 
+#include <vector>
+
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/single_thread_task_runner.h"
@@ -90,9 +92,14 @@ class ChildCallStackProfileCollector {
 
   using CallStackProfile = base::StackSamplingProfiler::CallStackProfile;
 
-  void Collect(const CallStackProfileParams& params,
-               base::TimeTicks start_timestamp,
-               std::vector<CallStackProfile> profiles);
+  base::Optional<base::StackSamplingProfiler::SamplingParams> Collect(
+      const CallStackProfileParams& params,
+      base::TimeTicks start_timestamp,
+      std::vector<CallStackProfile> profiles);
+
+  void CollectImpl(const CallStackProfileParams& params,
+                   base::TimeTicks start_timestamp,
+                   std::vector<CallStackProfile> profiles);
 
   // This object may be accessed on any thread, including the profiler
   // thread. The expected use case for the object is to be created and have

components/metrics/child_call_stack_profile_collector_unittest.cc

@@ -30,7 +30,7 @@ class ChildCallStackProfileCollectorTest : public testing::Test {
    public:
     using CallStackProfile = base::StackSamplingProfiler::CallStackProfile;
 
-    Receiver(mojom::CallStackProfileCollectorRequest request)
+    explicit Receiver(mojom::CallStackProfileCollectorRequest request)
         : binding_(this, std::move(request)) {}
     ~Receiver() override {}
 
@@ -73,6 +73,7 @@ class ChildCallStackProfileCollectorTest : public testing::Test {
   std::unique_ptr<Receiver> receiver_impl_;
   ChildCallStackProfileCollector child_collector_;
 
+ private:
   DISALLOW_COPY_AND_ASSIGN(ChildCallStackProfileCollectorTest);
 };
 

components/metrics/public/cpp/call_stack_profile_struct_traits.h

@@ -5,9 +5,11 @@
 // Defines StructTraits specializations for translating between mojo types and
 // base::StackSamplingProfiler types, with data validity checks.
 
-#ifndef COMPONENTS_METRICS_CALL_STACK_PROFILE_STRUCT_TRAITS_H_
-#define COMPONENTS_METRICS_CALL_STACK_PROFILE_STRUCT_TRAITS_H_
+#ifndef COMPONENTS_METRICS_PUBLIC_CPP_CALL_STACK_PROFILE_STRUCT_TRAITS_H_
+#define COMPONENTS_METRICS_PUBLIC_CPP_CALL_STACK_PROFILE_STRUCT_TRAITS_H_
 
+#include <string>
+#include <utility>
 #include <vector>
 
 #include "base/files/file_path.h"
@@ -307,6 +309,8 @@ struct EnumTraits<metrics::mojom::Trigger,
         return metrics::mojom::Trigger::JANKY_TASK;
       case metrics::CallStackProfileParams::Trigger::THREAD_HUNG:
         return metrics::mojom::Trigger::THREAD_HUNG;
+      case metrics::CallStackProfileParams::Trigger::PERIODIC_COLLECTION:
+        return metrics::mojom::Trigger::PERIODIC_COLLECTION;
     }
     NOTREACHED();
     return metrics::mojom::Trigger::UNKNOWN;
@@ -327,6 +331,9 @@ struct EnumTraits<metrics::mojom::Trigger,
       case metrics::mojom::Trigger::THREAD_HUNG:
         *out = metrics::CallStackProfileParams::Trigger::THREAD_HUNG;
         return true;
+      case metrics::mojom::Trigger::PERIODIC_COLLECTION:
+        *out = metrics::CallStackProfileParams::Trigger::PERIODIC_COLLECTION;
+        return true;
     }
     return false;
   }
@@ -371,7 +378,6 @@ struct StructTraits<metrics::mojom::CallStackProfileParamsDataView,
 template <>
 struct EnumTraits<metrics::mojom::SampleOrderingSpec,
                   metrics::CallStackProfileParams::SampleOrderingSpec> {
-
   static metrics::mojom::SampleOrderingSpec ToMojom(
       metrics::CallStackProfileParams::SampleOrderingSpec spec) {
     switch (spec) {
@@ -400,6 +406,6 @@ struct EnumTraits<metrics::mojom::SampleOrderingSpec,
   }
 };
 
-}  // mojo
+}  // namespace mojo
 
-#endif  // COMPONENTS_METRICS_CALL_STACK_PROFILE_STRUCT_TRAITS_H_
+#endif  // COMPONENTS_METRICS_PUBLIC_CPP_CALL_STACK_PROFILE_STRUCT_TRAITS_H_

components/metrics/public/interfaces/call_stack_profile_collector.mojom

@@ -66,6 +66,7 @@ enum Trigger {
   PROCESS_STARTUP,
   JANKY_TASK,
   THREAD_HUNG,
+  PERIODIC_COLLECTION,
 };
 
 enum SampleOrderingSpec {