Make stack sampling profiler sample beyond startup.

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 39d665cb24f77064097330137c0818142ed4004c..ab2268be5eb22cd7bbaa3556afa33a4a4e74e26b 100644
--- a/base/profiler/stack_sampling_profiler.cc
+++ b/base/profiler/stack_sampling_profiler.cc
@@ -139,12 +139,13 @@ class StackSamplingProfiler::SamplingThread : public Thread {
   };
 
   struct CollectionContext {
-    CollectionContext(PlatformThreadId target,
+    CollectionContext(int collection_id,
+                      PlatformThreadId target,
                       const SamplingParams& params,
                       const CompletedCallback& callback,
                       WaitableEvent* finished,
                       std::unique_ptr<NativeStackSampler> sampler)
-        : collection_id(next_collection_id_.GetNext()),
+        : collection_id(collection_id),
           target(target),
           params(params),
           callback(callback),
@@ -177,7 +178,6 @@ class StackSamplingProfiler::SamplingThread : public Thread {
     // The collected stack samples. The active profile is always at the back().
     CallStackProfiles profiles;
 
-   private:
     static StaticAtomicSequenceNumber next_collection_id_;

[Mike Wittman, 2017/07/06 17:25:53]

nit: drop trailing underscore

[Alexei Svitkine (slow), 2017/07/06 19:36:26]

Done.

   };
 
@@ -228,8 +228,9 @@ class StackSamplingProfiler::SamplingThread : public Thread {
   // 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);
+  // Finishes a collection and reports collected data via callback. Returns
+  // the new collection params, if a new collection should be started.
+  Optional<SamplingParams> FinishCollection(CollectionContext* collection);
 
   // Records a single sample of a collection.
   void RecordSample(CollectionContext* collection);
@@ -475,7 +476,8 @@ StackSamplingProfiler::SamplingThread::GetTaskRunnerOnSamplingThread() {
   return Thread::task_runner();
 }
 
-void StackSamplingProfiler::SamplingThread::FinishCollection(
+Optional<StackSamplingProfiler::SamplingParams>
+StackSamplingProfiler::SamplingThread::FinishCollection(
     CollectionContext* collection) {
   DCHECK_EQ(GetThreadId(), PlatformThread::CurrentId());
 
@@ -495,16 +497,13 @@ void StackSamplingProfiler::SamplingThread::FinishCollection(
   CallStackProfiles profiles = std::move(collection->profiles);
   WaitableEvent* finished = collection->finished;
 
-  // Remove this collection from the map of known ones.  The |collection|
-  // parameter is invalid after this point.
-  size_t count = active_collections_.erase(collection->collection_id);
-  DCHECK_EQ(1U, count);
-
   // Run the associated callback, passing the collected profiles.
-  callback.Run(std::move(profiles));
+  Optional<SamplingParams> new_params = callback.Run(std::move(profiles));
 
   // Signal that this collection is finished.
   finished->Signal();
+
+  return new_params;
 }
 
 void StackSamplingProfiler::SamplingThread::RecordSample(
@@ -589,7 +588,14 @@ void StackSamplingProfiler::SamplingThread::RemoveCollectionTask(int id) {
   if (found == active_collections_.end())
     return;
 
-  FinishCollection(found->second.get());
+  auto* collection = found->second.get();
+  FinishCollection(collection);
+
+  // Remove this collection from the map of known ones. After this,
+  // |collection| will be invalid.
+  size_t count = active_collections_.erase(collection->collection_id);
+  DCHECK_EQ(1U, count);
+
   ScheduleShutdownIfIdle();
 }
 
@@ -612,19 +618,41 @@ void StackSamplingProfiler::SamplingThread::PerformCollectionTask(int id) {
   RecordSample(collection);
 
   // Update the time of the next sample recording.
-  if (UpdateNextSampleTime(collection)) {
-    bool success = GetTaskRunnerOnSamplingThread()->PostDelayedTask(
-        FROM_HERE,
-        BindOnce(&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. By not re-adding
-    // it to the task queue, the collection will "expire" (i.e. no further work
-    // will be done). The |collection| variable will be invalid after this call.
-    FinishCollection(collection);
-    ScheduleShutdownIfIdle();
+  const bool collection_finished = !UpdateNextSampleTime(collection);
+  if (collection_finished) {
+    // All capturing has completed so finish the collection. If no new params
+    // are returned, a new collection should not be started.
+    Optional<SamplingParams> new_params = FinishCollection(collection);
+    if (!new_params.has_value()) {
+      // Remove this collection from the map of known ones. After this,
+      // |collection| will be invalid.
+      size_t count = active_collections_.erase(collection->collection_id);
+      DCHECK_EQ(1U, count);
+
+      // By not adding it to the task queue, the collection will "expire" (i.e.
+      // no further work will be done).
+      ScheduleShutdownIfIdle();
+      return;
+    }
+
+    // Restart the collection with the new params. Keep the same collection
+    // id so the Stop() operation continues to work.
+    auto new_collection = MakeUnique<SamplingThread::CollectionContext>(
+        collection->collection_id, collection->target, new_params.value(),
+        collection->callback, collection->finished,
+        std::move(collection->native_sampler));
+    new_collection->next_sample_time =

[Mike Wittman, 2017/07/06 17:25:52]

Can we reuse the logic in AddCollectionTask for restarting the collection
(exclusive of the add events increment)? This code is pretty complex, so the
fewer ways we have of doing something the better.

[Alexei Svitkine (slow), 2017/07/06 19:36:25]

The way it's structured right now, it re-uses the logic for scheduling the next
sample (i.e. line 651 is shared between schedule to take next sample normally or
when periodic starts). It's pretty straight forward because the two work
basically the same (via next_sample_time).

The functional difference when using AddCollectionTask() is that
|thread_execution_state_add_events_| will be incremented, but I don't think it
makes a difference in this case?

We can't use it outright because it uses active_collections_.insert() which
won't replace an element. And if we made it replace it (e.g. by using
active_collections_[profiler_id] = syntax), it will hide away the invalidation
of |collection|. It will also be a bit less efficient since we'd need to do the
map lookup again.

So my preference is to keep it as-is so that the code is shared with the "next
sample case" here rather than "starting collection task". Another option is to
make a helper function wrapping the PostDelayedTask that both places could use.

[Mike Wittman, 2017/07/06 22:14:01]

I was thinking that we could remove the old collection then call
AddCollectionTask() on the new collection. I think that will result in
duplicating code to erase the old collection though (vs. the current
implementation which has duplicated code to handle the initial delay). Neither
is obviously better, so sticking with the current implementation sgtm.

+        Time::Now() + collection->params.initial_delay;
+    // Replace |collection| in the map by |new_collection|. After this,
+    // |collection| will be invalid.
+    found->second = std::move(new_collection);
   }
+
+  bool success = GetTaskRunnerOnSamplingThread()->PostDelayedTask(
+      FROM_HERE,
+      BindOnce(&SamplingThread::PerformCollectionTask, Unretained(this), id),
+      std::max(collection->next_sample_time - Time::Now(), TimeDelta()));
+  DCHECK(success);
 }
 
 void StackSamplingProfiler::SamplingThread::ShutdownTask(int add_events) {
@@ -784,6 +812,7 @@ void StackSamplingProfiler::Start() {
   DCHECK_EQ(NULL_COLLECTION_ID, collection_id_);
   collection_id_ = SamplingThread::GetInstance()->Add(
       MakeUnique<SamplingThread::CollectionContext>(
+          SamplingThread::CollectionContext::next_collection_id_.GetNext(),
           thread_id_, params_, completed_callback_, &profiling_inactive_,
           std::move(native_sampler)));
   DCHECK_NE(NULL_COLLECTION_ID, collection_id_);