Support parallel captures from the StackSamplingProfiler.

base/profiler/stack_sampling_profiler_unittest.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 <stddef.h>
 #include <stdint.h>
 
 #include <cstdlib>
 #include <memory>
 #include <utility>
 #include <vector>
 
+#include "base/atomicops.h"
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
-#include "base/message_loop/message_loop.h"
 #include "base/native_library.h"
 #include "base/path_service.h"
 #include "base/profiler/native_stack_sampler.h"
 #include "base/profiler/stack_sampling_profiler.h"
 #include "base/run_loop.h"
 #include "base/scoped_native_library.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/platform_thread.h"
@@ skipping 316 matching lines @@
   target_thread.SignalThreadToFinish();
 
   PlatformThread::Join(target_thread_handle);
 }
 
 template <class Function>
 void WithTargetThread(Function function) {
   WithTargetThread(function, StackConfiguration(StackConfiguration::NORMAL));
 }
 
+// Waits for one of multiple samplings to complete. If |delegates| is provided,
+// the vector must be pre-populated and of the same size as |params|.
+void CreateProfilers(
+    PlatformThreadId target_thread_id,
+    const std::vector<SamplingParams>& params,
+    std::vector<std::unique_ptr<NativeStackSamplerTestDelegate>>* delegates,
+    std::vector<CallStackProfiles>* profiles,
+    std::vector<std::unique_ptr<StackSamplingProfiler>>* profilers,
+    std::vector<std::unique_ptr<WaitableEvent>>* completed) {
+  ASSERT_TRUE(!params.empty());
+  ASSERT_TRUE(profiles->empty());
+  ASSERT_TRUE(profilers->empty());
+  ASSERT_TRUE(completed->empty());
+  if (delegates)
+    ASSERT_EQ(params.size(), delegates->size());
+
+  // Vectors have to be appropriately sized in advance so that the addresses of
+  // values don't change.
+  const size_t count = params.size();
+  profiles->reserve(count);
+  profilers->reserve(count);
+  completed->reserve(count);
+
+  for (size_t i = 0; i < count; ++i) {
+    profiles->push_back(CallStackProfiles());
+    completed->push_back(
+        MakeUnique<WaitableEvent>(WaitableEvent::ResetPolicy::AUTOMATIC,
+                                  WaitableEvent::InitialState::NOT_SIGNALED));
+    const StackSamplingProfiler::CompletedCallback callback =
+        Bind(&SaveProfilesAndSignalEvent, Unretained(&profiles->back()),
+             Unretained(completed->back().get()));
+    if (delegates) {

[Mike Wittman, 2017/03/28 19:32:12]

shorter:
profilers->push_back(MakeUnique<StackSamplingProfiler>(target_thread_id,
params[i], callback, delegates ? (*delegates[i]).get() : nullptr));

[bcwhite, 2017/03/29 14:56:58]

Done.

+      profilers->push_back(MakeUnique<StackSamplingProfiler>(
+          target_thread_id, params[i], callback, delegates->at(i).get()));

[Mike Wittman, 2017/03/28 19:32:02]

(*delegates)[i].get()

vector<>::at() is no different than operator[]() since Chrome builds without
exceptions, and is more confusing to read for the same reason.

[bcwhite, 2017/03/29 14:56:59]

at() is required when working with const vectors (though I had left that out
from the parameter definition).

[Mike Wittman, 2017/03/30 16:18:38]

The const change is good. at() is not required. vector<>::operator[] has a const
overload.

[bcwhite, 2017/03/30 18:54:51]

Right.  It's maps that don't allow [] with const.

+    } else {
+      profilers->push_back(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.
 void CaptureProfiles(const SamplingParams& params, TimeDelta profiler_wait_time,
                      CallStackProfiles* profiles) {
   profiles->clear();
 
   WithTargetThread([&params, profiles,
                     profiler_wait_time](PlatformThreadId target_thread_id) {
     WaitableEvent sampling_thread_completed(
         WaitableEvent::ResetPolicy::MANUAL,
         WaitableEvent::InitialState::NOT_SIGNALED);
     const StackSamplingProfiler::CompletedCallback callback =
         Bind(&SaveProfilesAndSignalEvent, Unretained(profiles),
              Unretained(&sampling_thread_completed));
     StackSamplingProfiler profiler(target_thread_id, params, callback);
     profiler.Start();
     sampling_thread_completed.TimedWait(profiler_wait_time);
     profiler.Stop();
     sampling_thread_completed.Wait();
   });
 }
 
+// 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
 // Table that jump to the functions. Checks for a jump instruction and if
 // present does a little decompilation to find the function's actual starting
 // address.
 const void* MaybeFixupFunctionAddressForILT(const void* function_address) {
 #if defined(_WIN64)
   const unsigned char* opcode =
       reinterpret_cast<const unsigned char*>(function_address);
@@ skipping 50 matching lines @@
 // asynchronous library loading has completed before walking the stack. If
 // false, the unloading may still be occurring during the stack walk.
 void TestLibraryUnload(bool wait_until_unloaded) {
   // Test delegate that supports intervening between the copying of the stack
   // and the walking of the stack.
   class StackCopiedSignaler : public NativeStackSamplerTestDelegate {
    public:
     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();
       if (wait_to_walk_stack_)
         start_stack_walk_->Wait();
     }
 
    private:
     WaitableEvent* const stack_copied_;
     WaitableEvent* const start_stack_walk_;
@@ skipping 120 matching lines @@
 }  // namespace
 
 // Checks that the basic expected information is present in a sampled call stack
 // profile.
 #if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
 #define MAYBE_Basic Basic
 #else
 #define MAYBE_Basic DISABLED_Basic
 #endif
 TEST(StackSamplingProfilerTest, MAYBE_Basic) {
+  StackSamplingProfiler::TestAPI::Reset();

[Mike Wittman, 2017/03/28 19:32:07]

Why do only some of the tests use Reset()?

[bcwhite, 2017/03/29 14:56:58]

It's only necessary for tests that deal with the startup of the sampling thread.
 I'd prefer it on all but figured you'd claim it was not required.  Added
everywhere.

[Mike Wittman, 2017/03/30 16:18:38]

If we need to clean up state to make the test runs hermetic, which it sounds
like we do, then ensuring every test executes with the expected state is the
appropriate thing to do. It prevents mysterious failures when people add tests
in the future.

The expected way to accomplish this is to use a test fixture and do the cleanup
in the TearDown method. The relevant principle is that tests should clean up
after themselves.

[bcwhite, 2017/03/30 18:54:51]

Done.

   StackSamplingProfiler::ResetAnnotationsForTesting();
 
   SamplingParams params;
   params.sampling_interval = TimeDelta::FromMilliseconds(0);
   params.samples_per_burst = 1;
 
   std::vector<CallStackProfile> profiles;
   CaptureProfiles(params, AVeryLongTimeDelta(), &profiles);
 
   // Check that the profile and samples sizes are correct, and the module
@@ skipping 113 matching lines @@
              reinterpret_cast<const void*>(&TargetThread::CallWithAlloca))
       << " was not found in stack:\n"
       << FormatSampleForDiagnosticOutput(sample, profile.modules);
 
   // These frames should be adjacent on the stack.
   EXPECT_EQ(1, alloca_frame - end_frame)
       << "Stack:\n"
       << 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)
 #define MAYBE_MultipleProfilesAndSamples MultipleProfilesAndSamples
 #else
 #define MAYBE_MultipleProfilesAndSamples DISABLED_MultipleProfilesAndSamples
 #endif
 TEST(StackSamplingProfilerTest, MAYBE_MultipleProfilesAndSamples) {
   SamplingParams params;
   params.burst_interval = params.sampling_interval =
       TimeDelta::FromMilliseconds(0);
   params.bursts = 2;
   params.samples_per_burst = 3;
 
   std::vector<CallStackProfile> profiles;
   CaptureProfiles(params, AVeryLongTimeDelta(), &profiles);
 
   ASSERT_EQ(2u, profiles.size());
   EXPECT_EQ(3u, profiles[0].samples.size());
   EXPECT_EQ(3u, profiles[1].samples.size());
 }
 
+// Checks that a profiler can stop/destruct without ever having started.
+#if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
+#define MAYBE_StopWithoutStarting StopWithoutStarting
+#else
+#define MAYBE_StopWithoutStarting DISABLED_StopWithoutStarting
+#endif
+TEST(StackSamplingProfilerTest, MAYBE_StopWithoutStarting) {
+  StackSamplingProfiler::TestAPI::Reset();
+
+  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);
+
+    profiler.Stop();  // Constructed but never started.
+    EXPECT_FALSE(sampling_completed.IsSignaled());
+  });
+}
+
+// Checks that its okay to stop a profiler before it finishes.
+#if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
+#define MAYBE_StopSafely StopSafely
+#else
+#define MAYBE_StopSafely DISABLED_StopSafely
+#endif
+TEST(StackSamplingProfilerTest, MAYBE_StopSafely) {
+  StackSamplingProfiler::TestAPI::DisableIdleShutdown();
+
+  // Test delegate that counts samples.
+  class SampleRecordedCounter : public NativeStackSamplerTestDelegate {
+   public:
+    SampleRecordedCounter() {}
+
+    void OnPreStackWalk() override {
+      subtle::NoBarrier_AtomicIncrement(&count_, 1);
+    }
+
+    int Get() { return subtle::NoBarrier_Load(&count_); }
+
+   private:
+    subtle::AtomicWord count_ = 0;

[Mike Wittman, 2017/03/28 19:32:02]

This should use locks rather than atomic ops. From the atomicops.h file header:
"If you plan to use these routines, you should have a good reason, such as solid
evidence that performance would otherwise suffer, or there being no
alternative."

[bcwhite, 2017/03/29 14:56:58]

I'm relatively well versed in the subtleties of atomics, thanks.  But fine,
we'll do it your way.

[Mike Wittman, 2017/03/30 16:18:38]

You may be but readers generally won't, and we optimize for code readability,
not writeability.

This is not my way, it's the accepted guidance for using this functionality in
both Chrome and google3, and has been for 8 years.

+  };
+
+  WithTargetThread([](PlatformThreadId target_thread_id) {
+    std::vector<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 ordered by their scheduled, interleaved times regardless of
+    // whatever interval the thread wakes up.
+    params[0].initial_delay = TimeDelta::FromMilliseconds(10);
+    params[0].sampling_interval = TimeDelta::FromMilliseconds(1);
+    params[0].samples_per_burst = 100000;
+
+    params[1].initial_delay = TimeDelta::FromMilliseconds(10);
+    params[1].sampling_interval = TimeDelta::FromMilliseconds(1);
+    params[1].samples_per_burst = 100000;
+
+    std::vector<CallStackProfiles> profiles;
+    std::vector<std::unique_ptr<SampleRecordedCounter>> samples_recorded;

[Mike Wittman, 2017/03/28 19:32:02]

std::vector<std::unique_ptr<NativeStackSamplerTestDelegate>>
and access with
static_cast<SampleRecordedCounter*>(samples_recorded[0].get())

reinterpret_cast across two levels of template instantiation is highly unsafe
and dependent on multiple layers of undefined behavior.

[bcwhite, 2017/03/29 14:56:58]

I tried many different ways this was the only one that would fully compile. 
unique_ptr will down-cast automatically but not a vector of them even though
they're compatible.  Making it a native vector of the base class would mean
up-casting it with every use in this method.

[Mike Wittman, 2017/03/30 16:18:38]

Casting to SampleRecordedCounter* on use is what's required. Using
reinterpret_cast in this way is not typesafe or valid C++, and only functions at
all because std::unique_ptr<NativeStackSamplerTestDelegate> and
std::unique_ptr<SampleRecordedCounter>, and
std::vector<std::unique_ptr<NativeStackSamplerTestDelegate>>
std::vector<std::unique_ptr<SampleRecordedCounter>> happen to compile to the
same binary representations.

A reasonable rule of thumb for casting in Chrome is: use static_cast where
possible, and reinterpret_cast between pointers to POD types*. Any other case is
probably not type safe.

* with caveats: see bit_cast.

[bcwhite, 2017/03/30 18:54:51]

Done.

+    std::vector<std::unique_ptr<WaitableEvent>> sampling_completed;
+    std::vector<std::unique_ptr<StackSamplingProfiler>> profilers;
+    for (size_t i = 0; i < params.size(); ++i)
+      samples_recorded.push_back(MakeUnique<SampleRecordedCounter>());
+    CreateProfilers(
+        target_thread_id, params,
+        reinterpret_cast<
+            std::vector<std::unique_ptr<NativeStackSamplerTestDelegate>>*>(
+            &samples_recorded),
+        &profiles, &profilers, &sampling_completed);
+
+    profilers[0]->Start();
+    profilers[1]->Start();
+
+    // Wait for both to start accumulating samples.

[Mike Wittman, 2017/03/28 19:32:06]

It seems like using WaitableEvents in the test delegate would be more
appropriate here than sleeping until the relevant conditions are met.

However, I considered what it would take to do this and I think it results in a
more complicated solution. There's inherently a race between the task posted by
the Stop call and the next PerformCollectionTask on profiler 0, so it's possible
that either zero or one collections could take place on that profiler after
Stop() returns. Thus one can't know how many times to wait for collection on
profiler 0 before it stops.

Can you add a comment with this information so that future readers of this test
know why this seemingly appropriate solution doesn't work well here?

[bcwhite, 2017/03/29 14:56:58]

Done.

+    while (samples_recorded[0]->Get() == 0 || samples_recorded[1]->Get() == 0)
+      PlatformThread::Sleep(TimeDelta::FromMilliseconds(1));
+
+    // Ensure that the first sampler can be safely stopped while the second
+    // continues to run. The stopped first profiler will still have a
+    // PerformCollectionTask pending that will do nothing when executed because
+    // the collection will have been removed by Stop().
+    profilers[0]->Stop();
+    sampling_completed[0]->Wait();
+    int count0 = samples_recorded[0]->Get();
+    int count1 = samples_recorded[1]->Get();
+
+    // Waiting for the second sampler to collect a couple samples ensures that
+    // the pending PerformCollectionTask for the first has executed because
+    // tasks are always ordered by their next scheduled time.
+    while (samples_recorded[1]->Get() < count1 + 2)
+      PlatformThread::Sleep(TimeDelta::FromMilliseconds(1));
+
+    // Ensure that the first profiler didn't do anything since it was stopped.
+    EXPECT_EQ(count0, samples_recorded[0]->Get());
+  });
+}
+
 // Checks that no call stack profiles are captured if the profiling is stopped
 // during the initial delay.
 #if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
 #define MAYBE_StopDuringInitialDelay StopDuringInitialDelay
 #else
 #define MAYBE_StopDuringInitialDelay DISABLED_StopDuringInitialDelay
 #endif
 TEST(StackSamplingProfilerTest, MAYBE_StopDuringInitialDelay) {
   SamplingParams params;
   params.initial_delay = TimeDelta::FromSeconds(60);
@@ skipping 69 matching lines @@
   });
 }
 
 // Checks that the same profiler may be run multiple times.
 #if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
 #define MAYBE_CanRunMultipleTimes CanRunMultipleTimes
 #else
 #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;
 
   std::vector<CallStackProfile> profiles;
   CaptureProfiles(params, AVeryLongTimeDelta(), &profiles);
   ASSERT_EQ(1u, profiles.size());
 
   profiles.clear();
   CaptureProfiles(params, AVeryLongTimeDelta(), &profiles);
   ASSERT_EQ(1u, profiles.size());
 }
 
-// Checks that requests to start profiling while another profile is taking place
-// are ignored.
-#if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
-#define MAYBE_ConcurrentProfiling ConcurrentProfiling
-#else
-#define MAYBE_ConcurrentProfiling DISABLED_ConcurrentProfiling
-#endif
-TEST(StackSamplingProfilerTest, MAYBE_ConcurrentProfiling) {
-  WithTargetThread([](PlatformThreadId target_thread_id) {
-    SamplingParams params[2];
+// Checks that a sampler can be started while another is running.
+#if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
+#define MAYBE_MultipleStart MultipleStart
+#else
+#define MAYBE_MultipleStart DISABLED_MultipleStart
+#endif
+TEST(StackSamplingProfilerTest, MAYBE_MultipleStart) {
+  StackSamplingProfiler::TestAPI::DisableIdleShutdown();
+
+  WithTargetThread([](PlatformThreadId target_thread_id) {
+    std::vector<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 ordered by their scheduled, interleaved times regardless of
+    // whatever interval the thread wakes up. Thus, total execution time
+    // will be 10ms (delay) + 10x1ms (sampling) + 1/2 timer minimum interval.

[Mike Wittman, 2017/03/28 19:32:05]

This comment is no longer relevant and can be removed.

[bcwhite, 2017/03/29 14:56:58]

Done.

+    params[0].initial_delay = TimeDelta::FromDays(1);

[Mike Wittman, 2017/03/28 19:32:09]

AVeryLongTimeDelta() is the established way to say "effectively infinite time"
in these tests. Applies one other place below too.

[bcwhite, 2017/03/29 14:56:58]

Done.

+    params[0].sampling_interval = TimeDelta::FromMilliseconds(1);

[Mike Wittman, 2017/03/28 19:32:11]

This parameter can be removed.

[bcwhite, 2017/03/29 14:56:59]

Done.

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

[Mike Wittman, 2017/03/28 19:32:02]

This line can be removed since this is the default initial delay.

Same comment applies four other places below.

[bcwhite, 2017/03/29 14:56:59]

Done.

+    params[1].sampling_interval = TimeDelta::FromMilliseconds(1);
+    params[1].samples_per_burst = 1;
+
+    std::vector<CallStackProfiles> profiles;
+    std::vector<std::unique_ptr<WaitableEvent>> sampling_completed;
+    std::vector<std::unique_ptr<StackSamplingProfiler>> profilers;
+    CreateProfilers(target_thread_id, params, nullptr, &profiles, &profilers,
+                    &sampling_completed);
+
+    profilers[0]->Start();
+    profilers[1]->Start();

[Mike Wittman, 2017/03/28 19:32:08]

We should wait on the second profiler to finish and check that it got the right
data, to validate that the Start() call succeeded.

[bcwhite, 2017/03/29 14:56:58]

Done.

+    EXPECT_FALSE(sampling_completed[0]->IsSignaled());

[Mike Wittman, 2017/03/28 19:32:08]

This should be removed since what it's checking is unrelated to the behavior
under test.

[bcwhite, 2017/03/29 14:56:58]

Done.

+  });
+}
+
+// Checks that the sampling thread can shut down.
+#if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
+#define MAYBE_SamplerIdleShutdown SamplerIdleShutdown
+#else
+#define MAYBE_SamplerIdleShutdown DISABLED_SamplerIdleShutdown
+#endif
+TEST(StackSamplingProfilerTest, MAYBE_SamplerIdleShutdown) {
+  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 and ensure it happens. Idle-shutdown was
+  // disabled above so the test will fail due to a timeout if it does not
+  // exit.
+  StackSamplingProfiler::TestAPI::PerformSamplingThreadIdleShutdown(false);
+
+  // While the shutdown has been initiated, the actual exit of the thread still
+  // happens asynchronously. Watch until the thread actually exits. This test
+  // will time-out in the case of failure.
+  while (StackSamplingProfiler::TestAPI::IsSamplingThreadRunning())
+    PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(1));
+}
+
+// Checks that additional requests will restart a stopped profiler.
+#if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
+#define MAYBE_WillRestartSamplerAfterIdleShutdown \
+  WillRestartSamplerAfterIdleShutdown
+#else
+#define MAYBE_WillRestartSamplerAfterIdleShutdown \
+  DISABLED_WillRestartSamplerAfterIdleShutdown
+#endif
+TEST(StackSamplingProfilerTest, MAYBE_WillRestartSamplerAfterIdleShutdown) {
+  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());
+
+  // Post a ShutdownTask on the sampling thread which, when executed, will
+  // mark the thread as EXITING and begin shut down of the thread.
+  StackSamplingProfiler::TestAPI::PerformSamplingThreadIdleShutdown(false);
+
+  // 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_StopAfterIdleShutdown StopAfterIdleShutdown
+#else
+#define MAYBE_StopAfterIdleShutdown DISABLED_StopAfterIdleShutdown
+#endif
+TEST(StackSamplingProfilerTest, MAYBE_StopAfterIdleShutdown) {
+  StackSamplingProfiler::TestAPI::DisableIdleShutdown();
+
+  WithTargetThread([](PlatformThreadId target_thread_id) {
+    std::vector<SamplingParams> params(1);

[Mike Wittman, 2017/03/28 19:32:04]

There's no need for vectors here since there's only one profiler.

[bcwhite, 2017/03/29 14:56:58]

CreateProfilers() takes a vector so this allows code-reuse.

[Mike Wittman, 2017/03/30 16:18:38]

Yes, but we already have several instances where scalar profilers are created
directly within tests, so this would be the one inconsistency in the code.

[bcwhite, 2017/03/30 18:54:51]

Usually where it's being passed to CaptureProfiles, which does all the
heavy-lifting of creating the profiler.  Here I'm using CreateProfilers to do
the heavy-lifting and it takes vectors.  It's much more difficult to make
mistakes by re-using CreateProfilers even if it makes a bit more boiler-plate
code in the test.

[Mike Wittman, 2017/03/31 01:38:21]

OK, sounds reasonable.

+
+    params[0].initial_delay = TimeDelta::FromMilliseconds(0);
+    params[0].sampling_interval = TimeDelta::FromMilliseconds(1);
+    params[0].samples_per_burst = 1;
+
+    std::vector<CallStackProfiles> profiles;
+    std::vector<std::unique_ptr<WaitableEvent>> sampling_completed;
+    std::vector<std::unique_ptr<StackSamplingProfiler>> profilers;
+    CreateProfilers(target_thread_id, params, nullptr, &profiles, &profilers,
+                    &sampling_completed);
+
+    profilers[0]->Start();
+    sampling_completed[0]->Wait();
+
+    // Capture thread should still be running at this point.
+    ASSERT_TRUE(StackSamplingProfiler::TestAPI::IsSamplingThreadRunning());
+
+    // Perform an idle shutdown.
+    StackSamplingProfiler::TestAPI::PerformSamplingThreadIdleShutdown(false);
+
+    // Stop should be safe though its impossible to know at this moment if the
+    // sampling thread has completely exited or will just "stop soon".
+    profilers[0]->Stop();
+  });
+}
+
+// Checks that profilers can run both before and after the sampling thread has
+// started.
+#if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
+#define MAYBE_ProfileBeforeAndAfterSamplingThreadRunning \
+  ProfileBeforeAndAfterSamplingThreadRunning
+#else
+#define MAYBE_ProfileBeforeAndAfterSamplingThreadRunning \
+  DISABLED_ProfileBeforeAndAfterSamplingThreadRunning
+#endif
+TEST(StackSamplingProfilerTest,
+     MAYBE_ProfileBeforeAndAfterSamplingThreadRunning) {
+  StackSamplingProfiler::TestAPI::Reset();
+  StackSamplingProfiler::TestAPI::DisableIdleShutdown();
+
+  WithTargetThread([](PlatformThreadId target_thread_id) {
+    std::vector<SamplingParams> params(2);
+
+    params[0].initial_delay = TimeDelta::FromDays(1);
+    params[0].sampling_interval = TimeDelta::FromMilliseconds(1);
+    params[0].samples_per_burst = 1;
+
+    params[1].initial_delay = TimeDelta::FromMilliseconds(0);
+    params[1].sampling_interval = TimeDelta::FromMilliseconds(1);
+    params[1].samples_per_burst = 1;
+
+    std::vector<CallStackProfiles> profiles;
+    std::vector<std::unique_ptr<WaitableEvent>> sampling_completed;
+    std::vector<std::unique_ptr<StackSamplingProfiler>> profilers;
+    CreateProfilers(target_thread_id, params, nullptr, &profiles, &profilers,
+                    &sampling_completed);
+
+    // First profiler is started when there has never been a sampling thread.
+    EXPECT_FALSE(StackSamplingProfiler::TestAPI::IsSamplingThreadRunning());
+    profilers[0]->Start();
+    // Second profiler is started when sampling thread is already running.
+    EXPECT_TRUE(StackSamplingProfiler::TestAPI::IsSamplingThreadRunning());
+    profilers[1]->Start();
+
+    // Only the second profiler should finish before test times out.
+    size_t completed_profiler = WaitForSamplingComplete(&sampling_completed);
+    EXPECT_EQ(1U, completed_profiler);
+  });
+}
+
+// Checks that an idle-shutdown task will abort if a new profiler starts
+// between when it was posted and when it runs.
+#if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
+#define MAYBE_IdleShutdownAbort IdleShutdownAbort
+#else
+#define MAYBE_IdleShutdownAbort DISABLED_IdleShutdownAbort
+#endif
+TEST(StackSamplingProfilerTest, MAYBE_IdleShutdownAbort) {
+  StackSamplingProfiler::TestAPI::DisableIdleShutdown();
+
+  WithTargetThread([](PlatformThreadId target_thread_id) {
+    std::vector<SamplingParams> params(1);
+
+    params[0].initial_delay = TimeDelta::FromMilliseconds(0);
+    params[0].sampling_interval = TimeDelta::FromMilliseconds(1);
+    params[0].samples_per_burst = 1;
+
+    std::vector<CallStackProfiles> profiles;
+    std::vector<std::unique_ptr<WaitableEvent>> sampling_completed;
+    std::vector<std::unique_ptr<StackSamplingProfiler>> profilers;
+    CreateProfilers(target_thread_id, params, nullptr, &profiles, &profilers,
+                    &sampling_completed);
+
+    profilers[0]->Start();
+    sampling_completed[0]->Wait();
+
+    // Perform an idle shutdown but simulate that a new capture is started
+    // before it can actually run.
+    StackSamplingProfiler::TestAPI::PerformSamplingThreadIdleShutdown(true);
+
+    // Though the shutdown-task has been executed, any actual exit of the

[Mike Wittman, 2017/03/28 19:32:02]

Since we can't reliably validate that the thread was not stopped, I think the
best way to check the behavior is to collect another profile using a second
profiler and validate that it works as expected. This is the expected
user-observable behavior anyway.

[bcwhite, 2017/03/29 14:56:58]

If the sampling thread did stop (when it shouldn't) then starting a new
collection would quietly restart it.  Thus, the test would always pass.

[Mike Wittman, 2017/03/30 16:18:38]

It's still worth checking this to prevent future regressions in user-observable
behavior on this code path.

[bcwhite, 2017/03/30 18:54:51]

That is either WillRestartSamplerAfterIdleShutdown or CanRunMultipleTimes.

[Mike Wittman, 2017/03/31 01:38:21]

Neither of those tests exercise the shutdown abort code path.

[bcwhite, 2017/03/31 13:57:56]

Done.

+    // thread is asynchronous so there is no way to detect that *didn't* exit
+    // except to wait a reasonable amount of time and then check. Since the
+    // thread was just running ("perform" blocked until it was), it should
+    // finish almost immediately and without any waiting for tasks or events.
+    PlatformThread::Sleep(base::TimeDelta::FromMilliseconds(200));
+    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) {
+  StackSamplingProfiler::TestAPI::DisableIdleShutdown();
+
+  WithTargetThread([](PlatformThreadId target_thread_id) {
+    std::vector<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 ordered by their scheduled, interleaved times regardless of
+    // whatever interval the thread wakes up. Thus, total execution time
+    // will be 10ms (delay) + 10x1ms (sampling) + 1/2 timer minimum interval.
     params[0].initial_delay = TimeDelta::FromMilliseconds(10);
-    params[0].sampling_interval = TimeDelta::FromMilliseconds(0);
-    params[0].samples_per_burst = 1;
-
-    params[1].sampling_interval = TimeDelta::FromMilliseconds(0);
-    params[1].samples_per_burst = 1;
-
-    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);
-    }
-
-    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(); });
+    params[0].sampling_interval = TimeDelta::FromMilliseconds(1);
+    params[0].samples_per_burst = 9;
+
+    params[1].initial_delay = TimeDelta::FromMilliseconds(11);
+    params[1].sampling_interval = TimeDelta::FromMilliseconds(1);
+    params[1].samples_per_burst = 8;
+
+    std::vector<CallStackProfiles> profiles;
+    std::vector<std::unique_ptr<WaitableEvent>> sampling_completed;
+    std::vector<std::unique_ptr<StackSamplingProfiler>> profilers;
+    CreateProfilers(target_thread_id, params, nullptr, &profiles, &profilers,
+                    &sampling_completed);
+
+    profilers[0]->Start();
+    profilers[1]->Start();
+
     // Wait for one profiler to finish.
-    size_t completed_profiler =
-        WaitableEvent::WaitMany(sampling_completed_rawptrs.data(), 2);
+    size_t completed_profiler = WaitForSamplingComplete(&sampling_completed);
+    ASSERT_EQ(1u, profiles[completed_profiler].size());
+
+    size_t other_profiler = 1 - completed_profiler;
+    // Wait for the other profiler to finish.
+    sampling_completed[other_profiler]->Wait();
+    ASSERT_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) {
+  StackSamplingProfiler::TestAPI::DisableIdleShutdown();
+
+  WithTargetThread([](PlatformThreadId target_thread_id) {
+    std::vector<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;
+
+    std::vector<CallStackProfiles> profiles;
+    std::vector<std::unique_ptr<WaitableEvent>> sampling_completed;
+    std::vector<std::unique_ptr<StackSamplingProfiler>> profilers;
+    CreateProfilers(target_thread_id, params, nullptr, &profiles, &profilers,
+                    &sampling_completed);
+
+    for (size_t i = 0; i < profilers.size(); ++i)
+      profilers[i]->Start();
+
+    // Wait for one profiler to finish.
+    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();
-    sampling_completed[other_profiler]->Wait();
-    EXPECT_EQ(1u, profiles[other_profiler].size());
-  });
-}
-
+    // Stop and destroy all profilers, always in the same order. Don't crash.
+    for (size_t i = 0; i < profilers.size(); ++i)
+      profilers[i]->Stop();
+    for (size_t i = 0; i < profilers.size(); ++i)
+      profilers[i].reset();
+  });
+}
+
 // Checks that a stack that runs through another library produces a stack with
 // the expected functions.
 #if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
 #define MAYBE_OtherLibrary OtherLibrary
 #else
 #define MAYBE_OtherLibrary DISABLED_OtherLibrary
 #endif
 TEST(StackSamplingProfilerTest, MAYBE_OtherLibrary) {
   SamplingParams params;
   params.sampling_interval = TimeDelta::FromMilliseconds(0);
@@ skipping 74 matching lines @@
 #if defined(STACK_SAMPLING_PROFILER_SUPPORTED)
 #define MAYBE_UnloadedLibrary UnloadedLibrary
 #else
 #define MAYBE_UnloadedLibrary DISABLED_UnloadedLibrary
 #endif
 TEST(StackSamplingProfilerTest, MAYBE_UnloadedLibrary) {
   TestLibraryUnload(true);
 }
 
 }  // namespace base