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 f294251cd32b6dff9d3b3b4bee1bbf8134e66ba7..fd73e7f7732cbca8252ae2085059fe396088d505 100644
--- a/base/profiler/stack_sampling_profiler.cc
+++ b/base/profiler/stack_sampling_profiler.cc
@@ -5,16 +5,23 @@
 #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"
 
@@ -22,66 +29,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
-// 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) {
@@ -160,102 +107,424 @@ 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) {}
+class StackSamplingProfiler::SamplingThread : public Thread {
+ public:
+  struct CollectionContext {
+    CollectionContext(PlatformThreadId target,
+                      const SamplingParams& params,
+                      const CompletedCallback& callback,
+                      WaitableEvent* finished,
+                      std::unique_ptr<NativeStackSampler> sampler)
+        : collection_id(next_collection_id_.GetNext()),
+          target(target),
+          params(params),
+          callback(callback),
+          finished(finished),
+          native_sampler(std::move(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.
+    const std::unique_ptr<NativeStackSampler> 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.
+  void CheckForIdle();
+
+  // These methods are tasks that get posted to the internal message queue.
+  void AddCollectionTask(std::unique_ptr<CollectionContext> collection_ptr);
+  void RemoveCollectionTask(int id);
+  void PerformCollectionTask(int id);
+  void ShutdownTask();
+
+  // 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(5);

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

I think it's worth bumping this up to something more like a minute, since
keeping a thread around is very cheap.

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

Done.

+  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();
+
+  // There may be no task-runner if the sampling thread has been permanently
+  // shut down.
+  if (task_runner) {

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

No need to check the task_runner anymore.

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

Done.

+    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.
+  task_runner->PostTask(FROM_HERE,
+                        Bind(&SamplingThread::ShutdownTask, Unretained(this)));
+}
+
+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 on by another

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

nit: stopped by

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

Done.

+    // 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),
+  // calculated it now.

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

nit: calculate

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

Done.

+  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);
+  }
 
-StackSamplingProfiler::SamplingThread::~SamplingThread() {}
+  // The currently active profile being acptured.
+  CallStackProfile& profile = collection->profiles.back();
 
-void StackSamplingProfiler::SamplingThread::ThreadMain() {
-  PlatformThread::SetName("Chrome_SamplingProfilerThread");
+  // Record a single sample.
+  profile.samples.push_back(Sample());
+  Sample& sample = profile.samples.back();
+  collection->native_sampler->RecordStackSample(&sample);

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

nit: RecordStackSample(&profile.samples.back()) and remove the previous line

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

Done.

 
-  // For now, just ignore any requests to profile while another profiler is
-  // working.
-  if (!concurrent_profiling_lock.Get().Try())
+  // 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::CheckForIdle() {

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

how about calling this ScheduleShutdownIfIdle, to be clear what's happening
here?

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

Done.

+  if (!active_collections_.empty())
     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();
+  AutoLock lock(task_runner_lock_);
+  if (!task_runner_idle_shutdown_time_.is_zero()) {
+    GetTaskRunnerOnSamplingThread()->PostDelayedTask(
+        FROM_HERE, Bind(&SamplingThread::ShutdownTask, Unretained(this)),
+        task_runner_idle_shutdown_time_);
   }
+}
+
+void StackSamplingProfiler::SamplingThread::AddCollectionTask(
+    std::unique_ptr<CollectionContext> collection_ptr) {
+  // Ownership of the collection is going to be given to a map but a pointer
+  // to it will be needed later.
+  CollectionContext* collection = collection_ptr.get();

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

nit: better to save off the id and initial_delay and pass those below, to
eliminate the burden on the reader of figuring out if the pointer usage is safe 

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

Done.

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

This is no longer potentially dangerous, so I think we can remove the comment as
well.

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

Done.

+  active_collections_.insert(
+      std::make_pair(collection->collection_id, std::move(collection_ptr)));
+
+  GetTaskRunnerOnSamplingThread()->PostDelayedTask(
+      FROM_HERE, Bind(&SamplingThread::PerformCollectionTask, Unretained(this),
+                      collection->collection_id),
+      collection->params.initial_delay);
+}
+
+void StackSamplingProfiler::SamplingThread::RemoveCollectionTask(int id) {
+  auto found = active_collections_.find(id);
+  if (found == active_collections_.end())
+    return;
+
+  FinishCollection(found->second.get());
+  CheckForIdle();
+}
+
+void StackSamplingProfiler::SamplingThread::PerformCollectionTask(int id) {
+  auto found = active_collections_.find(id);
 
-  *elapsed_time = profile_timer.Elapsed();
-  profile->profile_duration = *elapsed_time;
-  native_sampler_->ProfileRecordingStopped();
+  // 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();

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

Can we initialize this in the CollectionContext constructor?

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

No because it needs to be recorded when the first sample is being done which is
after the initial delay plus any other things the sampling-thread is doing.

[Mike Wittman, 2017/02/15 22:52:16]

But the value recorded isn't derived from next_sample_time; the only place where
next_sample_time is read is in the PostDelayedTaskCall below. It looks to me
that the max expression will be equal to TimeDelta() regardless of whether
next_sample_time is set to Time::Now() here or when the CollectionContext is
constructed.

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

next_sample_time is persistent and all sample times will be calculated from its
starting value.  If it is set any time before the very first sample, then it
will race to catch up, capturing multiple samples at the start.

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

Ah, right. Missed that all the sample times are offset from the initial one.

+
+  // 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);
+    CheckForIdle();
+  }
 }
 
-// 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::ShutdownTask() {
+  // Holding this lock ensures that any attempt to start another job will
+  // get postponed until StopSoon can run thus eliminating the race.
+  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;
+  // It's possible that a new AddCollectionTask has been posted after this
+  // task. Reset the "create requests" counter and try again after any other
+  // pending tasks.
+  if (task_runner_create_requests_ > 0 && task_runner_) {
+    task_runner_create_requests_ = 0;
+    task_runner_->PostTask(
+        FROM_HERE, Bind(&SamplingThread::ShutdownTask, Unretained(this)));
     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) {
+  if (++collection->sample < collection->params.samples_per_burst) {
+    collection->next_sample_time += collection->params.sampling_interval;
+    return true;
+  }
 
-  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;
+  // This will keep a consistent average interval between samples but will

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

Should this comment be on the above if statement?

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

Done.

+  // 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->burst < collection->params.bursts) {
+    collection->sample = 0;
+    collection->next_sample_time += collection->params.burst_interval;
+    return true;
   }
+
+  return false;
 }
 
-void StackSamplingProfiler::SamplingThread::Stop() {
-  stop_event_.Signal();
+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 ------------------------------------------------------
@@ -271,55 +540,81 @@ StackSamplingProfiler::SamplingParams::SamplingParams()
 }
 
 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,

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

Can we continue to use the current strategy of constructing this as a local
variable in Start()? It's moved within that function, so the pointer is not
valid after that anyway.

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

I thought about that but decided to leave it this way because it should allow
multiple start/stop operations without having to recreate it each time.

[Mike Wittman, 2017/02/15 22:52:16]

On second look, I think we have to create the native sampler in Start() because
the current implementation is not correct. The native sampler is moved into the
CollectionContext the first time Start is called, and is never recreated, so
it's not valid to move again when Start is called a second time.

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

That was the case but not any more.  Ownership of the native sampler stays with
this object and the context has only a pointer to it, much like the signaled
event.

A invalid native_sampler_ is used to return early in many API calls so as to not
try to access a sampling-thread that doesn't exist.

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

I just remembered why I implemented the creation of the native sampler in Start
originally: it's so that the use and destruction of the object does not occur on
different threads. This makes it trivial to reason about the correctness of the
use of the object, without having to consider any synchronization concerns.

The readability benefit of not having to think about synchronization greatly
outweighs the runtime benefit of avoiding additional object constructions, so we
should keep the existing behavior.
I don't understand. In patch set 25, how can native_sampler_ ever be null after
creation?

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

Use will always be on a different thread that construction because use is always
on the SamplingThread.

Right now construction & destruction of the native sampler occurs on the same
thread that does construction & destruction of the generic object.

If I move the construction to Start and Start can be called by yet another
thread, then construction and destruction of the native sampler can be called on
different threads because the destruction of the native sampler will occur with
the destruction of the generic sampler.

Moving destruction of the native sampler to Stop won't help because there is no
requirement that Stop ever be called.
The existing behavior was that the native sampler was constructed by the thread
calling Start but destructed on the sampling thread.  That seems riskier.
Unsupported platforms return nullptr from NativeStackSampler::Create().

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

What's the risk you're concerned about in creating and destroying the native
sampler on different threads? It's not possible for those two lifecycle events
to overlap, so there's no coordination required. I'm not seeing where this could
go wrong.

If use and destruction are on different threads, however, then this requires
coordination between the threads to ensure that the object is not destroyed
while it is still being used. The more state that requires cross-thread
synchronization, the more difficult it is to understand and validate the code,
and the harder it is to make future changes correctly.
Right. I don't think it's worth the complexity to try to maintain the
NativeStackSampler pointer as the sentinel for whether sampling is supported.
Better to save off a boolean on first attempted creation or add a
NativeStackSampler::IsSupported() function, whatever's simpler.

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

That's already done because destruction requires waiting on collection having
finished.
Existing collection_id will do it, too.  Done.

[Mike Wittman, 2017/02/22 03:06:47]

That works.

+                                               test_delegate_);
 }
 
 StackSamplingProfiler::~StackSamplingProfiler() {
+  // 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_);
-}
 
-// static
-void StackSamplingProfiler::StartAndRunAsync(
-    PlatformThreadId thread_id,
-    const SamplingParams& params,
-    const CompletedCallback& callback) {
-  CHECK(ThreadTaskRunnerHandle::Get());
-  AsyncRunner::Run(thread_id, params, callback);
+  // 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 execute one
+  // short task or none at all if sampling has already completed.

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

nit: ... as long as it takes to collect one sample, taking ~200μs, or none at
all ...

Jank has a precise definition (tasks taking >118ms) so readers will want to
evaluate the wait duration in absolute terms.

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

Done.

+  ThreadRestrictions::ScopedAllowWait allow_wait;
+  finished_event_.Wait();
 }
 
 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)
+  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();
+  DCHECK_EQ(-1, collection_id_);
+  collection_id_ = SamplingThread::GetInstance()->Add(
+      MakeUnique<SamplingThread::CollectionContext>(
+          thread_id_, params_, completed_callback_, &finished_event_,
+          std::move(native_sampler_)));
+  DCHECK_NE(-1, collection_id_);
 }
 
 void StackSamplingProfiler::Stop() {
-  if (sampling_thread_)
-    sampling_thread_->Stop();
+  SamplingThread::GetInstance()->Remove(collection_id_);
+  collection_id_ = -1;
+}
+
+// static
+bool StackSamplingProfiler::IsSamplingThreadRunningForTesting() {
+  return SamplingThread::GetInstance()->IsRunning();
+}
+
+// static
+void StackSamplingProfiler::SetSamplingThreadIdleShutdownTimeForTesting(
+    int shutdown_ms) {
+  SamplingThread::GetInstance()->SetIdleShutdownTime(shutdown_ms);
+}
+
+// static
+void StackSamplingProfiler::InitiateSamplingThreadIdleShutdownForTesting() {
+  SamplingThread::GetInstance()->ShutdownIfIdle();
 }
 
 // static