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..c37453fbdb1cdc6e12085d2e96265f717f852b7b 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
@@ -193,34 +194,69 @@ class BASE_EXPORT StackSamplingProfiler {
   // 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.
+  //
+  // IMPORTANT: This should generally be created on the local stack (i.e. NOT

[Mike Wittman, 2017/02/14 17:52:32]

This guidance is more conservative than necessary. I think it's sufficient to
say that the object must be destroyed before thread exit.

The general expectation in Chrome is that non-singleton objects are destroyed on
the thread where they are created. Not destroying would be a memory leak, so the
restriction for this constructor is fairly unremarkable and probably doesn't
justify an IMPORTANT label.

[bcwhite, 2017/02/14 20:13:27]

Done.

+  // using "new") with the expectation of profiling only until that object
+  // goes out of scope.
+  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: Only threads guaranteed to live beyond the lifetime of the

[Mike Wittman, 2017/02/14 17:52:32]

It would be best to move the prescriptive advice to the top, and drop the text
about the current thread within this comment.

Maybe something like:

IMPORTANT: Users of this interface must ensure the specified thread outlives
this object. Otherwise the profiler will continue trying to profile past thread
exit, resulting in crashes or worse.

[bcwhite, 2017/02/14 20:13:27]

Done.

+  // profiler object can be safely sampled. Sampling the activity of the current
+  // thread within the scope of this object is safe, but sampling any other
+  // thread could cause Bad Things(tm) to occur should that thread exit before
+  // sampling is complete; ensure the profiler object gets destructed before
+  // the exit of the thread under test. USE WITH CAUTION!
+  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();
 
+  // Stops all active profiles and cleans up any resources in anticipation of
+  // a full shutdown of the current process. The thread calling this method
+  // must allow waiting (ThreadRestrictions::AllowWait).
+  static void Shutdown();

[Mike Wittman, 2017/02/14 17:52:32]

It's worth considering process shutdown now that we have a solution for thread
exit.

The destruction of the profilers before profiled thread exit will ensure we
won't be actively profiling at process shutdown, so we don't need to worry about
the profiled threads at shutdown.

The guidance from the thread scheduler team is that we should not be trying to
terminate the profiler thread at process shutdown, because it's just unnecessary
work. So we don't need to do anything special there either.

I'm not aware of anything else that would need to happen at shutdown, so I think
we can remove this function and the next one.

[bcwhite, 2017/02/14 20:13:27]

As long as we assume that the assumptions a thread makes about the lifetime of a
thread under test are still valid during shutdown.

Done.

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

There's a well-defined shut down procedure for process threads within
BrowserMainLoop::ShutdownThreadsAndCleanUp(), so I think this is a reasonable
assumption. A worst case scenario might require some special case code to run
before this function, but I don't anticipate this to be necessary.

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

Acknowledged.

+
+  // Resets the "shutdown" state so that the sampling thread can be restarted.
+  static void UndoShutdownForTesting();
+
+  // Returns whether the sampling thread is currently running or not.
+  static bool IsSamplingThreadRunningForTesting();
+
+  // 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 +269,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 +286,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_;