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..46c87ac13c156183af71485272c8df1ccb09c47e 100644
--- a/base/profiler/stack_sampling_profiler.h
+++ b/base/profiler/stack_sampling_profiler.h
@@ -13,6 +13,7 @@
 #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"
 
@@ -23,7 +24,7 @@ namespace base {
 // 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;
@@ -33,7 +34,7 @@ namespace base {
 //
 //   // To process the profiles within Chrome rather than via UMA, set a custom
 //   // completed callback:
-//   base::Callback<void(const std::vector<Profile>&)>
+//   base::StackStackSamplingProfiler::CompletedCallback
 //       thread_safe_callback = ...;
 //   profiler.SetCustomCompletedCallback(thread_safe_callback);
 //
@@ -41,13 +42,24 @@ namespace base {
 //   // ... work being done on the target thread here ...
 //   profiler.Stop();  // optional, stops collection before complete per params
 //
+// 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 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.
+// 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 Profiles. Each Profile 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.
@@ -71,14 +83,17 @@ class BASE_EXPORT StackSamplingProfiler {
 
   // Frame represents an individual sampled stack frame with module information.
   struct Frame {
-    Frame();
+    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 Profile::modules. We don't represent module state
+    // directly here to save space.
+    size_t module_index;
+
+    // Identifies a unknown module.

[Peter Kasting, 2015/03/30 23:07:35]

Nit: an

[Mike Wittman, 2015/03/31 01:06:37]

Done.

+    static const size_t kUnknownModuleIndex = static_cast<size_t>(-1);

[Peter Kasting, 2015/03/30 23:07:35]

Constants go above the constructor; see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_O...
.

[Mike Wittman, 2015/03/31 01:06:36]

Done.

   };
 
   // Sample represents a set of stack frames.
@@ -102,31 +117,7 @@ class BASE_EXPORT StackSamplingProfiler {
 
   // 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);
-  };
+  class NativeStackSampler;

[Peter Kasting, 2015/03/30 23:07:35]

Because the style guide bans public member classes that aren't part of the class
interface (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Nested_Classes
), I would leave this as an external class but make it a non-member class (just
leave it at global scope).

(That's what I assumed you were going to do originally.)

[Mike Wittman, 2015/03/31 01:06:37]

Done.

 
   // Represents parameters that configure the sampling.
   struct BASE_EXPORT SamplingParams {
@@ -142,27 +133,30 @@ class BASE_EXPORT StackSamplingProfiler {
     // 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<Profile>&)>;
+
   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 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.
@@ -170,15 +164,49 @@ class BASE_EXPORT StackSamplingProfiler {
 
   // 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);
+  // however, it is called with the collected profiles instead. Note that the
+  // call to the callback occurs *on the profiler thread*.

[Peter Kasting, 2015/03/30 23:07:34]

Regarding the last sentence here, aren't all calls to/from the public API here
on the profiler thread?  I'm wondering why a caller would expect something else,
that this needs to be called out.

[Mike Wittman, 2015/03/31 01:06:37]

"on the profiler thread" is imprecise -- this is actually being called from the
SamplerThread. What the caller really needs to know though is that it will be
called on some unspecified other thread than the one that constructed the
object. Updated the comment to hopefully be more clear.

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

OK, now as a reader I'm frightened.  Are callers expected to handle having this
called on *any* thread?  Couldn't we do something like PostTask to the
construction thread, so we could call the caller back on the same thread they
called this on, or would that be too big of an imposition?

It seems very unusual to be told "we're going to call you on another thread" and
nothing more.  If this is really the best design, we should probably at least
say something like "... callback occurs on a thread the profiler constructs,
rather than on the thread used to construct the profile and call this setter,
and thus the callback must be callable on any thread."  You could also move some
of that last bit up to a new comment on the CompletedCallback alias defined
above if you want.

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

I'd like to keep this usable from threads both with and without message loops. I
do expect users with message loops to immediately PostTask back to themselves in
the callback.
Done.

[Peter Kasting, 2015/04/01 06:16:40]

That's fine, but you might want to explicitly say in your comments that that's
why we don't try to post back to the creation thread, and that callers who do
have message loops on that thread would likely want to do this themselves.

[Mike Wittman, 2015/04/01 19:08:22]

Done.

+  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 profiles. |completed_callback|
+    // must be thread-safe.
+    SamplingThread(scoped_ptr<NativeStackSampler> native_sampler,
+                   const SamplingParams& params,
+                   CompletedCallback completed_callback);
+    ~SamplingThread() override;
+
+    // PlatformThread::Delegate:
+    void ThreadMain() override;
+
+    void Stop();
+
+   private:
+    // Collects a profile from a single burst. Returns true if the profile was
+    // collected, or false if collection was stopped before it completed.
+    bool CollectProfile(Profile* profile, TimeDelta* elapsed_time);
+
+    // Collects profiles from all bursts, or until the sampling is stopped. If
+    // stopped before complete, |profiles| will contain only full bursts.
+    void CollectProfiles(std::vector<Profile>* 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,21 +214,21 @@ 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);
 
 }  // namespace base
 
 #endif  // BASE_PROFILER_STACK_SAMPLING_PROFILER_H_
+