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 16205ac2b09f22e25975cda252c8306ba6eb918a..d96efc384451f25db8af766c3f3ec94ceb74a0a9 100644
--- a/base/profiler/stack_sampling_profiler_unittest.cc
+++ b/base/profiler/stack_sampling_profiler_unittest.cc
@@ -444,9 +444,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 +720,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 +831,46 @@ 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.TimedWait(AVeryLongTimeDelta());
+    profiler.Stop();
+    sampling_completed.Wait();
+    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,27 +884,62 @@ 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
 #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())
+    PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(100));
+
+  // 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 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];
     params[0].initial_delay = TimeDelta::FromMilliseconds(10);

[Mike Wittman, 2017/02/27 23:27:34]

Do we need an initial delay for this set of params (and the one below)?

[bcwhite, 2017/03/13 18:50:18]

The initial delay just provides some extra time to be confident that both are
scheduled before one starts to execute.

[Mike Wittman, 2017/03/14 18:57:33]

Ok. Please document this within the test.

[bcwhite, 2017/03/16 15:56:25]

Done.

-    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 = 10;

[Mike Wittman, 2017/02/27 23:27:35]

Can we reduce this to something like 3-5 samples (and the one below)? If the
sampling is serviced at the normal timer tick interval of 15.6ms, then the 10
samples in this test likely will take 160+ ms.

We should strive to minimize test execution time where possible.

Same comment applies to the subsequent tests as well.

[bcwhite, 2017/03/13 18:50:18]

The sampling will take 10ms + timer-resolution.  Samples are taken at strict
times and if the thread runs behind then multiple samples will be taken to
"catch up".

[Mike Wittman, 2017/03/14 18:57:33]

Please document this as well.

[bcwhite, 2017/03/16 15:56:25]

Done.

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

I don't see something equivalent to this in the comments. The text above ("The
sampling will take 10ms + ...") would be fine to just copy into the comment.

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

Done.

 
-    params[1].sampling_interval = TimeDelta::FromMilliseconds(0);
-    params[1].samples_per_burst = 1;
+    params[1].initial_delay = TimeDelta::FromMilliseconds(10);
+    params[1].sampling_interval = TimeDelta::FromMilliseconds(1);
+    params[1].samples_per_burst = 10;

[Mike Wittman, 2017/02/27 23:27:34]

We should make this value different than above, and check the number of samples
returned below, to test that we've profiled against each set of parameters once.

[bcwhite, 2017/03/13 18:50:18]

Done.

 
     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) {
+    std::vector<std::unique_ptr<WaitableEvent>> sampling_completed(
+        arraysize(params));
+    std::vector<std::unique_ptr<StackSamplingProfiler>> profiler(
+        arraysize(params));
+    for (size_t i = 0; i < arraysize(params); ++i) {
       sampling_completed[i] =
           MakeUnique<WaitableEvent>(WaitableEvent::ResetPolicy::AUTOMATIC,
                                     WaitableEvent::InitialState::NOT_SIGNALED);
@@ -914,22 +960,183 @@ TEST(StackSamplingProfilerTest, MAYBE_ConcurrentProfiling) {
         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 = WaitableEvent::WaitMany(
+        sampling_completed_rawptrs.data(), sampling_completed_rawptrs.size());

[Mike Wittman, 2017/02/27 23:27:35]

This block of code down to this line can be extracted into a utility function
and reused across all these tests.

[bcwhite, 2017/03/13 18:50:18]

Done.

     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();
-    sampling_completed[other_profiler]->Wait();
+    // Give the other profiler a chance to finish and verify it does so.
+    EXPECT_TRUE(sampling_completed[other_profiler]->TimedWait(

[Mike Wittman, 2017/02/27 23:27:35]

This should be a regular Wait() call. The test will fail with a time out if the
event is never signaled.

[bcwhite, 2017/03/13 18:50:18]

Done.

+        TimeDelta::FromMilliseconds(250)));
     EXPECT_EQ(1u, profiles[other_profiler].size());
   });
 }
 
+// Checks that interleaved multiple sampling requests execute in parallel.
+#if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
+#define MAYBE_ConcurrentProfiling_Interleaved ConcurrentProfiling_Interleaved
+#else
+#define MAYBE_ConcurrentProfiling_Interleaved \
+  DISABLED_ConcurrentProfiling_Interleaved
+#endif
+TEST(StackSamplingProfilerTest, MAYBE_ConcurrentProfiling_Interleaved) {
+  WithTargetThread([](PlatformThreadId target_thread_id) {
+    SamplingParams params[2];
+    params[0].initial_delay = TimeDelta::FromMilliseconds(1);
+    params[0].sampling_interval = TimeDelta::FromMilliseconds(2);
+    params[0].samples_per_burst = 10;
+
+    params[1].initial_delay = TimeDelta::FromMilliseconds(2);

[Mike Wittman, 2017/02/27 23:27:34]

The resolution for all waiting values is the 15.6ms timer tick interval, so this
profiler is likely to start on the same timer tick as the previous one (with 94%
probability).

The same logic applies to the sampling intervals, so this test will
substantially be testing the same behavior as the previous one, except for the
Stop behavior at the end.

As a general principle, we can't rely on wait times to force a specific
interleaving of execution. Even if things work out relative to the timer tick
interval, execution still can be delayed and push things onto the same timer
tick if the system is under load.

Given this, I think the best we can do for the ConcurrentProfiling tests is to
test various interleavings of Start/Stop/destroy calls.

e.g.:

Start() // 1
// sample
Start() // 2
// sample
Stop() // 1
// sample
Stop() // 2

Start() // 1
// sample
Start() // 2
// sample
destroy // 2
// sample
destroy // 1

where the fact that samples have occurred is validated by signaling the test
delegate.

[bcwhite, 2017/03/13 18:50:18]

It doesn't actually matter because events are posted to the queue in order based
on desired run time.  Thus, regardless of when execution is scheduled, the two
tasks will interleave.

[Mike Wittman, 2017/03/14 18:57:33]

Ok, I can see that. In that case though, it's not clear to me what the value is
in testing multiple interleavings (see my general comment below).

+    params[1].sampling_interval = TimeDelta::FromMilliseconds(2);
+    params[1].samples_per_burst = 10;

[Mike Wittman, 2017/02/27 23:27:35]

If this test is intending to exercise the stopping of one profiler, then the
params for this profiler (or the other) should be set up to ensure it executes
much longer than the other one, to try to avoid races between profiler exit and
the Stop call.

[bcwhite, 2017/03/13 18:50:18]

Races between exit and stop should be safe.  Keeping them close helps test that
edge case.  The test is just "don't crash".

[Mike Wittman, 2017/03/14 18:57:33]

If the desire is to test both winning and losing the race, that should be done
in two separate tests, each of which is written to unambiguously exercise one
case or the other.

Having a race in the test, even if benign, risks flaky failures if only one the
two cases fails.

[bcwhite, 2017/03/16 15:56:25]

The desire is to make sure that different sampling parameters will operate in
parallel.  Anything else is superfluous.

+
+    CallStackProfiles profiles[2];
+    std::vector<std::unique_ptr<WaitableEvent>> sampling_completed(
+        arraysize(params));
+    std::vector<std::unique_ptr<StackSamplingProfiler>> profiler(
+        arraysize(params));
+    for (size_t i = 0; i < arraysize(params); ++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);
+    }
+
+    for (size_t i = 0; i < profiler.size(); ++i)
+      profiler[i]->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(), sampling_completed_rawptrs.size());
+    EXPECT_EQ(1u, profiles[completed_profiler].size());
+    // Stop and destroy all the profilers. 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();

[Mike Wittman, 2017/02/27 23:27:35]

The resetting code is unnecessary; the profilers will be destroyed when
|profilers| is destroyed at the end of the block.

[bcwhite, 2017/03/13 18:50:18]

Yes but they will be destroyed in descending order (oppose of construction). I
want them destroyed in ascending order just like all other calls.

[Mike Wittman, 2017/03/14 18:57:33]

Why do you want this in this particular test? How does the ordering of the Stop
calls and destruction relate to the interleaving scenario above? Same question
applies to the tests below too.

[bcwhite, 2017/03/16 15:56:25]

It's an "interleave" test so order needs to be consistent to ensure that.

+  });
+}
+
+// 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);

[Mike Wittman, 2017/02/27 23:27:35]

Same comments here with respect to the timer tick interval and forcing specific
interleaving of execution.

[bcwhite, 2017/03/13 18:50:18]

Acknowledged.

+    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));
+    for (size_t i = 0; i < arraysize(params); ++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);
+    }
+
+    for (size_t i = 0; i < profiler.size(); ++i)
+      profiler[i]->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(), sampling_completed_rawptrs.size());
+    EXPECT_EQ(1u, profiles[completed_profiler].size());
+    // Destroy all the profilers. Don't crash.
+    for (size_t i = 0; i < profiler.size(); ++i)
+      profiler[i].reset();

[Mike Wittman, 2017/02/27 23:27:34]

This is unnecessary.

[bcwhite, 2017/03/13 18:50:18]

Acknowledged.

+  });
+}
+
+// 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));
+    for (size_t i = 0; i < arraysize(params); ++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);
+    }
+
+    profiler[0]->Start();
+    profiler[1]->Start();
+    sampling_completed[0]->Wait();
+    EXPECT_FALSE(sampling_completed[1]->IsSignaled());

[Mike Wittman, 2017/03/14 18:57:33]

The first two profilers could both complete by this point if the system is under
load, resulting in flaky failures.

[bcwhite, 2017/03/16 15:56:25]

Done.

+    profiler[2]->Start();
+    profiler[0]->Stop();
+    profiler[1]->Stop();
+    sampling_completed[1]->Wait();
+    EXPECT_FALSE(sampling_completed[2]->IsSignaled());

[Mike Wittman, 2017/03/14 18:57:33]

Same here for the second and third profilers.

[bcwhite, 2017/03/16 15:56:25]

Done.

+    sampling_completed[2]->Wait();
+    EXPECT_EQ(1u, profiles[0].size());
+    EXPECT_EQ(1u, profiles[1].size());
+    EXPECT_EQ(1u, profiles[2].size());
+  });
+}
+
 // Checks that a stack that runs through another library produces a stack with
 // the expected functions.
 #if defined(STACK_SAMPLING_PROFILER_SUPPORTED)