Support parallel captures from the StackSamplingProfiler.

base/profiler/stack_sampling_profiler.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/profiler/stack_sampling_profiler.h"
 
 #include <algorithm>
+#include <map>
 #include <utility>
 
+#include "base/atomic_sequence_num.h"
+#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/profiler/native_stack_sampler.h"
 #include "base/synchronization/lock.h"
+#include "base/threading/thread.h"
+#include "base/threading/thread_restrictions.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/timer/elapsed_timer.h"
 
 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
-// initiator, without being concerned with the profiler's lifetime.
-class AsyncRunner {
- public:
-  // Sets up a profiler and arranges for it to be deleted on its completed
-  // callback.
-  static void Run(PlatformThreadId thread_id,
-                  const StackSamplingProfiler::SamplingParams& params,
-                  const StackSamplingProfiler::CompletedCallback& callback);
-
- private:
-  AsyncRunner();
-
-  // Runs the callback and deletes the AsyncRunner instance. |profiles| is not
-  // const& because it must be passed with std::move.
-  static void RunCallbackAndDeleteInstance(
-      std::unique_ptr<AsyncRunner> object_to_be_deleted,
-      const StackSamplingProfiler::CompletedCallback& callback,
-      scoped_refptr<SingleThreadTaskRunner> task_runner,
-      StackSamplingProfiler::CallStackProfiles profiles);
-
-  std::unique_ptr<StackSamplingProfiler> profiler_;
-
-  DISALLOW_COPY_AND_ASSIGN(AsyncRunner);
-};
-
-// static
-void AsyncRunner::Run(
-    PlatformThreadId thread_id,
-    const StackSamplingProfiler::SamplingParams& params,
-    const StackSamplingProfiler::CompletedCallback &callback) {
-  std::unique_ptr<AsyncRunner> runner(new AsyncRunner);
-  AsyncRunner* temp_ptr = runner.get();
-  temp_ptr->profiler_.reset(
-      new StackSamplingProfiler(thread_id, params,
-                                Bind(&AsyncRunner::RunCallbackAndDeleteInstance,
-                                     Passed(&runner), callback,
-                                     ThreadTaskRunnerHandle::Get())));
-  // The callback won't be called until after Start(), so temp_ptr will still
-  // be valid here.
-  temp_ptr->profiler_->Start();
-}
-
-AsyncRunner::AsyncRunner() {}
-
-void AsyncRunner::RunCallbackAndDeleteInstance(
-    std::unique_ptr<AsyncRunner> object_to_be_deleted,
-    const StackSamplingProfiler::CompletedCallback& callback,
-    scoped_refptr<SingleThreadTaskRunner> task_runner,
-    StackSamplingProfiler::CallStackProfiles profiles) {
-  callback.Run(std::move(profiles));
-  // Delete the instance on the original calling thread.
-  task_runner->DeleteSoon(FROM_HERE, object_to_be_deleted.release());
-}
-
 void ChangeAtomicFlags(subtle::Atomic32* flags,
                        subtle::Atomic32 set,
                        subtle::Atomic32 clear) {
   DCHECK(set != 0 || clear != 0);
   DCHECK_EQ(0, set & clear);
 
   subtle::Atomic32 bits = subtle::NoBarrier_Load(flags);
   while (true) {
     subtle::Atomic32 existing =
         subtle::NoBarrier_CompareAndSwap(flags, bits, (bits | set) & ~clear);
@@ skipping 58 matching lines @@
 StackSamplingProfiler::CallStackProfile
 StackSamplingProfiler::CallStackProfile::CopyForTesting() const {
   return CallStackProfile(*this);
 }
 
 StackSamplingProfiler::CallStackProfile::CallStackProfile(
     const CallStackProfile& other) = default;
 
 // 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() {}
-
-void StackSamplingProfiler::SamplingThread::ThreadMain() {
-  PlatformThread::SetName("Chrome_SamplingProfilerThread");
-
-  // For now, just ignore any requests to profile while another profiler is
-  // working.
-  if (!concurrent_profiling_lock.Get().Try())
-    return;
-
-  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;
-      }
-    }
-    ElapsedTimer sample_timer;
-    profile->samples.push_back(Sample());
-    native_sampler_->RecordStackSample(&profile->samples.back());
-    previous_elapsed_sample_time = sample_timer.Elapsed();
-  }
-
-  *elapsed_time = profile_timer.Elapsed();
-  profile->profile_duration = *elapsed_time;
-  native_sampler_->ProfileRecordingStopped();
-}
-
-// 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))
-    return;
-
-  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;
-    }
-
-    CallStackProfile profile;
-    bool was_stopped = false;
-    CollectProfile(&profile, &previous_elapsed_profile_time, &was_stopped);
-    if (!profile.samples.empty())
-      profiles->push_back(std::move(profile));
-
-    if (was_stopped)
-      return;
-  }
-}
-
-void StackSamplingProfiler::SamplingThread::Stop() {
-  stop_event_.Signal();
+class StackSamplingProfiler::SamplingThread : public Thread {
+ public:
+  struct CollectionContext {
+    CollectionContext(PlatformThreadId target,
+                      const SamplingParams& params,
+                      const CompletedCallback& callback,
+                      WaitableEvent* finished,
+                      NativeStackSampler* sampler)
+        : collection_id(next_collection_id_.GetNext()),
+          target(target),
+          params(params),
+          callback(callback),
+          finished(finished),
+          native_sampler(sampler) {}
+    ~CollectionContext() {}
+
+    // An identifier for this collection, used to uniquely identify it to
+    // outside interests.
+    const int collection_id;
+
+    const PlatformThreadId target;     // ID of The thread being sampled.
+    const SamplingParams params;       // Information about how to sample.
+    const CompletedCallback callback;  // Callback made when sampling complete.
+    WaitableEvent* const finished;     // Signaled when all sampling complete.
+
+    // Platform-specific module that does the actual sampling. Ownership stays
+    // with the parent StackSamplingProfiler object.
+    NativeStackSampler* const native_sampler;
+
+    // The absolute time for the next sample.
+    Time next_sample_time;
+
+    // The time that a profile was started, for calculating the total duration.
+    Time profile_start_time;
+
+    // Counters that indicate the current position along the acquisition.
+    int burst = 0;
+    int sample = 0;
+
+    // The collected stack samples. The active profile is always at the back().
+    CallStackProfiles profiles;
+
+   private:
+    static StaticAtomicSequenceNumber next_collection_id_;
+  };
+
+  // Gets the single instance of this class.
+  static SamplingThread* GetInstance();
+
+  // Starts the thread.
+  void Start();
+
+  // Adds a new CollectionContext 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<CollectionContext> collection);
+
+  // Removes an active collection 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 Remove(int id);
+
+  // Begins an idle shutdown as if the idle-timer had expired.
+  void ShutdownIfIdle();
+
+  // Sets the number of ms to wait after becoming idle before shutting down.
+  // Set to zero to disable.
+  void SetIdleShutdownTime(int shutdown_ms);
+
+ private:
+  SamplingThread();
+  ~SamplingThread() override;
+  friend struct DefaultSingletonTraits<SamplingThread>;
+
+  // Get task runner that is usable from the outside.
+  scoped_refptr<SingleThreadTaskRunner> GetOrCreateTaskRunner();
+  scoped_refptr<SingleThreadTaskRunner> GetTaskRunner();
+
+  // Get task runner that is usable from the sampling thread itself.
+  scoped_refptr<SingleThreadTaskRunner> GetTaskRunnerOnSamplingThread();
+
+  // Finishes a collection and reports collected data via callback.
+  void FinishCollection(CollectionContext* collection);
+
+  // Records a single sample of a collection.
+  void RecordSample(CollectionContext* collection);
+
+  // Check if the sampling thread is idle and begin a shutdown if so.
+  void ScheduleShutdownIfIdle();
+
+  // These methods are tasks that get posted to the internal message queue.
+  void AddCollectionTask(std::unique_ptr<CollectionContext> collection);
+  void RemoveCollectionTask(int id);
+  void PerformCollectionTask(int id);
+  void ShutdownTask(int create_requests, bool second);
+
+  // Updates the |next_sample_time| time based on configured parameters.
+  bool UpdateNextSampleTime(CollectionContext* collection);
+
+  // Thread:
+  void CleanUp() override;
+
+  // The task-runner for the sampling thread and some information about it.
+  // This must always be accessed while holding the lock. The saved task-runner
+  // can be freely used by any calling thread.
+  scoped_refptr<SingleThreadTaskRunner> task_runner_;
+  int task_runner_create_requests_ = 0;
+  TimeDelta task_runner_idle_shutdown_time_ = TimeDelta::FromSeconds(60);
+  Lock task_runner_lock_;
+
+  // A map of IDs to collection contexts. Because this class is a singleton
+  // that is never destroyed, context objects will never be destructed except
+  // by explicit action. Thus, it's acceptable to pass unretained pointers
+  // to these objects when posting tasks.
+  std::map<int, std::unique_ptr<CollectionContext>> active_collections_;
+
+  DISALLOW_COPY_AND_ASSIGN(SamplingThread);
+};
+
+StaticAtomicSequenceNumber StackSamplingProfiler::SamplingThread::
+    CollectionContext::next_collection_id_;
+
+StackSamplingProfiler::SamplingThread::SamplingThread()
+    : Thread("Chrome_SamplingProfilerThread") {}
+
+StackSamplingProfiler::SamplingThread::~SamplingThread() {
+  Stop();
+}
+
+StackSamplingProfiler::SamplingThread*
+StackSamplingProfiler::SamplingThread::GetInstance() {
+  return Singleton<SamplingThread, LeakySingletonTraits<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<CollectionContext> collection) {
+  int id = collection->collection_id;
+  scoped_refptr<SingleThreadTaskRunner> task_runner = GetOrCreateTaskRunner();
+
+  task_runner->PostTask(FROM_HERE, Bind(&SamplingThread::AddCollectionTask,
+                                        Unretained(this), Passed(&collection)));
+
+  return id;
+}
+
+void StackSamplingProfiler::SamplingThread::Remove(int id) {
+  scoped_refptr<SingleThreadTaskRunner> task_runner = GetTaskRunner();
+  if (!task_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.
+  task_runner->PostTask(FROM_HERE, Bind(&SamplingThread::RemoveCollectionTask,
+                                        Unretained(this), id));
+}
+
+void StackSamplingProfiler::SamplingThread::ShutdownIfIdle() {
+  scoped_refptr<SingleThreadTaskRunner> task_runner = GetTaskRunner();
+  if (!task_runner)
+    return;  // Everything has already stopped.
+
+  // ShutdownTask will check if the thread is idle and skip the shutdown if not.
+  AutoLock lock(task_runner_lock_);
+  task_runner->PostTask(FROM_HERE,
+                        Bind(&SamplingThread::ShutdownTask, Unretained(this),
+                             task_runner_create_requests_, /*second=*/false));
+}
+
+void StackSamplingProfiler::SamplingThread::SetIdleShutdownTime(
+    int shutdown_ms) {
+  AutoLock lock(task_runner_lock_);
+  task_runner_idle_shutdown_time_ = TimeDelta::FromMilliseconds(shutdown_ms);
+}
+
+scoped_refptr<SingleThreadTaskRunner>
+StackSamplingProfiler::SamplingThread::GetOrCreateTaskRunner() {
+  AutoLock lock(task_runner_lock_);
+  ++task_runner_create_requests_;
+  if (!task_runner_) {
+    // If this is not the first time the sampling thread has been launched, the
+    // previous instance has only been partially cleaned up. It is necessary
+    // to call Stop() before Start(). This is safe even the thread has never
+    // been started.
+    Stop();
+    // The thread is not running. Start it and get associated runner. The task-
+    // runner has to be saved for future use because though it can be used from
+    // any thread, it can be acquired via task_runner() only on the created
+    // thread and the thread that creates it (i.e. this thread).
+    Start();
+    task_runner_ = Thread::task_runner();
+    // Detach the sampling thread from the "sequence" (i.e. thread) that
+    // started it so that it can be self-managed or stopped by another thread.
+    DetachFromSequence();
+  } else {
+    // This shouldn't be called from the sampling thread as it's inefficient.
+    // Use GetTaskRunnerOnSamplingThread() instead.
+    DCHECK_NE(GetThreadId(), PlatformThread::CurrentId());
+  }
+
+  return task_runner_;
+}
+
+scoped_refptr<SingleThreadTaskRunner>
+StackSamplingProfiler::SamplingThread::GetTaskRunner() {
+  AutoLock lock(task_runner_lock_);
+
+  // GetThreadId() will block for the thread to start so don't check if there
+  // is no task-runner. Otherwise, methods like Remove() will wait forever if
+  // the thread isn't already running when they could just return.
+  if (task_runner_) {
+    // This shouldn't be called from the sampling thread as it's inefficient.
+    // Use GetTaskRunnerOnSamplingThread() instead.
+    DCHECK_NE(GetThreadId(), PlatformThread::CurrentId());
+  }
+
+  return task_runner_;
+}
+
+scoped_refptr<SingleThreadTaskRunner>
+StackSamplingProfiler::SamplingThread::GetTaskRunnerOnSamplingThread() {
+  // This should be called only from the sampling thread as it has limited
+  // accessibility.
+  DCHECK_EQ(GetThreadId(), PlatformThread::CurrentId());
+
+  return Thread::task_runner();
+}
+
+void StackSamplingProfiler::SamplingThread::FinishCollection(
+    CollectionContext* collection) {
+  // If there is no duration for the final profile (because it was stopped),
+  // calculate it now.
+  if (!collection->profiles.empty() &&
+      collection->profiles.back().profile_duration == TimeDelta()) {
+    collection->profiles.back().profile_duration =
+        Time::Now() - collection->profile_start_time;
+  }
+
+  // Run the associated callback, passing the collected profiles. It's okay to
+  // move them because this collection is about to be deleted.
+  collection->callback.Run(std::move(collection->profiles));
+
+  // Signal that this collection is finished.
+  collection->finished->Signal();
+
+  // Remove this collection from the map of known ones.  This must be done
+  // last as the |collection| parameter is invalid after this point.
+  size_t count = active_collections_.erase(collection->collection_id);
+  DCHECK_EQ(1U, count);
+}
+
+void StackSamplingProfiler::SamplingThread::RecordSample(
+    CollectionContext* collection) {
+  DCHECK(collection->native_sampler);
+
+  // If this is the first sample of a burst, a new Profile needs to be created
+  // and filled.
+  if (collection->sample == 0) {
+    collection->profiles.push_back(CallStackProfile());
+    CallStackProfile& profile = collection->profiles.back();
+    profile.sampling_period = collection->params.sampling_interval;
+    collection->profile_start_time = Time::Now();
+    collection->native_sampler->ProfileRecordingStarting(&profile.modules);
+  }
+
+  // The currently active profile being captured.
+  CallStackProfile& profile = collection->profiles.back();
+
+  // Record a single sample.
+  profile.samples.push_back(Sample());
+  collection->native_sampler->RecordStackSample(&profile.samples.back());
+
+  // If this is the last sample of a burst, record the total time.
+  if (collection->sample == collection->params.samples_per_burst - 1) {
+    profile.profile_duration = Time::Now() - collection->profile_start_time;
+    collection->native_sampler->ProfileRecordingStopped();
+  }
+}
+
+void StackSamplingProfiler::SamplingThread::ScheduleShutdownIfIdle() {
+  if (!active_collections_.empty())
+    return;
+
+  AutoLock lock(task_runner_lock_);
+  if (!task_runner_idle_shutdown_time_.is_zero()) {
+    GetTaskRunnerOnSamplingThread()->PostDelayedTask(
+        FROM_HERE, Bind(&SamplingThread::ShutdownTask, Unretained(this),
+                        task_runner_create_requests_, /*second=*/false),
+        task_runner_idle_shutdown_time_);
+  }
+}
+
+void StackSamplingProfiler::SamplingThread::AddCollectionTask(
+    std::unique_ptr<CollectionContext> collection) {
+  const int collection_id = collection->collection_id;
+  const TimeDelta initial_delay = collection->params.initial_delay;
+
+  active_collections_.insert(
+      std::make_pair(collection_id, std::move(collection)));
+
+  GetTaskRunnerOnSamplingThread()->PostDelayedTask(
+      FROM_HERE, Bind(&SamplingThread::PerformCollectionTask, Unretained(this),
+                      collection_id),
+      initial_delay);
+}
+
+void StackSamplingProfiler::SamplingThread::RemoveCollectionTask(int id) {
+  auto found = active_collections_.find(id);
+  if (found == active_collections_.end())
+    return;
+
+  FinishCollection(found->second.get());
+  ScheduleShutdownIfIdle();
+}
+
+void StackSamplingProfiler::SamplingThread::PerformCollectionTask(int id) {
+  auto found = active_collections_.find(id);
+
+  // The task won't be found if it has been stopped.
+  if (found == active_collections_.end())
+    return;
+
+  CollectionContext* collection = found->second.get();
+
+  // Handle first-run with no "next time".
+  if (collection->next_sample_time == Time())
+    collection->next_sample_time = Time::Now();
+
+  // Do the collection of a single sample.
+  RecordSample(collection);
+
+  // Update the time of the next sample recording.
+  if (UpdateNextSampleTime(collection)) {
+    bool success = GetTaskRunnerOnSamplingThread()->PostDelayedTask(
+        FROM_HERE,
+        Bind(&SamplingThread::PerformCollectionTask, Unretained(this), id),
+        std::max(collection->next_sample_time - Time::Now(), TimeDelta()));
+    DCHECK(success);
+  } else {
+    // All capturing has completed so finish the collection. Let object expire.
+    // The |collection| variable will be invalid after this call.
+    FinishCollection(collection);
+    ScheduleShutdownIfIdle();
+  }
+}
+
+void StackSamplingProfiler::SamplingThread::ShutdownTask(int create_requests,
+                                                         bool second) {
+  // Holding this lock ensures that any attempt to start another job will
+  // get postponed until StopSoon can run thus eliminating the race.

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

It seems like the key to eliminating the race is actually setting task_runner_
to null while holding the lock, since that indicates to GetOrCreateTaskRunner
that it needs to wait for the thread to shut down before restarting it.

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

Done.

+  AutoLock lock(task_runner_lock_);
+
+  // If active_collections_ is not empty, something new has arrived since
+  // this task got posted. Abort the shutdown so it can be processed.
+  if (!active_collections_.empty())
+    return;
+
+  // If the current count of creation requests doesn't match the passed count
+  // then other tasks have been created since this was posted. Abort shutdown.
+  if (task_runner_create_requests_ != create_requests)
+    return;
+
+  // It's possible that a new AddCollectionTask has been posted after this
+  // task. Post a second shutdown task to do the actual shutdown, delayed (by 0)
+  // to ensure it runs after any immediate tasks that may be pending.
+  if (!second && task_runner_) {

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

I think the task_runner_create_requests_ check above should avoid the need for a
second task posting:

If GetOrCreateTaskRunner was already executed, then we would have failed the
check, regardless of whether the task got posted yet. If GetOrCreateTaskRunner
is being executed now and waiting on task_runner_lock_, then it will not start
executing until we've done the shutdown sequence below and reset task_runner_ so
it won't have a chance to post any tasks until after restart.

task_runner_create_requests_ essentially serves as a proxy for whether there
could be pending add requests that haven't been executed yet.

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

Yeah, I think that's reasonable on the assumption that the "idle delay" is
sufficient to ensure the execution of the AddCollectionTask associated with an
increment of task_runner_create_requests.

But what about this:

- task_runner_create_requests_ is incremented (at time T)
- ScheduleShutdownIfIdle runs with this new value
- AddTask gets posted
- New collections runs for 55 seconds
- ShutdownTask runs at T+60 seconds
- thread exits only 5 seconds after the completion of the last collection

It's not really a problem; it just results in the thread possibly exiting
without waiting for the full delay.

Also, I think the fix would be complicated.  ScheduleShutdownIfIdle would have
to do the two back-to-back runs to get both the latest
task_runner_creation_requests_ count and the non-empty collections boolean.

On top of that...  The PostTask done by Add() would have be done while the
task_runner_lock_ remains held to ensure that both those two
ScheduleShutdownIfIdle() task runs couldn't occur between the increment and the
posting.  Doing that would require undoing the GetOrCreateTaskRunner helper
method.

[Mike Wittman, 2017/02/21 18:57:02]

It seems like the basic issue here is distinguishing whether the
active_collections_.empty() state is the result of the state being empty since
the ShutdownTask was posted, or the result of collections occurring then
completing.

Rather than trying to figure this out post hoc, I think a simpler mechanism
would be to make a note of when collections start, e.g. by separately
incrementing task_runner_create_requests_ (under lock) in AddCollectionTask.
Then the create_requests check would detect this case too, and there would be no
need to try to divine the current state using the active_collections_.empty()
check and multiple task postings in ShutdownTask.

[bcwhite, 2017/02/21 21:48:05]

Works for me.

+    task_runner_->PostDelayedTask(FROM_HERE,
+                                  Bind(&SamplingThread::ShutdownTask,
+                                       Unretained(this), create_requests, true),
+                                  TimeDelta());
+    return;
+  }
+
+  // There can be no new AddCollectionTasks at this point because creating
+  // those always increments "create requests". There may be other requests,
+  // like Remove, but it's okay to schedule the thread to stop once they've
+  // been executed (i.e. "soon").
+  StopSoon();
+
+  // StopSoon will have set the owning sequence (again) so it must be detached
+  // (again) in order for Stop/Start to be called (again) should more work
+  // come in. Holding the |task_runner_lock_| ensures the necessary happens-
+  // after with regard to this detach and future Thread API calls.
+  DetachFromSequence();
+
+  // Clear the task_runner_ variable so the thread will be restarted when
+  // new work comes in.
+  task_runner_ = nullptr;
+}
+
+bool StackSamplingProfiler::SamplingThread::UpdateNextSampleTime(
+    CollectionContext* collection) {
+  // 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.
+  if (++collection->sample < collection->params.samples_per_burst) {
+    collection->next_sample_time += collection->params.sampling_interval;
+    return true;
+  }
+
+  if (++collection->burst < collection->params.bursts) {
+    collection->sample = 0;
+    collection->next_sample_time += collection->params.burst_interval;
+    return true;
+  }
+
+  return false;
+}
+
+void StackSamplingProfiler::SamplingThread::CleanUp() {
+  // There should be no collections remaining when the thread stops.
+  DCHECK(active_collections_.empty());
+
+  // Let the parent clean up.
+  Thread::CleanUp();
 }
 
 // StackSamplingProfiler ------------------------------------------------------
 
+// static
+bool StackSamplingProfiler::TestAPI::IsSamplingThreadRunning() {
+  return SamplingThread::GetInstance()->IsRunning();
+}
+
+// static
+void StackSamplingProfiler::TestAPI::SetSamplingThreadIdleShutdownTime(
+    int shutdown_ms) {
+  SamplingThread::GetInstance()->SetIdleShutdownTime(shutdown_ms);
+}
+
+// static
+void StackSamplingProfiler::TestAPI::InitiateSamplingThreadIdleShutdown() {
+  SamplingThread::GetInstance()->ShutdownIfIdle();
+}
+
 subtle::Atomic32 StackSamplingProfiler::process_milestones_ = 0;
 
 StackSamplingProfiler::SamplingParams::SamplingParams()
     : initial_delay(TimeDelta::FromMilliseconds(0)),
       bursts(1),
       burst_interval(TimeDelta::FromMilliseconds(10000)),
       samples_per_burst(300),
       sampling_interval(TimeDelta::FromMilliseconds(100)) {
 }
 
 StackSamplingProfiler::StackSamplingProfiler(
-    PlatformThreadId thread_id,
     const SamplingParams& params,
-    const CompletedCallback& callback)
-    : StackSamplingProfiler(thread_id, params, callback, nullptr) {}
+    const CompletedCallback& callback,
+    NativeStackSamplerTestDelegate* test_delegate)
+    : StackSamplingProfiler(base::PlatformThread::CurrentId(),
+                            params,
+                            callback,
+                            test_delegate) {}
 
 StackSamplingProfiler::StackSamplingProfiler(
     PlatformThreadId thread_id,
     const SamplingParams& params,
     const CompletedCallback& callback,
     NativeStackSamplerTestDelegate* test_delegate)
-    : thread_id_(thread_id), params_(params), completed_callback_(callback),
+    : thread_id_(thread_id),
+      params_(params),
+      completed_callback_(callback),
+      finished_event_(WaitableEvent::ResetPolicy::MANUAL,
+                      WaitableEvent::InitialState::NOT_SIGNALED),
       test_delegate_(test_delegate) {
+  native_sampler_ = NativeStackSampler::Create(thread_id_, &RecordAnnotations,
+                                               test_delegate_);
 }
 
 StackSamplingProfiler::~StackSamplingProfiler() {
+  if (!native_sampler_)
+    return;
+
+  // Stop is immediate but asynchronous. There is a non-zero probability that
+  // one more sample will be taken after this call returns.
   Stop();
-  if (!sampling_thread_handle_.is_null())
-    PlatformThread::Join(sampling_thread_handle_);
+
+  // The behavior of sampling a thread that has exited is undefined and could
+  // cause Bad Things(tm) to occur. The safety model provided by this class is
+  // that an instance of this object is expected to live at least as long as
+  // the thread it is sampling. However, because the sampling is performed
+  // asynchronously by the SamplingThread, there is no way to guarantee this
+  // is true without waiting for it to signal that it has finished.
+  //
+  // The wait time should, at most, be only as long as it takes to collect one
+  // sample (~200us) or none at all if sampling has already completed.
+  ThreadRestrictions::ScopedAllowWait allow_wait;
+  finished_event_.Wait();
+}
+
+void StackSamplingProfiler::Start() {
+  if (!native_sampler_ || completed_callback_.is_null())
+    return;
+
+  DCHECK_EQ(-1, collection_id_);
+  collection_id_ = SamplingThread::GetInstance()->Add(
+      MakeUnique<SamplingThread::CollectionContext>(
+          thread_id_, params_, completed_callback_, &finished_event_,
+          native_sampler_.get()));
+  DCHECK_NE(-1, collection_id_);
+}
+
+void StackSamplingProfiler::Stop() {
+  if (!native_sampler_)
+    return;
+
+  SamplingThread::GetInstance()->Remove(collection_id_);
+  collection_id_ = -1;
 }
 
 // static
-void StackSamplingProfiler::StartAndRunAsync(
-    PlatformThreadId thread_id,
-    const SamplingParams& params,
-    const CompletedCallback& callback) {
-  CHECK(ThreadTaskRunnerHandle::Get());
-  AsyncRunner::Run(thread_id, params, callback);
-}
-
-void StackSamplingProfiler::Start() {
-  if (completed_callback_.is_null())
-    return;
-
-  std::unique_ptr<NativeStackSampler> native_sampler =
-      NativeStackSampler::Create(thread_id_, &RecordAnnotations,
-                                 test_delegate_);
-  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();
-}
-
-void StackSamplingProfiler::Stop() {
-  if (sampling_thread_)
-    sampling_thread_->Stop();
-}
-
-// static
 void StackSamplingProfiler::SetProcessMilestone(int milestone) {
   DCHECK_LE(0, milestone);
   DCHECK_GT(static_cast<int>(sizeof(process_milestones_) * 8), milestone);
   DCHECK_EQ(0, subtle::NoBarrier_Load(&process_milestones_) & (1 << milestone));
   ChangeAtomicFlags(&process_milestones_, 1 << milestone, 0);
 }
 
 // static
 void StackSamplingProfiler::ResetAnnotationsForTesting() {
   subtle::NoBarrier_Store(&process_milestones_, 0u);
@@ skipping 42 matching lines @@
 }
 
 bool operator<(const StackSamplingProfiler::Frame &a,
                const StackSamplingProfiler::Frame &b) {
   return (a.module_index < b.module_index) ||
       (a.module_index == b.module_index &&
        a.instruction_pointer < b.instruction_pointer);
 }
 
 }  // namespace base