StackSamplingProfiler clean up

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 57b7b355a080923b2acc4b2201d4e58d7bb666ab..5d9a904945542186a1d8bf4ab1f54917ee00464a 100644
--- a/base/profiler/stack_sampling_profiler.cc
+++ b/base/profiler/stack_sampling_profiler.cc
@@ -13,27 +13,32 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/timer/elapsed_timer.h"
 
-template <typename T> struct DefaultSingletonTraits;
-
 namespace base {
 
+// PendingProfiles ------------------------------------------------------------
+
 namespace {
 
 // Thread-safe singleton class that stores collected profiles waiting to be
 // processed.
 class PendingProfiles {
  public:
-  PendingProfiles();
   ~PendingProfiles();
 
   static PendingProfiles* GetInstance();
 
-  // Appends |profiles|. This function is thread safe.
-  void PutProfiles(const std::vector<StackSamplingProfiler::Profile>& profiles);
-  // Gets the pending profiles into *|profiles|. This function is thread safe.
-  void GetProfiles(std::vector<StackSamplingProfiler::Profile>* profiles);
+  // Appends |profiles| to |profiles_|. This function is thread safe.
+  void AppendProfiles(
+      const std::vector<StackSamplingProfiler::Profile>& profiles);
+
+  // Moves the pending profiles from |profiles_| into *|profiles|. This function
+  // is thread safe.
+  void MoveProfiles(std::vector<StackSamplingProfiler::Profile>* profiles);
 
  private:
+  PendingProfiles();
+  friend struct DefaultSingletonTraits<PendingProfiles>;
+
   Lock profiles_lock_;
   std::vector<StackSamplingProfiler::Profile> profiles_;
 
@@ -49,47 +54,56 @@ PendingProfiles* PendingProfiles::GetInstance() {
   return Singleton<PendingProfiles>::get();
 }
 
-void PendingProfiles::PutProfiles(
+void PendingProfiles::AppendProfiles(
     const std::vector<StackSamplingProfiler::Profile>& profiles) {
   AutoLock scoped_lock(profiles_lock_);
   profiles_.insert(profiles_.end(), profiles.begin(), profiles.end());
 }
 
-void PendingProfiles::GetProfiles(
+void PendingProfiles::MoveProfiles(
     std::vector<StackSamplingProfiler::Profile>* profiles) {
   profiles->clear();
 
   AutoLock scoped_lock(profiles_lock_);
   profiles_.swap(*profiles);
 }
+
 }  // namespace
 
+// StackSamplingProfiler::Module ----------------------------------------------
+
 StackSamplingProfiler::Module::Module() : base_address(nullptr) {}
 
 StackSamplingProfiler::Module::~Module() {}
 
-StackSamplingProfiler::Frame::Frame()
-    : instruction_pointer(nullptr),
-      module_index(-1) {}
+// StackSamplingProfiler::Frame -----------------------------------------------
+
+StackSamplingProfiler::Frame::Frame(const void* instruction_pointer,
+                                    size_t module_index)
+    : instruction_pointer(instruction_pointer),
+      module_index(module_index) {}
 
 StackSamplingProfiler::Frame::~Frame() {}
 
+// StackSamplingProfiler::Profile ---------------------------------------------
+
 StackSamplingProfiler::Profile::Profile() : preserve_sample_ordering(false) {}
 
 StackSamplingProfiler::Profile::~Profile() {}
 
+// StackSamplingProfiler::SamplingThread --------------------------------------
+
 class StackSamplingProfiler::SamplingThread : public PlatformThread::Delegate {
  public:
   // Samples stacks using |native_sampler|. When complete, invokes
-  // |profiles_callback| with the collected profiles. |profiles_callback| must
-  // be thread-safe and may consume the contents of the vector.
-  SamplingThread(
-      scoped_ptr<NativeStackSampler> native_sampler,
-      const SamplingParams& params,
-      Callback<void(const std::vector<Profile>&)> completed_callback);
+  // |completed_callback| with the collected profiles. |completed_callback| must
+  // be thread-safe.
+  SamplingThread(scoped_ptr<NativeStackSampler> native_sampler,
+                 const SamplingParams& params,
+                 CompletedCallback completed_callback);
   ~SamplingThread() override;
 
-  // Implementation of PlatformThread::Delegate:
+  // PlatformThread::Delegate:
   void ThreadMain() override;
 
   void Stop();
@@ -98,17 +112,19 @@ class StackSamplingProfiler::SamplingThread : public PlatformThread::Delegate {
   // Collects a profile from a single burst. Returns true if the profile was
   // collected, or false if collection was stopped before it completed.
   bool CollectProfile(Profile* profile, TimeDelta* elapsed_time);
+
   // Collects profiles from all bursts, or until the sampling is stopped. If
-  // stopped before complete, |profiles| will contains only full bursts.
+  // stopped before complete, |profiles| will contain only full bursts.
   void CollectProfiles(std::vector<Profile>* profiles);
 
   scoped_ptr<NativeStackSampler> native_sampler_;
-
   const SamplingParams params_;
 
+  // If Stop() is called, it signals this event to force the sampling to
+  // terminate before all the samples specified in |params_| are collected.
   WaitableEvent stop_event_;
 
-  Callback<void(const std::vector<Profile>&)> completed_callback_;
+  CompletedCallback completed_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(SamplingThread);
 };
@@ -116,7 +132,7 @@ class StackSamplingProfiler::SamplingThread : public PlatformThread::Delegate {
 StackSamplingProfiler::SamplingThread::SamplingThread(
     scoped_ptr<NativeStackSampler> native_sampler,
     const SamplingParams& params,
-    Callback<void(const std::vector<Profile>&)> completed_callback)
+    CompletedCallback completed_callback)
     : native_sampler_(native_sampler.Pass()),
       params_(params),
       stop_event_(false, false),
@@ -133,6 +149,13 @@ void StackSamplingProfiler::SamplingThread::ThreadMain() {
   completed_callback_.Run(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
+// adhereing 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.
 bool StackSamplingProfiler::SamplingThread::CollectProfile(
     Profile* profile,
     TimeDelta* elapsed_time) {
@@ -140,32 +163,38 @@ bool StackSamplingProfiler::SamplingThread::CollectProfile(
   Profile current_profile;
   native_sampler_->ProfileRecordingStarting(&current_profile);
   current_profile.sampling_period = params_.sampling_interval;
-  bool stopped_early = false;
+  bool burst_completed = true;
+  TimeDelta previous_elapsed_sample_time;
   for (int i = 0; i < params_.samples_per_burst; ++i) {
-    ElapsedTimer sample_timer;
-    current_profile.samples.push_back(Sample());
-    native_sampler_->RecordStackSample(&current_profile.samples.back());
-    TimeDelta elapsed_sample_time = sample_timer.Elapsed();
-    if (i != params_.samples_per_burst - 1) {
+    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 - elapsed_sample_time,
+              std::max(params_.sampling_interval - previous_elapsed_sample_time,
                        TimeDelta()))) {
-        stopped_early = true;
+        burst_completed = false;
         break;
       }
     }
+    ElapsedTimer sample_timer;
+    current_profile.samples.push_back(Sample());
+    native_sampler_->RecordStackSample(&current_profile.samples.back());
+    previous_elapsed_sample_time = sample_timer.Elapsed();
   }
 
   *elapsed_time = profile_timer.Elapsed();
   current_profile.profile_duration = *elapsed_time;
   native_sampler_->ProfileRecordingStopped();
 
-  if (!stopped_early)
+  if (burst_completed)
     *profile = current_profile;
 
-  return !stopped_early;
+  return burst_completed;
 }
 
+// In an analogous manner to CollectProfiles 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(
     std::vector<Profile>* profiles) {
   if (stop_event_.TimedWait(params_.initial_delay))
@@ -174,11 +203,12 @@ void StackSamplingProfiler::SamplingThread::CollectProfiles(
   for (int i = 0; i < params_.bursts; ++i) {
     Profile profile;
     TimeDelta elapsed_profile_time;
-    if (CollectProfile(&profile, &elapsed_profile_time))
-      profiles->push_back(profile);
-    else
+    if (!CollectProfile(&profile, &elapsed_profile_time))
       return;
+    profiles->push_back(profile);
 
+    // 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 - elapsed_profile_time,
                      TimeDelta())))
@@ -195,10 +225,14 @@ void StackSamplingProfiler::SamplingThreadDeleter::operator()(
   delete thread;
 }
 
+// StackSamplingProfiler::NativeStackSampler ----------------------------------
+
 StackSamplingProfiler::NativeStackSampler::NativeStackSampler() {}
 
 StackSamplingProfiler::NativeStackSampler::~NativeStackSampler() {}
 
+// StackSamplingProfiler ------------------------------------------------------
+
 StackSamplingProfiler::SamplingParams::SamplingParams()
     : initial_delay(TimeDelta::FromMilliseconds(0)),
       bursts(1),
@@ -223,11 +257,12 @@ void StackSamplingProfiler::Start() {
       new SamplingThread(
           native_sampler_.Pass(), params_,
           (custom_completed_callback_.is_null() ?
-           Bind(&PendingProfiles::PutProfiles,
+           Bind(&PendingProfiles::AppendProfiles,
                 Unretained(PendingProfiles::GetInstance())) :
            custom_completed_callback_)));
-  if (!PlatformThread::CreateNonJoinable(0, sampling_thread_.get()))
-    LOG(ERROR) << "failed to create thread";
+  if (!PlatformThread::CreateNonJoinable(0, sampling_thread_.get())) {
+    NOTREACHED() << "failed to create thread";

[Peter Kasting, 2015/03/27 23:44:47]

OK; just remember, NOTREACHED means this can never be reached even in
exceptional circumstances.

If you believe this can fail and you want to know why, it may be best to make it
a CHECK with a TODO() about removing the check (and, if necessary, handling
failure in a non-fatal way) once you understand the failures.

[Mike Wittman, 2015/03/30 21:01:13]

I don't know that it's worth crashing people and committing to trying to
understand possible failure modes at this point. A better approach is probably
just to bail out in the code. We should be able to monitor UMA data and see if
we're receiving fewer reports than we expect, and if it's significant we can
revisit trying to identify the cause of the problem.

+  }
 }
 
 void StackSamplingProfiler::Stop() {
@@ -237,13 +272,10 @@ void StackSamplingProfiler::Stop() {
 
 // static
 void StackSamplingProfiler::GetPendingProfiles(std::vector<Profile>* profiles) {
-  PendingProfiles::GetInstance()->GetProfiles(profiles);
+  PendingProfiles::GetInstance()->MoveProfiles(profiles);
 }
 
-void StackSamplingProfiler::SetCustomCompletedCallback(
-    Callback<void(const std::vector<Profile>&)> callback) {
-  custom_completed_callback_ = callback;
-}
+// StackSamplingProfiler::Frame global functions ------------------------------
 
 bool operator==(const StackSamplingProfiler::Frame &a,
                 const StackSamplingProfiler::Frame &b) {
@@ -259,3 +291,4 @@ bool operator<(const StackSamplingProfiler::Frame &a,
 }
 
 }  // namespace base
+