StackSamplingProfiler clean up

base/profiler/stack_sampling_profiler.h

Index: base/profiler/stack_sampling_profiler.h
diff --git a/base/profiler/stack_sampling_profiler.h b/base/profiler/stack_sampling_profiler.h
index 60faa51b780476ad3cf791f1482c2b393d2d3f55..ee7642a48306324b7c4cc73f7a36710d2c96a655 100644
--- a/base/profiler/stack_sampling_profiler.h
+++ b/base/profiler/stack_sampling_profiler.h
@@ -13,17 +13,20 @@
 #include "base/files/file_path.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string16.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 
 namespace base {
 
+class NativeStackSampler;
+
 // StackSamplingProfiler periodically stops a thread to sample its stack, for
 // the purpose of collecting information about which code paths are
 // executing. This information is used in aggregate by UMA to identify hot
 // and/or janky code paths.
 //
-// Sample StackStackSamplingProfiler usage:
+// Sample StackSamplingProfiler usage:
 //
 //   // Create and customize params as desired.
 //   base::StackStackSamplingProfiler::SamplingParams params;
@@ -31,9 +34,9 @@ namespace base {
 //   base::StackSamplingProfiler profiler(base::PlatformThread::CurrentId()),
 //       params);
 //
-//   // To process the profiles within Chrome rather than via UMA, set a custom
-//   // completed callback:
-//   base::Callback<void(const std::vector<Profile>&)>
+//   // To process the call stack profiles within Chrome rather than via UMA,
+//   // set a custom completed callback:
+//   base::StackStackSamplingProfiler::CompletedCallback
 //       thread_safe_callback = ...;
 //   profiler.SetCustomCompletedCallback(thread_safe_callback);
 //
@@ -41,13 +44,24 @@ namespace base {
 //   // ... work being done on the target thread here ...
 //   profiler.Stop();  // optional, stops collection before complete per params
 //
-// When all profiles are complete or the profiler is stopped, if the custom
-// completed callback was set it will be called from the profiler thread with
-// the completed profiles. If no callback was set, the profiles are stored
-// internally and retrieved for UMA through
-// GetPendingProfiles(). GetPendingProfiles() should never be called by other
-// code; to retrieve profiles for in-process processing, set a completed
-// callback.
+// The default SamplingParams causes stacks to be recorded in a single burst at
+// a 10Hz interval for a total of 30 seconds. All of these parameters may be
+// altered as desired.
+//
+// When all call stack profiles are complete or the profiler is stopped, if the
+// custom completed callback was set it is called from the profiler thread with
+// the completed profiles. A profile is considered complete if all requested
+// samples were recorded for the profile (i.e. it was not stopped
+// prematurely). If no callback was set, the completed profiles are stored
+// internally and retrieved for UMA through GetPendingProfiles().
+// GetPendingProfiles() should never be called by other code; to retrieve
+// profiles for in-process processing, set a completed callback.
+//
+// The results of the profiling are passed to the completed callback and consist
+// of a vector of CallStackProfiles. Each CallStackProfile corresponds to a
+// burst as specified in SamplingParams and contains a set of Samples and
+// Modules. One Sample corresponds to a single recorded stack, and the Modules
+// record those modules associated with the recorded stack frames.
 class BASE_EXPORT StackSamplingProfiler {
  public:
   // Module represents the module (DLL or exe) corresponding to a stack frame.
@@ -57,6 +71,7 @@ class BASE_EXPORT StackSamplingProfiler {
 
     // Points to the base address of the module.
     const void* base_address;
+
     // An opaque binary string that uniquely identifies a particular program
     // version with high probability. This is parsed from headers of the loaded
     // module.
@@ -65,120 +80,147 @@ class BASE_EXPORT StackSamplingProfiler {
     // On Windows:
     //   GUID + AGE in the debug image headers of a module.
     std::string id;
+
     // The filename of the module.
     FilePath filename;
   };
 
   // Frame represents an individual sampled stack frame with module information.
   struct Frame {
-    Frame();
+    // Identifies an unknown module.
+    static const size_t kUnknownModuleIndex = static_cast<size_t>(-1);
+
+    Frame(const void* instruction_pointer, size_t module_index);
     ~Frame();
 
     // The sampled instruction pointer within the function.
     const void* instruction_pointer;
-    // Index of the module in the array of modules. We don't represent module
-    // state directly here to save space.
-    int module_index;
+
+    // Index of the module in CallStackProfile::modules. We don't represent
+    // module state directly here to save space.
+    size_t module_index;
   };
 
   // Sample represents a set of stack frames.
   using Sample = std::vector<Frame>;
 
-  // Profile represents a set of samples.
-  struct BASE_EXPORT Profile {
-    Profile();
-    ~Profile();
+  // CallStackProfile represents a set of samples.
+  struct BASE_EXPORT CallStackProfile {
+    CallStackProfile();
+    ~CallStackProfile();
 
     std::vector<Module> modules;
     std::vector<Sample> samples;
+
     // Duration of this profile.
     TimeDelta profile_duration;
+
     // Time between samples.
     TimeDelta sampling_period;
+
     // True if sample ordering is important and should be preserved if and when
     // this profile is compressed and processed.
     bool preserve_sample_ordering;
   };
 
-  // NativeStackSampler abstracts the native implementation required to record a
-  // stack sample for a given thread.
-  class NativeStackSampler {
-   public:
-    virtual ~NativeStackSampler();
-
-    // Create a stack sampler that records samples for |thread_handle|. Returns
-    // null if this platform does not support stack sampling.
-    static scoped_ptr<NativeStackSampler> Create(PlatformThreadId thread_id);
-
-    // Notify the sampler that we're starting to record a new profile. This
-    // function is called on the SamplingThread.
-    virtual void ProfileRecordingStarting(Profile* profile) = 0;
-
-    // Record a stack sample. This function is called on the SamplingThread.
-    virtual void RecordStackSample(Sample* sample) = 0;
-
-    // Notify the sampler that we've stopped recording the current profile. This
-    // function is called on the SamplingThread.
-    virtual void ProfileRecordingStopped() = 0;
-
-   protected:
-    NativeStackSampler();
-
-   private:
-    DISALLOW_COPY_AND_ASSIGN(NativeStackSampler);
-  };
-
   // Represents parameters that configure the sampling.
   struct BASE_EXPORT SamplingParams {
     SamplingParams();
 
     // Time to delay before first samples are taken. Defaults to 0.
     TimeDelta initial_delay;
+
     // Number of sampling bursts to perform. Defaults to 1.
     int bursts;
+
     // Interval between sampling bursts. This is the desired duration from the
     // start of one burst to the start of the next burst. Defaults to 10s.
     TimeDelta burst_interval;
+
     // Number of samples to record per burst. Defaults to 300.
     int samples_per_burst;
+
     // Interval between samples during a sampling burst. This is the desired
-    // duration from the start of one burst to the start of the next
-    // burst. Defaults to 100ms.
+    // duration from the start of one sample to the start of the next
+    // sample. Defaults to 100ms.
     TimeDelta sampling_interval;
+
     // True if sample ordering is important and should be preserved if and when
     // this profile is compressed and processed. Defaults to false.
     bool preserve_sample_ordering;
   };
 
+  using CompletedCallback =
+      Callback<void(const std::vector<CallStackProfile>&)>;
+
   StackSamplingProfiler(PlatformThreadId thread_id,
                         const SamplingParams& params);
   ~StackSamplingProfiler();
 
   // Initializes the profiler and starts sampling.
   void Start();
+
   // Stops the profiler and any ongoing sampling. Calling this function is
-  // optional; if not invoked profiling will terminate when all the profiling
-  // bursts specified in the SamplingParams are completed.
+  // optional; if not invoked profiling terminates when all the profiling bursts
+  // specified in the SamplingParams are completed.
   void Stop();
 
-  // Gets the pending profiles into *|profiles| and clears the internal
-  // storage. This function is thread safe.
+  // Moves all pending call stack profiles from internal storage to
+  // |profiles|. This function is thread safe.
   //
   // ***This is intended for use only by UMA.*** Callers who want to process the
   // collected profiles should use SetCustomCompletedCallback.
-  static void GetPendingProfiles(std::vector<Profile>* profiles);
-
-  // By default, collected profiles are stored internally and can be retrieved
-  // by GetPendingProfiles. If a callback is provided via this function,
-  // however, it will be called with the collected profiles instead. Note that
-  // this call to the callback occurs *on the profiler thread*.
-  void SetCustomCompletedCallback(
-      Callback<void(const std::vector<Profile>&)> callback);
+  static void GetPendingProfiles(
+      std::vector<CallStackProfile>* call_stack_profiles);

[Peter Kasting, 2015/03/31 01:51:26]

You use std::vector<CallStackProfile> in enough places in these various files
that it might make sense to define an alias for it.  Normally in Chrome code
we'd call this "CallStackProfiles" (and avoid "vector" in the name).

[Mike Wittman, 2015/03/31 19:29:41]

Done.

+
+  // By default, collected call stack profiles are stored internally and can be
+  // retrieved by GetPendingProfiles. If a callback is provided via this
+  // function, however, it is called with the collected profiles instead. Note
+  // that the call to the callback occurs *on a different thread* from the one
+  // that constructed this object.
+  void set_custom_completed_callback(CompletedCallback callback) {
+    custom_completed_callback_ = callback;
+  }
 
  private:
-  class SamplingThread;
-  struct SamplingThreadDeleter {
-    void operator() (SamplingThread* thread) const;
+  // SamplingThread is a separate thread used to suspend and sample stacks from
+  // the target thread.
+  class SamplingThread : public PlatformThread::Delegate {
+   public:
+    // Samples stacks using |native_sampler|. When complete, invokes
+    // |completed_callback| with the collected call stack profiles.
+    // |completed_callback| must be thread-safe.

[Peter Kasting, 2015/03/31 01:51:26]

Nit: "must be callable on any thread" might be clearer?

[Mike Wittman, 2015/03/31 19:29:41]

Done.

+    SamplingThread(scoped_ptr<NativeStackSampler> native_sampler,
+                   const SamplingParams& params,
+                   CompletedCallback completed_callback);
+    ~SamplingThread() override;
+
+    // PlatformThread::Delegate:
+    void ThreadMain() override;
+
+    void Stop();
+
+   private:
+    // Collects a call stack profile from a single burst. Returns true if the
+    // profile was collected, or false if collection was stopped before it
+    // completed.
+    bool CollectProfile(CallStackProfile* profile, TimeDelta* elapsed_time);
+
+    // Collects call stack profiles from all bursts, or until the sampling is
+    // stopped. If stopped before complete, |call_stack_profiles| will contain
+    // only full bursts.
+    void CollectProfiles(std::vector<CallStackProfile>* profiles);
+
+    scoped_ptr<NativeStackSampler> native_sampler_;
+    const SamplingParams params_;
+
+    // If Stop() is called, it signals this event to force the sampling to
+    // terminate before all the samples specified in |params_| are collected.
+    WaitableEvent stop_event_;
+
+    CompletedCallback completed_callback_;
+
+    DISALLOW_COPY_AND_ASSIGN(SamplingThread);
   };
 
   // The thread whose stack will be sampled.
@@ -186,18 +228,17 @@ class BASE_EXPORT StackSamplingProfiler {
 
   const SamplingParams params_;
 
-  scoped_ptr<SamplingThread, SamplingThreadDeleter> sampling_thread_;
-  scoped_ptr<NativeStackSampler> native_sampler_;
+  scoped_ptr<SamplingThread> sampling_thread_;
 
-  Callback<void(const std::vector<Profile>&)> custom_completed_callback_;
+  CompletedCallback custom_completed_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(StackSamplingProfiler);
 };
 
-// Defined to allow equality check of Samples.
+// The metrics provider code wants to put Samples in a map and compare them,
+// which requires us to define a few operators.
 BASE_EXPORT bool operator==(const StackSamplingProfiler::Frame& a,
                             const StackSamplingProfiler::Frame& b);
-// Defined to allow ordering of Samples.
 BASE_EXPORT bool operator<(const StackSamplingProfiler::Frame& a,
                            const StackSamplingProfiler::Frame& b);