Support parallel captures from the StackSamplingProfiler.

base/profiler/stack_sampling_profiler.cc

Index: base/profiler/stack_sampling_profiler.cc
diff --git a/base/profiler/stack_sampling_profiler.cc b/base/profiler/stack_sampling_profiler.cc
index d77858427edd6c922a17df7f29e985551640e99d..5b7b924b5d4f7b5b0986891a787145cb605ef3d7 100644
--- a/base/profiler/stack_sampling_profiler.cc
+++ b/base/profiler/stack_sampling_profiler.cc
@@ -5,16 +5,22 @@
 #include "base/profiler/stack_sampling_profiler.h"
 
 #include <algorithm>
+#include <queue>
 #include <utility>
 
+#include "base/atomicops.h"
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/lazy_instance.h"
 #include "base/location.h"
 #include "base/macros.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/singleton.h"
+#include "base/memory/weak_ptr.h"
 #include "base/profiler/native_stack_sampler.h"
 #include "base/synchronization/lock.h"
+#include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/timer/elapsed_timer.h"
 
@@ -22,9 +28,6 @@ namespace base {
 
 namespace {
 
-// Used to ensure only one profiler is running at a time.
-LazyInstance<Lock>::Leaky concurrent_profiling_lock = LAZY_INSTANCE_INITIALIZER;
-
 // AsyncRunner ----------------------------------------------------------------
 
 // Helper class to allow a profiler to be run completely asynchronously from the
@@ -160,102 +163,313 @@ StackSamplingProfiler::CallStackProfile::CallStackProfile(
 
 // StackSamplingProfiler::SamplingThread --------------------------------------
 
-StackSamplingProfiler::SamplingThread::SamplingThread(
-    std::unique_ptr<NativeStackSampler> native_sampler,
-    const SamplingParams& params,
-    const CompletedCallback& completed_callback)
-    : native_sampler_(std::move(native_sampler)),
-      params_(params),
-      stop_event_(WaitableEvent::ResetPolicy::AUTOMATIC,
-                  WaitableEvent::InitialState::NOT_SIGNALED),
-      completed_callback_(completed_callback) {}
-
-StackSamplingProfiler::SamplingThread::~SamplingThread() {}
+class StackSamplingProfiler::SamplingThread : public Thread {
+ public:
+  struct ActiveCapture {
+    ActiveCapture(PlatformThreadId target,
+                  const SamplingParams& params,
+                  const CompletedCallback& callback,
+                  std::unique_ptr<NativeStackSampler> sampler)
+        : capture_id(subtle::NoBarrier_AtomicIncrement(&next_capture_id_, 1)),
+          target(target),
+          params(params),
+          callback(callback),
+          native_sampler(std::move(sampler)),
+          weak_ptr_factory_(this) {}
+    ~ActiveCapture() {}
+
+    // Updates the |next_sample_time| time based on configured parameters.
+    // This will keep a consistent average interval between samples but will
+    // result in constant series of acquisitions, thus nearly locking out the
+    // target thread, if the interval is smaller than the time it takes to
+    // actually acquire the sample. Anything sampling that quickly is going
+    // to be a problem anyway so don't worry about it.
+    bool UpdateNextSampleTime() {
+      if (stopped)
+        return false;
+
+      if (++sample < params.samples_per_burst) {
+        next_sample_time += params.sampling_interval;
+        return true;
+      }
 
-void StackSamplingProfiler::SamplingThread::ThreadMain() {
-  PlatformThread::SetName("Chrome_SamplingProfilerThread");
+      if (++burst < params.bursts) {
+        sample = 0;
+        next_sample_time += params.burst_interval;
+        return true;
+      }
 
-  // For now, just ignore any requests to profile while another profiler is
-  // working.
-  if (!concurrent_profiling_lock.Get().Try())
-    return;
+      return false;
+    }
 
-  CallStackProfiles profiles;
-  CollectProfiles(&profiles);
-  concurrent_profiling_lock.Get().Release();
-  completed_callback_.Run(std::move(profiles));
-}
-
-// Depending on how long the sampling takes and the length of the sampling
-// interval, a burst of samples could take arbitrarily longer than
-// samples_per_burst * sampling_interval. In this case, we (somewhat
-// arbitrarily) honor the number of samples requested rather than strictly
-// adhering to the sampling intervals. Once we have established users for the
-// StackSamplingProfiler and the collected data to judge, we may go the other
-// way or make this behavior configurable.
-void StackSamplingProfiler::SamplingThread::CollectProfile(
-    CallStackProfile* profile,
-    TimeDelta* elapsed_time,
-    bool* was_stopped) {
-  ElapsedTimer profile_timer;
-  native_sampler_->ProfileRecordingStarting(&profile->modules);
-  profile->sampling_period = params_.sampling_interval;
-  *was_stopped = false;
-  TimeDelta previous_elapsed_sample_time;
-  for (int i = 0; i < params_.samples_per_burst; ++i) {
-    if (i != 0) {
-      // Always wait, even if for 0 seconds, so we can observe a signal on
-      // stop_event_.
-      if (stop_event_.TimedWait(
-              std::max(params_.sampling_interval - previous_elapsed_sample_time,
-                       TimeDelta()))) {
-        *was_stopped = true;
-        break;
-      }
+    WeakPtr<ActiveCapture> GetWeakPtr() {
+      return weak_ptr_factory_.GetWeakPtr();
     }
-    ElapsedTimer sample_timer;
-    profile->samples.push_back(Sample());
-    native_sampler_->RecordStackSample(&profile->samples.back());
-    previous_elapsed_sample_time = sample_timer.Elapsed();
+
+    // An identifier for this capture, used to uniquely identify it to outside
+    // interests.
+    const int capture_id;
+
+    Time next_sample_time;
+
+    PlatformThreadId target;
+    SamplingParams params;
+    CompletedCallback callback;
+
+    std::unique_ptr<NativeStackSampler> native_sampler;
+
+    // Counters that indicate the current position along the acquisition.
+    int burst = 0;
+    int sample = 0;
+
+    // Indicates if the capture has been stopped (and reported).
+    bool stopped = false;
+
+    // The time that a profile was started, for calculating the total duration.
+    Time profile_start_time;
+
+    // The captured stack samples. The active profile is always at the back().
+    CallStackProfiles profiles;
+
+   private:
+    static subtle::AtomicWord next_capture_id_;
+
+    WeakPtrFactory<ActiveCapture> weak_ptr_factory_;
+  };
+
+  // Gets the single instance of this class.
+  static SamplingThread* GetInstance();
+
+  // Starts the thread.
+  void Start();
+
+  // Adds a new ActiveCapture to the thread. This can be called externally
+  // from any thread. This returns an ID that can later be used to stop
+  // the sampling.
+  int Add(std::unique_ptr<ActiveCapture> capture);
+
+  // Stops an active capture based on its ID, forcing it to run its callback
+  // if any data has been collected. This can be called externally from any
+  // thread.
+  void Stop(int id);
+
+ private:
+  SamplingThread();
+  ~SamplingThread() override;
+  friend struct DefaultSingletonTraits<SamplingThread>;
+
+  // These methods are called when a new capture begins, when it is
+  // finished, and for each individual sample, respectively.
+  void BeginCapture(ActiveCapture* capture);
+  void FinishCapture(ActiveCapture* capture);
+  void PerformCapture(ActiveCapture* capture);

[Mike Wittman, 2016/12/09 21:45:02]

The term "capture" is overloaded in the method names to mean both the recording
of all the samples and the recording of a single sample. Can we use something
like "record sample" for the latter case to be consistent with the
NativeStackSampler? e.g. this becomes something like RecordSampleForCapture.

[bcwhite, 2016/12/15 18:07:50]

Done, though shortened to just Begin/End/PerformRecording.

[Mike Wittman, 2016/12/15 20:37:53]

This still has the same issue: "recording" is used to refer to both the
recording of one sample and all the samples. It's also not clear what the
relationship between "capture", "recording", and "collection" is; all three
terms are used variously in code and comments.

Can we regularize all this terminology? My suggestion:
- use "record" or "record sample" to refer to the recording of one stack/sample
- use "collection" to refer to the collection of all the samples for one
request.

With that, these functions become BeginCollection, EndCollection, RecordSample.
ActiveCapture becomes ActiveCollection, or better, CollectionContext since that
makes clear it's strictly state associated with a collection and not behavior.

[bcwhite, 2016/12/21 16:39:10]

Done.

+
+  // These methods are tasks that get posted to the internal message queue.
+  void StartCaptureTask(std::unique_ptr<ActiveCapture> capture);
+  void StopCaptureTask(int id);
+  void PerformCaptureTask(std::unique_ptr<ActiveCapture> capture);
+
+  // Thread:
+  void Init() override;
+
+  static constexpr int kMinimumThreadRunTimeSeconds = 60;
+
+  // A map of IDs to active captures. This is a weak-pointer because it's
+  // possible that objects are deleted because their collection is complete.
+  std::map<int, WeakPtr<ActiveCapture>> active_captures_;
+
+  DISALLOW_COPY_AND_ASSIGN(SamplingThread);
+};
+
+subtle::AtomicWord
+    StackSamplingProfiler::SamplingThread::ActiveCapture::next_capture_id_ = 0;
+
+StackSamplingProfiler::SamplingThread::SamplingThread()
+    : Thread("Chrome_SamplingProfilerThread") {}
+
+StackSamplingProfiler::SamplingThread::~SamplingThread() {
+  Thread::Stop();
+}
+
+StackSamplingProfiler::SamplingThread*
+StackSamplingProfiler::SamplingThread::GetInstance() {
+  return Singleton<SamplingThread>::get();
+}
+
+void StackSamplingProfiler::SamplingThread::Start() {
+  Thread::Options options;
+  // Use a higher priority for a more accurate sampling interval.
+  options.priority = ThreadPriority::DISPLAY;
+
+  Thread::StartWithOptions(options);
+}
+
+int StackSamplingProfiler::SamplingThread::Add(
+    std::unique_ptr<ActiveCapture> capture) {
+  int id = capture->capture_id;
+
+  scoped_refptr<SingleThreadTaskRunner> runner = task_runner();

[Mike Wittman, 2016/12/09 21:45:01]

According to the Thread documentation, task_runner() can only be safely called
from the thread that invokes Start().

Thread's API is not thread-safe in general, so care should be taken to ensure
that it's only used from the proper threads, including making liberal use of
DCHECKs/ThreadChecker since it's non-trivial to validate from reading the code.

The same goes for ensuring execution on proper threads in other functions in
this class (and for documenting thread expectations to the reader).

[bcwhite, 2016/12/09 23:38:30]

The comment for Thread::task_runner() says:
  // In addition to this Thread's owning sequence, this can also safely be
  // called from the underlying thread itself.
There's a DCHECK in Thread::task_runner() that verifies this.

[Mike Wittman, 2016/12/10 00:24:23]

Right, but Add() will never be called on the thread itself, correct?

If I'm not mistaken task_runner() will be invoked on a thread other than the
thread itself and the one that called Start(), once a second thread attempts to
profile itself concurrently.

Unit tests for the concurrency functionality will help catch this type of issue.

[bcwhite, 2016/12/13 16:08:11]

Ah, I understand.  So if this is called from other than the thread that started
it, I need to post-task to the thread that started it... which will then
post-task to the worker thread.

But is it possible that whatever thread started it has the same restrictions and
won't allow posts from just anywhere?

[Mike Wittman, 2016/12/13 18:16:41]

It may be possible to call task_runner() on the Start thread, then maintain an
instance of the scoped_refptr<SingleThreadTaskRunner> as a member variable on
SamplingThread, since SingleThreadTaskRunner is thread-safe refcounted type.

Delaying the quit-when-idle behavior to a follow on CL hopefully should make
this a little easier to deal with.

[bcwhite, 2016/12/14 15:37:59]

Wasn't there an issue with a task-runner not being available for the Startup
collection?  Why the callback had to be direct instead of a posted task?

This is getting complicated.  Are you sure it's not easier and less prone to
problems to just use a lock and some shared data structures?

[Mike Wittman, 2016/12/14 18:00:47]

The UI thread does not have a task runner when it starts the profiler because it
hasn't started its message loop at that point. The profiler thread doesn't have
this issue because its message loop starts with the thread.
This shouldn't be significantly more complicated than the other places in Chrome
where threads are started for the purpose of other threads posting tasks to
them. The one extra factor here is the desire to start the thread from an
arbitrary thread. If that proves to be not workable, I think we can fall back to
requesting that the profiling thread be started on a specific thread (e.g. the
UI thread).

[bcwhite, 2016/12/14 19:39:39]

I must be missing something.  The SamplingThread's task-runner is postable by
only the SamplingThread and whatever thread created it.

The very first thing that will start a capture (and create the SamplingThread)
doesn't have a task-runner at that point so there is no way to get a refptr to
it for future use.  And even if it did, how would you know it would be postable
from any arbitrary thread?

[Mike Wittman, 2016/12/14 20:47:50]

I don't think this is correct. Thread::task_runner() can only be called on those
two threads, but I believe if the scoped_refptr<SingleThreadTaskRunner> returned
by that function is saved somewhere, other threads can use it to post tasks. The
scoped_refptr is thread-safe and all methods on TaskRunner are also thread-safe.

In Chrome for example, the IO thread is created by the UI thread, but other
threads can post tasks directly to it. So there must already be a defined
mechanism to do this.
Why can't the thread that starts the SamplingThread get the TaskRunner via the
task_runner() accessor after calling Start()?

[bcwhite, 2016/12/15 11:42:15]

Ah!  So _fetching_ the task-runner must be done on one of those two threads but,
once fetched, it can be _used_ from any thread?  I had assumed that the 2-thread
limitation applied to the latter.  This makes better sense since it seemed odd
to have restrictions about which threads could post to a queue.

[bcwhite, 2016/12/15 15:01:16]

It's going to still be necessary to have a lock, this time to protect the saved
task-runner pointer.  Since the thread is started on-demand rather than in the
(single-threaded) constructor, there could be other threads trying to read that
pointer at the same moment that it is being set.

The same lock will protect against having multiple Start() calls from attempting
to launch the sampling thread.

[Mike Wittman, 2016/12/15 17:22:46]

Yeah, I'm not surprised we can't completely avoid synchronization. Doing it in
this form will at least be fairly limited in scope.

+  if (!runner) {
+    // The thread is not running. Start it and try again.
+    Start();
+    runner = task_runner();
+    DCHECK(runner);
+  }
+
+  // Having the task-runner doesn't prevent the thread from exiting between
+  // when it was acquired and the PostTask below. If that were to happen, the
+  // PostTask would fail and no capture would be done. A retry isn't possible
+  // because the |capture| object was passed and thus no longer exists locally.
+  // To prevent this, first create a dummy task that is delayed long enough
+  // that the real task is, for all practical purposes, guaranteed to get
+  // queued.
+  if (!runner->PostDelayedTask(
+          FROM_HERE, Bind(&DoNothing),
+          TimeDelta::FromSeconds(kMinimumThreadRunTimeSeconds))) {
+    // The thread must have exited. Restart it and get the new task runner.
+    Start();
+    runner = task_runner();
+    DCHECK(runner);
+  }
+
+  bool success =
+      runner->PostTask(FROM_HERE, Bind(&SamplingThread::StartCaptureTask,

[Mike Wittman, 2016/12/09 21:45:02]

It's common practice to implement thread hopping using just one function,
checking whether the execution is on the desired thread at the start of the
function, and if not, posting a task back to the same function on the desired
thread. I think that could be done here by checking the thread id, and probably
would make the code a little easier to follow.

[bcwhite, 2016/12/09 23:38:30]

Add() and Stop() are always coming from a different thread.  The
StartCaptureTask() could be merged into the same method but I think that would
be more confusing because of all the work done above to make sure the thread is
actually running.

[Mike Wittman, 2016/12/10 00:24:23]

I think it's worth doing this for Stop() at least. The other benefit of the
one-function thread hop implementation is that it clearly documents/enforces
threading expectations for an operation in a single location.

[bcwhite, 2016/12/13 16:08:11]

I can see that.  The downside is that there are then two different styles for
methods of the same class.

+                                       Unretained(this), Passed(&capture)));
+  DCHECK(success);
+
+  return id;
+}
+
+void StackSamplingProfiler::SamplingThread::Stop(int id) {
+  scoped_refptr<SingleThreadTaskRunner> runner = task_runner();

[Mike Wittman, 2016/12/09 21:45:01]

Same issue with task_runner() here.

[bcwhite, 2016/12/15 18:07:50]

Done.

+  if (!runner)
+    return;  // Everything has already stopped.
+
+  // This can fail if the thread were to exit between acquisition of the task
+  // runner above and the call below. In that case, however, everything has
+  // stopped so there's no need to try to stop it.
+  runner->PostTask(
+      FROM_HERE, Bind(&SamplingThread::StopCaptureTask, Unretained(this), id));

[Mike Wittman, 2016/12/09 21:45:02]

Same comment here about thread hopping.

+}
+
+void StackSamplingProfiler::SamplingThread::BeginCapture(
+    ActiveCapture* capture) {
+  DCHECK(capture->native_sampler);
+}
+
+void StackSamplingProfiler::SamplingThread::FinishCapture(
+    ActiveCapture* capture) {
+  DCHECK(!capture->stopped);
+  capture->stopped = true;
+
+  // If there is no duration for the final profile (because it was stopped),
+  // calculated it now.
+  if (!capture->profiles.empty() &&
+      capture->profiles.back().profile_duration == TimeDelta()) {
+    capture->profiles.back().profile_duration =
+        Time::Now() - capture->profile_start_time;
   }
 
-  *elapsed_time = profile_timer.Elapsed();
-  profile->profile_duration = *elapsed_time;
-  native_sampler_->ProfileRecordingStopped();
+  // Run the associated callback, passing the captured profiles. It's okay to
+  // move them because this capture is about to be deleted.
+  capture->callback.Run(std::move(capture->profiles));
 }
 
-// In an analogous manner to CollectProfile() and samples exceeding the expected
-// total sampling time, bursts may also exceed the burst_interval. We adopt the
-// same wait-and-see approach here.
-void StackSamplingProfiler::SamplingThread::CollectProfiles(
-    CallStackProfiles* profiles) {
-  if (stop_event_.TimedWait(params_.initial_delay))
+void StackSamplingProfiler::SamplingThread::PerformCapture(
+    ActiveCapture* capture) {
+  DCHECK(!capture->stopped);
+
+  // If this is the first sample of a burst, a new Profile needs to be created
+  // and filled.
+  if (capture->sample == 0) {
+    capture->profiles.push_back(CallStackProfile());
+    CallStackProfile& profile = capture->profiles.back();
+    profile.sampling_period = capture->params.sampling_interval;
+    capture->profile_start_time = Time::Now();
+    capture->native_sampler->ProfileRecordingStarting(&profile.modules);
+  }
+
+  // The currently active profile being acptured.
+  CallStackProfile& profile = capture->profiles.back();
+
+  // Capture a single sample.
+  profile.samples.push_back(Sample());
+  capture->native_sampler->RecordStackSample(&profile.samples.back());
+
+  // If this is the last sample of a burst, record the total time.
+  if (capture->sample == capture->params.samples_per_burst - 1) {
+    profile.profile_duration = Time::Now() - capture->profile_start_time;
+    capture->native_sampler->ProfileRecordingStopped();
+  }
+}
+
+void StackSamplingProfiler::SamplingThread::StartCaptureTask(
+    std::unique_ptr<ActiveCapture> capture) {
+  active_captures_.insert(
+      std::make_pair(capture->capture_id, capture->GetWeakPtr()));
+  BeginCapture(capture.get());
+  bool success = task_runner()->PostDelayedTask(
+      FROM_HERE, Bind(&SamplingThread::PerformCaptureTask, Unretained(this),
+                      Passed(&capture)),
+      capture->params.initial_delay);
+  DCHECK(success);
+}
+
+void StackSamplingProfiler::SamplingThread::StopCaptureTask(int id) {
+  auto found = active_captures_.find(id);
+  if (found == active_captures_.end())
+    return;  // Gone and forgotten.
+
+  ActiveCapture* capture = found->second.get();
+  if (!capture)
+    return;  // Gone but not forgotten.
+
+  if (capture->stopped)
     return;
 

[Mike Wittman, 2016/12/09 21:45:01]

where does the capture get erased from active_captures_?

[bcwhite, 2016/12/09 23:38:30]

In ::Cleanup()
... which I realized after uploading that forgot to write.  :-)

-  TimeDelta previous_elapsed_profile_time;
-  for (int i = 0; i < params_.bursts; ++i) {
-    if (i != 0) {
-      // Always wait, even if for 0 seconds, so we can observe a signal on
-      // stop_event_.
-      if (stop_event_.TimedWait(
-              std::max(params_.burst_interval - previous_elapsed_profile_time,
-                       TimeDelta())))
-        return;
-    }
+  FinishCapture(capture);
+}
 
-    CallStackProfile profile;
-    bool was_stopped = false;
-    CollectProfile(&profile, &previous_elapsed_profile_time, &was_stopped);
-    if (!profile.samples.empty())
-      profiles->push_back(std::move(profile));
+void StackSamplingProfiler::SamplingThread::PerformCaptureTask(
+    std::unique_ptr<ActiveCapture> capture) {
+  DCHECK(capture);
+  // Don't do anything if the capture has already stopped.
+  if (capture->stopped)
+    return;
 
-    if (was_stopped)
-      return;
+  // Handle first-run with no "next time".
+  if (capture->next_sample_time == Time())
+    capture->next_sample_time = Time::Now();
+
+  // Do the collection of a single sample.
+  PerformCapture(capture.get());
+
+  // Update the time of the next capture.
+  if (capture->UpdateNextSampleTime()) {
+    // Place the updated entry back on the queue.
+    bool success = task_runner()->PostDelayedTask(
+        FROM_HERE, Bind(&SamplingThread::PerformCaptureTask, Unretained(this),
+                        Passed(&capture)),
+        std::max(capture->next_sample_time - Time::Now(), TimeDelta()));
+    DCHECK(success);
+  } else {
+    // All capturing has completed so finish the collection. Let object expire.
+    FinishCapture(capture.get());
   }
 }
 
-void StackSamplingProfiler::SamplingThread::Stop() {
-  stop_event_.Signal();
+void StackSamplingProfiler::SamplingThread::Init() {
+  // Let the parent initialize.
+  Thread::Init();
+
+  // Create a dummy task so that the thread won't exit for at least some
+  // minimum amount of time. Otherwise, the thread could exit just after
+  // starting and before a caller has time to start the real work.
+  DCHECK(task_runner());
+  bool success = task_runner()->PostDelayedTask(
+      FROM_HERE, Bind(&DoNothing),
+      TimeDelta::FromSeconds(kMinimumThreadRunTimeSeconds));

[Mike Wittman, 2016/12/09 21:45:01]

My understanding is that the message loop just waits if no tasks are present. I
believe it must be forcibly quit or its thread shut down to terminate it.

[bcwhite, 2016/12/09 23:38:30]

Correct.  My idea is to add the ability for it to self-destruct when "empty"
(which is not the same thing as "idle").

I didn't think it was possible for at outside class like this one to tell if the
message_loop is empty, but perhaps it can -- I'll have to check.  If so, then I
can add a check at the end of every task to terminate the loop if it is empty.

[Mike Wittman, 2016/12/10 00:24:23]

I think that will be confusing to readers since it will operate differently than
all the other message loops in the application. Couldn't this be addressed by
posting a delayed quit task when the number of captures drops to zero (and
canceling the task if the number of captures becomes non-zero)?

[bcwhite, 2016/12/13 16:08:11]

Message looks are already RunForever or RunUntilIdle.  Adding RunUntilEmpty
seems a natural (and generally useful) extension.

Posting a delayed quit is still a race condition because a new task could get
posted from another thread just as the quit starts executing.

It's possible that's a problem no matter what the solution.  Even RunUntilEmpty
may have that issue -- I'd have to investigate further.  There may need to be
some sort of atomic operation, such as a simple counter, no matter what.

Given the variations, I think it would be best to leave it as "run forever" in
this CL and do the quit-when-idle as a follow-up CL.

[Mike Wittman, 2016/12/13 18:16:41]

RunForever is pretty much the only mode that's used in Chrome itself;
RunUntilIdle is used almost exclusively for testing. I believe RunUntilIdle is
generally considered an anti-pattern in production code because of its action at
a distance properties -- anyone else in the system (including the OS) can
unintentionally keep the message loop alive by posting messages to it. There's
only a dozen instances of RunUntilIdle in actual Chrome code, all of which are
in highly constrained scenarios:
https://cs.chromium.org/search/?q=rununtilidle%5C(%5C);+file:%5C.cc$+-file:te...

RunUntilEmpty will be subject to the same issues, I think.
Deferring the quit-when-idle behavior to a follow-on CL SGTM. I suspect there
will be more than enough complexity to deal with just implementing the "run
forever" mode.

[bcwhite, 2016/12/15 18:07:50]

Acknowledged.

+  DCHECK(success);
 }
 
 // StackSamplingProfiler ------------------------------------------------------
@@ -287,8 +501,6 @@ StackSamplingProfiler::StackSamplingProfiler(
 
 StackSamplingProfiler::~StackSamplingProfiler() {
   Stop();
-  if (!sampling_thread_handle_.is_null())
-    PlatformThread::Join(sampling_thread_handle_);
 }
 
 // static
@@ -310,16 +522,13 @@ void StackSamplingProfiler::Start() {
   if (!native_sampler)
     return;
 
-  sampling_thread_.reset(new SamplingThread(std::move(native_sampler), params_,
-                                            completed_callback_));
-  if (!PlatformThread::Create(0, sampling_thread_.get(),
-                              &sampling_thread_handle_))
-    sampling_thread_.reset();
+  capture_id_ = SamplingThread::GetInstance()->Add(
+      MakeUnique<SamplingThread::ActiveCapture>(
+          thread_id_, params_, completed_callback_, std::move(native_sampler)));
 }
 
 void StackSamplingProfiler::Stop() {
-  if (sampling_thread_)
-    sampling_thread_->Stop();
+  SamplingThread::GetInstance()->Stop(capture_id_);
 }
 
 // static