StackSamplingProfiler clean up

base/profiler/stack_sampling_profiler.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 "base/profiler/stack_sampling_profiler.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/memory/singleton.h"
 #include "base/synchronization/lock.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/timer/elapsed_timer.h"
 
 template <typename T> struct DefaultSingletonTraits;

[Peter Kasting, 2015/03/26 04:29:05]

Is this necessary?  I don't see a usage.

[Mike Wittman, 2015/03/27 22:42:01]

No, since I include the relevant header. But I should be using it per the
Singleton documentation. Removed, and updated PendingProfiles.

 
 namespace base {
 
 namespace {
 
 // Thread-safe singleton class that stores collected profiles waiting to be
 // processed.
 class PendingProfiles {

[Peter Kasting, 2015/03/26 04:29:06]

Nit: Since you define several different classes in this one file, consider
putting divider comments above each to make it easier to quickly scan the file. 
See e.g. chrome/browser/ui/views/passwords/manage_passwords_bubble_view.cc for
sample such comments.

If you do this, I would put the divider for PendingProfiles just above
"namespace {" and the one for Module below "}  // namespace".

[Mike Wittman, 2015/03/27 22:42:02]

Done.

  public:
   PendingProfiles();
   ~PendingProfiles();
 
   static PendingProfiles* GetInstance();
 
   // Appends |profiles|. This function is thread safe.

[Peter Kasting, 2015/03/26 04:29:06]

Nit: Maybe "Appends |profiles| to |profiles_|" or "Adds |profiles| to the stored
set of profiles" or the like?

[Mike Wittman, 2015/03/27 22:42:02]

Done.

   void PutProfiles(const std::vector<StackSamplingProfiler::Profile>& profiles);

[Peter Kasting, 2015/03/26 04:29:06]

Nit: "Put" seems like a vague verb here -- perhaps AppendProfiles() or
AddProfiles()?

[Mike Wittman, 2015/03/27 22:42:02]

Done.

   // Gets the pending profiles into *|profiles|. This function is thread safe.

[Peter Kasting, 2015/03/26 04:29:05]

Nit: Blank line above

Consider using the same comment suggestion I gave in the header re: using "move"
instead of "get".

[Mike Wittman, 2015/03/27 22:42:01]

Done.

   void GetProfiles(std::vector<StackSamplingProfiler::Profile>* profiles);
 
  private:
   Lock profiles_lock_;
   std::vector<StackSamplingProfiler::Profile> profiles_;
 
   DISALLOW_COPY_AND_ASSIGN(PendingProfiles);
 };
 
 PendingProfiles::PendingProfiles() {}
@@ skipping 11 matching lines @@
   profiles_.insert(profiles_.end(), profiles.begin(), profiles.end());
 }
 
 void PendingProfiles::GetProfiles(
     std::vector<StackSamplingProfiler::Profile>* profiles) {
   profiles->clear();
 
   AutoLock scoped_lock(profiles_lock_);
   profiles_.swap(*profiles);
 }
 }  // namespace

[Peter Kasting, 2015/03/26 04:29:06]

Nit: Blank line above

[Mike Wittman, 2015/03/27 22:42:01]

Done.

 
 StackSamplingProfiler::Module::Module() : base_address(nullptr) {}
 
 StackSamplingProfiler::Module::~Module() {}
 
 StackSamplingProfiler::Frame::Frame()
     : instruction_pointer(nullptr),
       module_index(-1) {}
 
 StackSamplingProfiler::Frame::~Frame() {}
 
 StackSamplingProfiler::Profile::Profile() : preserve_sample_ordering(false) {}
 
 StackSamplingProfiler::Profile::~Profile() {}
 
 class StackSamplingProfiler::SamplingThread : public PlatformThread::Delegate {
  public:
   // Samples stacks using |native_sampler|. When complete, invokes
   // |profiles_callback| with the collected profiles. |profiles_callback| must

[Peter Kasting, 2015/03/26 04:29:06]

Do you mean |completed_callback|?

[Mike Wittman, 2015/03/27 22:42:02]

Yes, changed. Also removed the defunct last clause of the last sentence.

   // 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);
   ~SamplingThread() override;
 
   // Implementation of PlatformThread::Delegate:

[Peter Kasting, 2015/03/26 04:29:05]

Nit: "Implementation of" is not really necessary

[Mike Wittman, 2015/03/27 22:42:02]

Done.

   void ThreadMain() override;
 
   void Stop();
 
  private:
   // 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

[Peter Kasting, 2015/03/26 04:29:06]

Nit: Blank line above

[Mike Wittman, 2015/03/27 22:42:02]

Done.

   // stopped before complete, |profiles| will contains only full bursts.

[Peter Kasting, 2015/03/26 04:29:05]

contain

[Mike Wittman, 2015/03/27 22:42:02]

Done.

   void CollectProfiles(std::vector<Profile>* profiles);
 
   scoped_ptr<NativeStackSampler> native_sampler_;
 

[Peter Kasting, 2015/03/26 04:29:05]

Nit: You can remove the blank line between two members if neither has an
explanatory comment.

[Mike Wittman, 2015/03/27 22:42:02]

Done.

   const SamplingParams params_;
 
   WaitableEvent stop_event_;

[Peter Kasting, 2015/03/26 04:29:06]

At least this member probably deserves a comment as to what it does.

[Mike Wittman, 2015/03/27 22:42:01]

Done.

 
   Callback<void(const std::vector<Profile>&)> completed_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(SamplingThread);
 };
 
 StackSamplingProfiler::SamplingThread::SamplingThread(
     scoped_ptr<NativeStackSampler> native_sampler,
     const SamplingParams& params,
     Callback<void(const std::vector<Profile>&)> completed_callback)
@@ skipping 13 matching lines @@
   completed_callback_.Run(profiles);
 }
 
 bool StackSamplingProfiler::SamplingThread::CollectProfile(
     Profile* profile,
     TimeDelta* elapsed_time) {
   ElapsedTimer profile_timer;
   Profile current_profile;
   native_sampler_->ProfileRecordingStarting(&current_profile);
   current_profile.sampling_period = params_.sampling_interval;
   bool stopped_early = false;

[Peter Kasting, 2015/03/26 04:29:05]

Nit: I'm on the fence about this, but because you only ever check the negation
of this (!stopped_early), I wonder if this should be reversed (e.g.
"burst_completed = true") to eliminate the negations below.

One is not much better than the other, so use whichever you think is maximally
simple and readable.

[Mike Wittman, 2015/03/27 22:42:02]

burst_completed is probably slightly better; updated.

   for (int i = 0; i < params_.samples_per_burst; ++i) {

[Peter Kasting, 2015/03/26 04:29:05]

If sampling takes too long/things are laggy/the sampling interval is too short,
it's possible that a burst of samples could take (arbitrarily) longer than
samples_per_burst * sampling_interval ms.

Is this problematic?  Should the loop be checking for elapsed time instead, so
that in these cases, we collect fewer samples but from close to the expected
period?  You may want to add a comment about why one route is preferable to the
other.

See also my question on the TimedWait() call below.

[Mike Wittman, 2015/03/27 22:42:02]

At this point the goal for this functionality just is to get some data flowing
through the system so that we can evaluate the usefulness of the stack sampling
profiling. Assuming it's useful, we can decide later whether this is the best
approach based on anticipated users and collected data. Added a comment
accordingly.

     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) {

[Peter Kasting, 2015/03/26 04:29:06]

Super nitpicky nit: If you move this to the top of the loop and change to "if (i
!= 0)", it's not only slightly less verbose, I think it's slightly clearer (my
brain had to think briefly about the -1 here to ensure I understood what was
happening).

[Mike Wittman, 2015/03/27 22:42:02]

Done.

       if (stop_event_.TimedWait(
               std::max(params_.sampling_interval - elapsed_sample_time,

[Peter Kasting, 2015/03/26 04:29:06]

If the elapsed time is greater than the sampling interval, is it important that
we still do the TimedWait(), e.g. because that's the only way we'll catch the
"stop" signal if the sampling interval is so tight that we can't sample fast
enough to keep up?

If not, perhaps we should just check if the elapsed time is too large, and if
so, continue the loop, skipping the 0 ms wait.

If so, this probably deserves a comment.

[Mike Wittman, 2015/03/27 22:42:02]

Yes, that's exactly the reason. We could check stop_event_.IsSignalled(),
instead in the zero wait case, but that makes the code more complex, and on
Windows IsSignalled() just does the equivalent of TimedWait(TimeDelta()) anyway.
Added a comment.

                        TimeDelta()))) {
         stopped_early = true;
         break;
       }
     }
   }
 
   *elapsed_time = profile_timer.Elapsed();
   current_profile.profile_duration = *elapsed_time;
   native_sampler_->ProfileRecordingStopped();
 
   if (!stopped_early)
     *profile = current_profile;
 
   return !stopped_early;
 }
 
 void StackSamplingProfiler::SamplingThread::CollectProfiles(
     std::vector<Profile>* profiles) {
   if (stop_event_.TimedWait(params_.initial_delay))
     return;
 
   for (int i = 0; i < params_.bursts; ++i) {

[Peter Kasting, 2015/03/26 04:29:06]

Both the questions in CollectProfile() above about timer intervals and what to
do if we can't sample fast enough apply to this loop as well.

[Mike Wittman, 2015/03/27 22:42:02]

Added a comment.

     Profile profile;
     TimeDelta elapsed_profile_time;
     if (CollectProfile(&profile, &elapsed_profile_time))

[Peter Kasting, 2015/03/26 04:29:06]

Nit: Slightly simpler, and parallel to the construction below:

    if (!CollectProfile(&profile, &elapsed_profile_time))
      return;
    profiles->push_back(profile);

[Mike Wittman, 2015/03/27 22:42:02]

Done.

       profiles->push_back(profile);
     else
       return;
 
     if (stop_event_.TimedWait(
             std::max(params_.burst_interval - elapsed_profile_time,
                      TimeDelta())))
       return;
   }
 }
@@ skipping 32 matching lines @@
     return;
 
   sampling_thread_.reset(
       new SamplingThread(
           native_sampler_.Pass(), params_,
           (custom_completed_callback_.is_null() ?
            Bind(&PendingProfiles::PutProfiles,
                 Unretained(PendingProfiles::GetInstance())) :
            custom_completed_callback_)));
   if (!PlatformThread::CreateNonJoinable(0, sampling_thread_.get()))
     LOG(ERROR) << "failed to create thread";

[Peter Kasting, 2015/03/26 04:29:06]

Normally we avoid LOG statements, since they bloat the binary and we don't
usually have a way to capture the data usefully from customer machines anyway.

I would omit this LOG unless you have a specific way to capture this data or you
need this for debugging right now and have a concrete plan to remove it in the
near future.

(Same comment applies in other files)

[Mike Wittman, 2015/03/27 22:42:02]

I agree LOG is not a good approach here. The desire behind these is to try to
understand unanticipated scenarios where the associated functions could fail.
Short of providing UMA metrics, which is too heavyweight at this point, I'm
thinking NOTREACHED or DCHECK may be the best option. Updated the code.

 }
 
 void StackSamplingProfiler::Stop() {
   if (sampling_thread_)
     sampling_thread_->Stop();
 }
 
 // static
 void StackSamplingProfiler::GetPendingProfiles(std::vector<Profile>* profiles) {
   PendingProfiles::GetInstance()->GetProfiles(profiles);
 }
 
 void StackSamplingProfiler::SetCustomCompletedCallback(
     Callback<void(const std::vector<Profile>&)> callback) {
   custom_completed_callback_ = callback;

[Peter Kasting, 2015/03/26 04:29:06]

This sort of simple setter should probably be inlined into the header and named
unix_hacker()-style; see the Accessors section of
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names .

[Mike Wittman, 2015/03/27 22:42:02]

Done. This function used to have more logic, which I removed at some point and
forgot to update the name.

 }
 
 bool operator==(const StackSamplingProfiler::Frame &a,
                 const StackSamplingProfiler::Frame &b) {
   return a.instruction_pointer == b.instruction_pointer &&
       a.module_index == b.module_index;
 }
 
 bool operator<(const StackSamplingProfiler::Frame &a,
                const StackSamplingProfiler::Frame &b) {
   return (a.module_index < b.module_index) ||
       (a.module_index == b.module_index &&
        a.instruction_pointer < b.instruction_pointer);
 }
 
 }  // namespace base
+