Support parallel captures from the StackSamplingProfiler.

base/profiler/stack_sampling_profiler_unittest.cc

Index: base/profiler/stack_sampling_profiler_unittest.cc
diff --git a/base/profiler/stack_sampling_profiler_unittest.cc b/base/profiler/stack_sampling_profiler_unittest.cc
index 0b300d575d73c76f65a6afd7e53a372020a844a3..063091c3ba92c684044a204d115b9a350efd6026 100644
--- a/base/profiler/stack_sampling_profiler_unittest.cc
+++ b/base/profiler/stack_sampling_profiler_unittest.cc
@@ -351,6 +351,25 @@ void WithTargetThread(Function function) {
   WithTargetThread(function, StackConfiguration(StackConfiguration::NORMAL));
 }
 
+// Waits for one of multiple samplings to complete.
+void CreateProfilers(PlatformThreadId target_thread_id,

[Mike Wittman, 2017/03/18 01:38:41]

Nice, encapsulating this functionality makes the tests cleaner.

Can you pass the input profiles as a vector, and the three output arguments as
pointers to empty vectors (and push_back() here), so we don't need to keep all
the array/vector sizes in sync in the tests?

Also, input arguments should appear before output arguments in the interface.

[bcwhite, 2017/03/20 21:50:51]

Done.

[Mike Wittman, 2017/03/21 16:50:38]

param should be passed as a vector also.

[bcwhite, 2017/03/22 17:48:54]

Done.

+                     std::unique_ptr<StackSamplingProfiler>* profilers,
+                     std::unique_ptr<WaitableEvent>* completed,
+                     CallStackProfiles* profiles,
+                     SamplingParams* params,

[Mike Wittman, 2017/03/18 01:38:41]

const

[bcwhite, 2017/03/20 21:50:51]

Done.

+                     size_t count) {
+  for (size_t i = 0; i < count; ++i) {
+    completed[i] =
+        MakeUnique<WaitableEvent>(WaitableEvent::ResetPolicy::AUTOMATIC,
+                                  WaitableEvent::InitialState::NOT_SIGNALED);
+    const StackSamplingProfiler::CompletedCallback callback =
+        Bind(&SaveProfilesAndSignalEvent, Unretained(&profiles[i]),
+             Unretained(completed[i].get()));
+    profilers[i] = MakeUnique<StackSamplingProfiler>(target_thread_id,
+                                                     params[i], callback);
+  }
+}
+
 // Captures profiles as specified by |params| on the TargetThread, and returns
 // them in |profiles|. Waits up to |profiler_wait_time| for the profiler to
 // complete.
@@ -374,6 +393,21 @@ void CaptureProfiles(const SamplingParams& params, TimeDelta profiler_wait_time,
   });
 }
 
+// Waits for one of multiple samplings to complete.
+size_t WaitForSamplingComplete(
+    std::vector<std::unique_ptr<WaitableEvent>>* sampling_completed) {
+  // Map unique_ptrs to something that WaitMany can accept.
+  std::vector<WaitableEvent*> sampling_completed_rawptrs(
+      sampling_completed->size());
+  std::transform(
+      sampling_completed->begin(), sampling_completed->end(),
+      sampling_completed_rawptrs.begin(),
+      [](const std::unique_ptr<WaitableEvent>& elem) { return elem.get(); });
+  // Wait for one profiler to finish.
+  return WaitableEvent::WaitMany(sampling_completed_rawptrs.data(),
+                                 sampling_completed_rawptrs.size());
+}
+
 // If this executable was linked with /INCREMENTAL (the default for non-official
 // debug and release builds on Windows), function addresses do not correspond to
 // function code itself, but instead to instructions in the Incremental Link
@@ -444,9 +478,9 @@ void TestLibraryUnload(bool wait_until_unloaded) {
     StackCopiedSignaler(WaitableEvent* stack_copied,
                         WaitableEvent* start_stack_walk,
                         bool wait_to_walk_stack)
-        : stack_copied_(stack_copied), start_stack_walk_(start_stack_walk),
-          wait_to_walk_stack_(wait_to_walk_stack) {
-    }
+        : stack_copied_(stack_copied),
+          start_stack_walk_(start_stack_walk),
+          wait_to_walk_stack_(wait_to_walk_stack) {}
 
     void OnPreStackWalk() override {
       stack_copied_->Signal();
@@ -720,35 +754,6 @@ TEST(StackSamplingProfilerTest, MAYBE_Alloca) {
       << FormatSampleForDiagnosticOutput(sample, profile.modules);
 }
 
-// Checks that the fire-and-forget interface works.
-#if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
-#define MAYBE_StartAndRunAsync StartAndRunAsync
-#else
-#define MAYBE_StartAndRunAsync DISABLED_StartAndRunAsync
-#endif
-TEST(StackSamplingProfilerTest, MAYBE_StartAndRunAsync) {
-  // StartAndRunAsync requires the caller to have a message loop.
-  MessageLoop message_loop;
-
-  SamplingParams params;
-  params.samples_per_burst = 1;
-
-  CallStackProfiles profiles;
-  WithTargetThread([&params, &profiles](PlatformThreadId target_thread_id) {
-    WaitableEvent sampling_thread_completed(
-        WaitableEvent::ResetPolicy::AUTOMATIC,
-        WaitableEvent::InitialState::NOT_SIGNALED);
-    const StackSamplingProfiler::CompletedCallback callback =
-        Bind(&SaveProfilesAndSignalEvent, Unretained(&profiles),
-             Unretained(&sampling_thread_completed));
-    StackSamplingProfiler::StartAndRunAsync(target_thread_id, params, callback);
-    RunLoop().RunUntilIdle();
-    sampling_thread_completed.Wait();
-  });
-
-  ASSERT_EQ(1u, profiles.size());
-}
-
 // Checks that the expected number of profiles and samples are present in the
 // call stack profiles produced.
 #if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
@@ -860,6 +865,45 @@ TEST(StackSamplingProfilerTest, MAYBE_DestroyProfilerWhileProfiling) {
 #define MAYBE_CanRunMultipleTimes DISABLED_CanRunMultipleTimes
 #endif
 TEST(StackSamplingProfilerTest, MAYBE_CanRunMultipleTimes) {
+  StackSamplingProfiler::TestAPI::DisableIdleShutdown();
+
+  WithTargetThread([](PlatformThreadId target_thread_id) {
+    SamplingParams params;
+    params.sampling_interval = TimeDelta::FromMilliseconds(0);
+    params.samples_per_burst = 1;
+
+    CallStackProfiles profiles;
+    WaitableEvent sampling_completed(WaitableEvent::ResetPolicy::MANUAL,
+                                     WaitableEvent::InitialState::NOT_SIGNALED);
+    const StackSamplingProfiler::CompletedCallback callback =
+        Bind(&SaveProfilesAndSignalEvent, Unretained(&profiles),
+             Unretained(&sampling_completed));
+    StackSamplingProfiler profiler(target_thread_id, params, callback);
+
+    // Just start and stop to execute code paths.
+    profiler.Start();
+    profiler.Stop();
+    sampling_completed.Wait();
+
+    // Ensure a second request will run and not block.
+    sampling_completed.Reset();
+    profiles.clear();
+    profiler.Start();
+    sampling_completed.Wait();
+    profiler.Stop();
+    ASSERT_EQ(1u, profiles.size());
+  });
+}
+
+// Checks that the different profilers may be run.
+#if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
+#define MAYBE_CanRunMultipleProfilers CanRunMultipleProfilers
+#else
+#define MAYBE_CanRunMultipleProfilers DISABLED_CanRunMultipleProfilers
+#endif
+TEST(StackSamplingProfilerTest, MAYBE_CanRunMultipleProfilers) {
+  StackSamplingProfiler::TestAPI::DisableIdleShutdown();
+
   SamplingParams params;
   params.sampling_interval = TimeDelta::FromMilliseconds(0);
   params.samples_per_burst = 1;
@@ -873,60 +917,207 @@ TEST(StackSamplingProfilerTest, MAYBE_CanRunMultipleTimes) {
   ASSERT_EQ(1u, profiles.size());
 }
 
-// Checks that requests to start profiling while another profile is taking place
-// are ignored.
+// Checks that additional requests will restart a stopped profiler.
 #if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
-#define MAYBE_ConcurrentProfiling ConcurrentProfiling
+#define MAYBE_WillRestartSampler WillRestartSampler

[Mike Wittman, 2017/03/18 01:38:41]

WillRestartSamplerAfterIdleShutdown would be a better name for this test.

[bcwhite, 2017/03/20 21:50:51]

Done.

 #else
-#define MAYBE_ConcurrentProfiling DISABLED_ConcurrentProfiling
+#define MAYBE_WillRestartSampler DISABLED_WillRestartSampler
 #endif
-TEST(StackSamplingProfilerTest, MAYBE_ConcurrentProfiling) {
+TEST(StackSamplingProfilerTest, MAYBE_WillRestartSampler) {
+  StackSamplingProfiler::TestAPI::DisableIdleShutdown();
+
+  SamplingParams params;
+  params.sampling_interval = TimeDelta::FromMilliseconds(0);
+  params.samples_per_burst = 1;
+
+  std::vector<CallStackProfile> profiles;
+  CaptureProfiles(params, AVeryLongTimeDelta(), &profiles);
+  ASSERT_EQ(1u, profiles.size());
+
+  // Capture thread should still be running at this point.
+  ASSERT_TRUE(StackSamplingProfiler::TestAPI::IsSamplingThreadRunning());
+
+  // Initiate an "idle" shutdown.  The task will be run immediately but on
+  // another thread so wait for it to complete.
+  StackSamplingProfiler::TestAPI::InitiateSamplingThreadIdleShutdown();
+  while (StackSamplingProfiler::TestAPI::IsSamplingThreadRunning())

[Mike Wittman, 2017/03/18 01:38:41]

Thinking about this in terms of the underlying StackSamplingProfiler state, I
don't see the need to wait for the thread to exit in this test.

Thread::Stop() is documented to handle both the pre-exit and exited cases, so
the behavior in StackSamplingProfiler is the same regardless of which state the
thread is in.

[bcwhite, 2017/03/20 21:50:51]

But then you wouldn't be able to tell that the thread restarted.  For all the
test could know, the same thread would have continued to run.

[Mike Wittman, 2017/03/21 16:50:38]

That's true. But whether the thread stopped after idle shutdown is a different
concern than whether a new collection can start. It's also a very important
behavior on its own and deserves to be separated out into its own test.

[bcwhite, 2017/03/22 17:48:54]

Done.

+    PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(1));
+
+  // Ensure another capture will start the sampling thread and run.
+  profiles.clear();
+  CaptureProfiles(params, AVeryLongTimeDelta(), &profiles);
+  ASSERT_EQ(1u, profiles.size());
+  EXPECT_TRUE(StackSamplingProfiler::TestAPI::IsSamplingThreadRunning());
+}
+
+// Checks that it's safe to stop a task after it's completed and the sampling
+// thread has shut-down for being idle.
+#if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
+#define MAYBE_StopAfterIdle StopAfterIdle

[Mike Wittman, 2017/03/18 01:38:41]

StopAfterIdleShutdown would be a better name.

[bcwhite, 2017/03/20 21:50:51]

Done.

+#else
+#define MAYBE_StopAfterIdle DISABLED_StopAfterIdle
+#endif
+TEST(StackSamplingProfilerTest, MAYBE_StopAfterIdle) {
+  StackSamplingProfiler::TestAPI::DisableIdleShutdown();
+
+  WithTargetThread([](PlatformThreadId target_thread_id) {
+    SamplingParams params;
+    params.sampling_interval = TimeDelta::FromMilliseconds(0);
+    params.samples_per_burst = 1;
+
+    CallStackProfiles profiles;
+    WaitableEvent sampling_completed(WaitableEvent::ResetPolicy::MANUAL,
+                                     WaitableEvent::InitialState::NOT_SIGNALED);
+    const StackSamplingProfiler::CompletedCallback callback =
+        Bind(&SaveProfilesAndSignalEvent, Unretained(&profiles),
+             Unretained(&sampling_completed));
+    StackSamplingProfiler profiler(target_thread_id, params, callback);
+
+    // Let it run and then stop due to being idle.
+    profiler.Start();
+    sampling_completed.Wait();
+    StackSamplingProfiler::TestAPI::InitiateSamplingThreadIdleShutdown();
+    while (StackSamplingProfiler::TestAPI::IsSamplingThreadRunning())

[Mike Wittman, 2017/03/18 01:38:41]

I don't think we need this for the same reason as above.

+      PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(1));
+
+    // Ensure it's still safe to stop.
+    profiler.Stop();
+    EXPECT_FALSE(StackSamplingProfiler::TestAPI::IsSamplingThreadRunning());
+  });
+}
+
+// Checks that synchronized multiple sampling requests execute in parallel.
+#if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
+#define MAYBE_ConcurrentProfiling_InSync ConcurrentProfiling_InSync
+#else
+#define MAYBE_ConcurrentProfiling_InSync DISABLED_ConcurrentProfiling_InSync
+#endif
+TEST(StackSamplingProfilerTest, MAYBE_ConcurrentProfiling_InSync) {
   WithTargetThread([](PlatformThreadId target_thread_id) {
     SamplingParams params[2];
+
+    // Providing an initial delay makes it more likely that both will be
+    // scheduled before either starts to run. Once started, samples will
+    // run at their scheduled, interleaved times regardless of whatever

[Mike Wittman, 2017/03/18 01:38:41]

This doesn't make sense to me. How can the samples run at their scheduled times
if the thread hasn't woken up by then?

[bcwhite, 2017/03/20 21:50:51]

Fixed wording.

+    // interval the thread wakes up.
     params[0].initial_delay = TimeDelta::FromMilliseconds(10);
-    params[0].sampling_interval = TimeDelta::FromMilliseconds(0);
-    params[0].samples_per_burst = 1;
+    params[0].sampling_interval = TimeDelta::FromMilliseconds(1);
+    params[0].samples_per_burst = 9;
 
-    params[1].sampling_interval = TimeDelta::FromMilliseconds(0);
-    params[1].samples_per_burst = 1;
+    params[1].initial_delay = TimeDelta::FromMilliseconds(11);
+    params[1].sampling_interval = TimeDelta::FromMilliseconds(1);
+    params[1].samples_per_burst = 8;
 
     CallStackProfiles profiles[2];
-    std::vector<std::unique_ptr<WaitableEvent>> sampling_completed(2);
-    std::vector<std::unique_ptr<StackSamplingProfiler>> profiler(2);
-    for (int i = 0; i < 2; ++i) {
-      sampling_completed[i] =
-          MakeUnique<WaitableEvent>(WaitableEvent::ResetPolicy::AUTOMATIC,
-                                    WaitableEvent::InitialState::NOT_SIGNALED);
-      const StackSamplingProfiler::CompletedCallback callback =
-          Bind(&SaveProfilesAndSignalEvent, Unretained(&profiles[i]),
-               Unretained(sampling_completed[i].get()));
-      profiler[i] = MakeUnique<StackSamplingProfiler>(target_thread_id,
-                                                      params[i], callback);
-    }
+    std::vector<std::unique_ptr<WaitableEvent>> sampling_completed(
+        arraysize(params));
+    std::vector<std::unique_ptr<StackSamplingProfiler>> profiler(
+        arraysize(params));
+    CreateProfilers(target_thread_id, &profiler[0], &sampling_completed[0],
+                    profiles, params, arraysize(params));
 
     profiler[0]->Start();
     profiler[1]->Start();
 
-    std::vector<WaitableEvent*> sampling_completed_rawptrs(
-        sampling_completed.size());
-    std::transform(
-        sampling_completed.begin(), sampling_completed.end(),
-        sampling_completed_rawptrs.begin(),
-        [](const std::unique_ptr<WaitableEvent>& elem) { return elem.get(); });
     // Wait for one profiler to finish.
-    size_t completed_profiler =
-        WaitableEvent::WaitMany(sampling_completed_rawptrs.data(), 2);
+    size_t completed_profiler = WaitForSamplingComplete(&sampling_completed);
     EXPECT_EQ(1u, profiles[completed_profiler].size());
 
     size_t other_profiler = 1 - completed_profiler;
-    // Give the other profiler a chance to run and observe that it hasn't.
-    EXPECT_FALSE(sampling_completed[other_profiler]->TimedWait(
-        TimeDelta::FromMilliseconds(25)));
-
-    // Start the other profiler again and it should run.
-    profiler[other_profiler]->Start();
+    // Wait for the other profiler to finish.
     sampling_completed[other_profiler]->Wait();
     EXPECT_EQ(1u, profiles[other_profiler].size());
+
+    // Ensure each got the correct number of samples.
+    EXPECT_EQ(9u, profiles[0][0].samples.size());
+    EXPECT_EQ(8u, profiles[1][0].samples.size());
+  });
+}
+
+// Checks that several mixed sampling requests execute in parallel.
+#if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
+#define MAYBE_ConcurrentProfiling_Mixed ConcurrentProfiling_Mixed
+#else
+#define MAYBE_ConcurrentProfiling_Mixed DISABLED_ConcurrentProfiling_Mixed
+#endif
+TEST(StackSamplingProfilerTest, MAYBE_ConcurrentProfiling_Mixed) {
+  WithTargetThread([](PlatformThreadId target_thread_id) {
+    SamplingParams params[3];
+    params[0].initial_delay = TimeDelta::FromMilliseconds(8);
+    params[0].sampling_interval = TimeDelta::FromMilliseconds(4);
+    params[0].samples_per_burst = 10;
+
+    params[1].initial_delay = TimeDelta::FromMilliseconds(9);
+    params[1].sampling_interval = TimeDelta::FromMilliseconds(3);
+    params[1].samples_per_burst = 10;
+
+    params[2].initial_delay = TimeDelta::FromMilliseconds(10);
+    params[2].sampling_interval = TimeDelta::FromMilliseconds(2);
+    params[2].samples_per_burst = 10;
+
+    CallStackProfiles profiles[arraysize(params)];
+    std::vector<std::unique_ptr<WaitableEvent>> sampling_completed(
+        arraysize(params));
+    std::vector<std::unique_ptr<StackSamplingProfiler>> profiler(
+        arraysize(params));
+    CreateProfilers(target_thread_id, &profiler[0], &sampling_completed[0],
+                    profiles, params, arraysize(params));
+
+    for (size_t i = 0; i < profiler.size(); ++i)
+      profiler[i]->Start();
+
+    // Wait for one profiler to finish.
+    size_t completed_profiler = WaitForSamplingComplete(&sampling_completed);
+    EXPECT_EQ(1u, profiles[completed_profiler].size());
+    // Stop and destroy all profilers, always in the same order. Don't crash.
+    for (size_t i = 0; i < profiler.size(); ++i)
+      profiler[i]->Stop();
+    for (size_t i = 0; i < profiler.size(); ++i)
+      profiler[i].reset();
+  });
+}
+
+// Checks that sampling requests execute in a staggered manner.
+#if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
+#define MAYBE_ConcurrentProfiling_Staggered ConcurrentProfiling_Staggered
+#else
+#define MAYBE_ConcurrentProfiling_Staggered \
+  DISABLED_ConcurrentProfiling_Staggered
+#endif
+TEST(StackSamplingProfilerTest, MAYBE_ConcurrentProfiling_Staggered) {
+  WithTargetThread([](PlatformThreadId target_thread_id) {
+    SamplingParams params[3];
+    params[0].initial_delay = TimeDelta::FromMilliseconds(10);
+    params[0].sampling_interval = TimeDelta::FromMilliseconds(10);
+    params[0].samples_per_burst = 1;
+
+    params[1].initial_delay = TimeDelta::FromMilliseconds(5);
+    params[1].sampling_interval = TimeDelta::FromMilliseconds(10);
+    params[1].samples_per_burst = 2;
+
+    params[2].initial_delay = TimeDelta::FromMilliseconds(0);
+    params[2].sampling_interval = TimeDelta::FromMilliseconds(10);
+    params[2].samples_per_burst = 3;
+
+    CallStackProfiles profiles[arraysize(params)];
+    std::vector<std::unique_ptr<WaitableEvent>> sampling_completed(
+        arraysize(params));
+    std::vector<std::unique_ptr<StackSamplingProfiler>> profiler(
+        arraysize(params));
+    CreateProfilers(target_thread_id, &profiler[0], &sampling_completed[0],
+                    profiles, params, arraysize(params));
+
+    profiler[0]->Start();

[Mike Wittman, 2017/03/18 01:38:41]

How does the ordering of the Start and Stop calls and destruction relate to the
parameters above?

[bcwhite, 2017/03/20 21:50:51]

They don't.  It's three different sampling parameters that start in a staggered
ordering.

[Mike Wittman, 2017/03/21 16:50:38]

In that case, if there's still a motivation for this ordering then it should be
documented and moved to its own test since it's independent. If not, then this
can be simplified to three Start() calls followed by three wait calls and remove
the Stop() calls.

[bcwhite, 2017/03/22 17:48:54]

No motivation, per say.  If you don't feel that staggered start/stop calls are
of any use then I'll just remove the test because it would become essentially
the same as the other ConcurrentProfiling tests.

[Mike Wittman, 2017/03/23 22:18:31]

Removing SGTM. Any specific behaviors exercised by staggered stop/stop would be
better addressed in focused tests.

[bcwhite, 2017/03/27 17:52:43]

Done.

+    profiler[1]->Start();
+    sampling_completed[0]->Wait();
+    profiler[2]->Start();
+    profiler[0]->Stop();
+    profiler[1]->Stop();
+    sampling_completed[1]->Wait();
+    sampling_completed[2]->Wait();
+    EXPECT_EQ(1u, profiles[0].size());
+    EXPECT_EQ(1u, profiles[1].size());
+    EXPECT_EQ(1u, profiles[2].size());
   });
 }