Support parallel captures from the StackSamplingProfiler.

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 622f6e5764a5b4e59fbf0a6eed6c7d328d737fc3..0346a5c67aac8a93eef9a7dade22348640cc238f 100644
--- a/base/profiler/stack_sampling_profiler.h
+++ b/base/profiler/stack_sampling_profiler.h
@@ -25,6 +25,7 @@ namespace base {
 
 class NativeStackSampler;
 class NativeStackSamplerTestDelegate;
+class WaitableEvent;
 
 // StackSamplingProfiler periodically stops a thread to sample its stack, for
 // the purpose of collecting information about which code paths are
@@ -183,44 +184,65 @@ class BASE_EXPORT StackSamplingProfiler {
   };
 
   // The callback type used to collect completed profiles. The passed |profiles|
-  // are move-only.
+  // are move-only. This should run quickly as possible that another thread,

[Mike Wittman, 2017/02/15 03:26:44]

I'm having trouble parsing this. How about:

Other threads, including the UI thread, may block on callback completion, so
this should run as quickly as possible.

?

[bcwhite, 2017/02/15 16:17:35]

Done.

+  // even the UI thread, to block awaiting its completion.
   //
-  // IMPORTANT NOTE: the callback is invoked on a thread the profiler
+  // 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 a copy of the profiles is the recommended
+  // task to the message loop with the moved (i.e. std::move) profiles is the
   // thread-safe callback implementation.
   using CompletedCallback = Callback<void(CallStackProfiles)>;
 
-  // Creates a profiler that sends completed profiles to |callback|. The second
-  // constructor is for test purposes.
-  StackSamplingProfiler(PlatformThreadId thread_id,
-                        const SamplingParams& params,
-                        const CompletedCallback& callback);
-  StackSamplingProfiler(PlatformThreadId thread_id,
-                        const SamplingParams& params,
-                        const CompletedCallback& callback,
-                        NativeStackSamplerTestDelegate* test_delegate);
+  // Creates a profiler for the CURRENT thread that sends completed profiles
+  // to |callback|. An optional |test_delegate| can be supplied by tests.
+  // Ensure that this object gets destroyed before the current thread exits.

[Mike Wittman, 2017/02/15 03:26:44]

nit: The caller must ensure ...

Comments with imperatives directed at the reader are not generally done and will
be confusing to most developers.

[bcwhite, 2017/02/15 16:17:35]

Done.

+  StackSamplingProfiler(
+      const SamplingParams& params,
+      const CompletedCallback& callback,
+      NativeStackSamplerTestDelegate* test_delegate = nullptr);
+
+  // Creates a profiler for ANOTHER thread that sends completed profiles to
+  // |callback|. An optional |test_delegate| can be supplied by tests.
+  //
+  // IMPORTANT: Ensure that the thread being sampled does not exit before this

[Mike Wittman, 2017/02/15 03:26:44]

Same here.

[bcwhite, 2017/02/15 16:17:35]

Done.

+  // object gets destructed or Bad Things(tm) may occur.
+  StackSamplingProfiler(
+      PlatformThreadId thread_id,
+      const SamplingParams& params,
+      const CompletedCallback& callback,
+      NativeStackSamplerTestDelegate* test_delegate = nullptr);
+
   // Stops any profiling currently taking place before destroying the profiler.
+  // This will block until the callback has been run.
   ~StackSamplingProfiler();
 
-  // The fire-and-forget interface: starts a profiler and allows it to complete
-  // without the caller needing to manage the profiler lifetime. May be invoked
-  // from any thread, but requires that the calling thread has a message loop.
-  static void StartAndRunAsync(PlatformThreadId thread_id,
-                               const SamplingParams& params,
-                               const CompletedCallback& callback);
-
   // Initializes the profiler and starts sampling.
   void Start();
 
-  // Stops the profiler and any ongoing sampling. Calling this function is
-  // optional; if not invoked profiling terminates when all the profiling bursts
-  // specified in the SamplingParams are completed or the profiler is destroyed,
-  // whichever occurs first.
+  // Stops the profiler and any ongoing sampling. This method will return
+  // immediately with the callback being run asynchronously. At most one
+  // more stack sample will be taken after this method returns. Calling this
+  // function is optional; if not invoked profiling terminates when all the
+  // profiling bursts specified in the SamplingParams are completed or the
+  // profiler object is destroyed, whichever occurs first.
   void Stop();
 
+  // Returns whether the sampling thread is currently running or not.
+  static bool IsSamplingThreadRunningForTesting();

[Mike Wittman, 2017/02/15 03:26:44]

Can we move these three functions into an internal TestApi class? (See other
examples in the code.)

And also, do the same for the two functions they call in SamplingThread? It's
not obvious what parts of that class are there for test purposes.

[bcwhite, 2017/02/15 16:17:35]

Done.  Let me know if it's what you were thinking.

[Mike Wittman, 2017/02/15 21:56:00]

Looks good. Can you add a TestAPI to SamplingThread as well so we can
distinguish the test support code in that class from the other code?

[bcwhite, 2017/02/16 17:39:49]

I didn't think it was necessary since that class is embedded inside this .cc
file and so has no access from the outside.

[Mike Wittman, 2017/02/17 16:10:19]

Segregating test support code all the way down makes it clear which parts of the
class are core functionality. This makes it easier to understand the important
parts of the code and facilitates refactoring.

[bcwhite, 2017/02/21 16:21:19]

Done.

+
+  // Sets the auto-shutdown delay time for the sampling thread, in ms. Set this
+  // to zero to disable it.
+  static void SetSamplingThreadIdleShutdownTimeForTesting(int shutdown_ms);
+
+  // Initiates an idle shutdown task, as though the idle timer had expired,
+  // causing the thread to exit if there are no more sampling tasks pending.
+  // This returns immediately. Watch IsSamplingThreadRunningForTesting() to
+  // know when the thread has exited, though it will never do so if it is
+  // not idle.
+  static void InitiateSamplingThreadIdleShutdownForTesting();
+
   // Set the current system state that is recorded with each captured stack
   // frame. This is thread-safe so can be called from anywhere. The parameter
   // value should be from an enumeration of the appropriate type with values
@@ -233,44 +255,7 @@ class BASE_EXPORT StackSamplingProfiler {
  private:
   // 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 callable on any thread.
-    SamplingThread(std::unique_ptr<NativeStackSampler> native_sampler,
-                   const SamplingParams& params,
-                   const CompletedCallback& completed_callback);
-    ~SamplingThread() override;
-
-    // PlatformThread::Delegate:
-    void ThreadMain() override;
-
-    void Stop();
-
-   private:
-    // Collects |profile| from a single burst. If the profiler was stopped
-    // during collection, sets |was_stopped| and provides the set of samples
-    // collected up to that point.
-    void CollectProfile(CallStackProfile* profile, TimeDelta* elapsed_time,
-                        bool* was_stopped);
-
-    // Collects call stack profiles from all bursts, or until the sampling is
-    // stopped. If stopped before complete, the last profile in
-    // |call_stack_profiles| may contain a partial burst.
-    void CollectProfiles(CallStackProfiles* profiles);
-
-    std::unique_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_;
-
-    const CompletedCallback completed_callback_;
-
-    DISALLOW_COPY_AND_ASSIGN(SamplingThread);
-  };
+  class SamplingThread;
 
   // Adds annotations to a Sample.
   static void RecordAnnotations(Sample* sample);
@@ -287,11 +272,18 @@ class BASE_EXPORT StackSamplingProfiler {
 
   const SamplingParams params_;
 
-  std::unique_ptr<SamplingThread> sampling_thread_;
-  PlatformThreadHandle sampling_thread_handle_;
-
   const CompletedCallback completed_callback_;
 
+  // An event signaled when all sampling is complete and the callback done.
+  WaitableEvent finished_event_;
+
+  // Object that does the native sampling. This is created during construction
+  // 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.
+  int collection_id_ = -1;
+
   // Stored until it can be passed to the NativeStackSampler created in Start().
   NativeStackSamplerTestDelegate* const test_delegate_;