StackSamplingProfiler clean up

base/profiler/stack_sampling_profiler.h

 // 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.
 
 #ifndef BASE_PROFILER_STACK_SAMPLING_PROFILER_H_
 #define BASE_PROFILER_STACK_SAMPLING_PROFILER_H_
 
 #include <string>
 #include <vector>
 
 #include "base/base_export.h"
 #include "base/callback.h"
 #include "base/files/file_path.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/strings/string16.h"
 #include "base/threading/platform_thread.h"
 #include "base/time/time.h"
 
 namespace base {
 
 // StackSamplingProfiler periodically stops a thread to sample its stack, for
 // the purpose of collecting information about which code paths are
 // executing. This information is used in aggregate by UMA to identify hot
 // and/or janky code paths.
 //
 // Sample StackStackSamplingProfiler usage:

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

StackStack?

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

Done.

 //
 //   // Create and customize params as desired.
 //   base::StackStackSamplingProfiler::SamplingParams params;
 //   // Any thread's ID may be passed as the target.
 //   base::StackSamplingProfiler profiler(base::PlatformThread::CurrentId()),
 //       params);
 //
 //   // To process the profiles within Chrome rather than via UMA, set a custom
 //   // completed callback:
 //   base::Callback<void(const std::vector<Profile>&)>

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

Nit: I think Profile should be base::StackSamplingProfiler::Profile here.

If you follow my type alis suggestion below, this comment can make use of the
alias types.

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

Used an alias.

 //       thread_safe_callback = ...;
 //   profiler.SetCustomCompletedCallback(thread_safe_callback);
 //
 //   profiler.Start();
 //   // ... work being done on the target thread here ...
 //   profiler.Stop();  // optional, stops collection before complete per params
 //
 // When all profiles are complete or the profiler is stopped, if the custom

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

What does it mean for a Profile to be "complete"?  Perhaps you need to expand
your comment to discuss the SamplingParams and Profile classes in more detail?

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

Updated and expanded the comment, including specifying what it means for a
profile to be complete.

 // completed callback was set it will be called from the profiler thread with
 // the completed profiles. If no callback was set, the profiles are stored
 // internally and retrieved for UMA through

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

Nit: Odd line wrapping

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

Done.

 // GetPendingProfiles(). GetPendingProfiles() should never be called by other
 // code; to retrieve profiles for in-process processing, set a completed
 // callback.
 class BASE_EXPORT StackSamplingProfiler {
  public:
   // Module represents the module (DLL or exe) corresponding to a stack frame.
   struct Module {
     Module();
     ~Module();
 
@@ skipping 11 matching lines @@
     FilePath filename;
   };
 
   // Frame represents an individual sampled stack frame with module information.
   struct Frame {
     Frame();
     ~Frame();
 
     // The sampled instruction pointer within the function.
     const void* instruction_pointer;
     // Index of the module in the array of modules. We don't represent module

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

The array of modules where?  Presumably you mean Profile::modules; be explicit. 
(Also, technically, that's not an array.)

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

Done.

     // state directly here to save space.
     int module_index;

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

If this is an index within an object in memory, its type should probably be
size_t.  If you need to represent an "invalid index", I would then define that
as a constant, much like std::string::npos:

  static const size_t kNoModule = static_cast<size_t>(-1);

Using such a named constant is clearer than using "-1" as a magic number, and
using size_t like this matches the STL.

(This will eliminate the need to cast to int in unittest code and
NativeStackSamplerWin::CopyToSample().  It will also cause your operator<()
below to sort samples from invalid modules to the end of the list, which might
be a nice behavior.)

There are a lot of other variables across various files (especially in
stack_sampling_profiler_win.cc) which could arguably be either size_t or int;
you generally use int, while I would probably use size_t for any "count of
things in memory or loop index over those things".  The style guide simply says
to use size_t "when appropriate", so this is somewhat up to implementer/reviewer
discretion.  I won't mandate any particular thing, but if you find yourself e.g.
doing "foo[i]", consider making |i| a size_t (which may have a chain-reaction
effect on a variety of other variables' types).

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

I'm accustomed to using size_t in this case, and didn't use it here based on the
apparent Google predilection for int. :)

I've changed the code over to size_t. It removes the casting here, but
unfortunately just moves it elsewhere, as the main consumer of these types
transcodes them to protobufs (which have an int size type API).

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

D'oh!

Oh well, that's still probably better, as the casting happens as far as possible
from the functional code, so in theory someone can update the protobuf type or
whatever and see nearby whether the type is safe to store the underlying
(pre-cast) value.

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

Acknowledged.

   };
 
   // Sample represents a set of stack frames.
   using Sample = std::vector<Frame>;
 
   // Profile represents a set of samples.
   struct BASE_EXPORT Profile {
     Profile();
     ~Profile();
 
     std::vector<Module> modules;
     std::vector<Sample> samples;
     // Duration of this profile.
     TimeDelta profile_duration;
     // Time between samples.
     TimeDelta sampling_period;
     // True if sample ordering is important and should be preserved if and when
     // this profile is compressed and processed.
     bool preserve_sample_ordering;
   };
 
   // NativeStackSampler abstracts the native implementation required to record a
   // stack sample for a given thread.

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

It seems like this class should either be private, or should be merely
forward-declared and then get its own header file.  The idea either way is to
avoid exposing this here, publicly, when external consumers don't need to use
it.

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

I agree with the sentiment, but I'm not super happy with the result of any of
the three options.

As you've suggested, the current option exposes an implementation detail in the
public interface.

If we make this private though, then we need to forward declare the native
subclasses in StackSamplingProfiler and make them part of the class. This
sort-of couples the implementations with the interface, which feels odd. Also,
we either wind up forward declaring classes that aren't meaningful or defined
(NativeStackSamplerWin on Linux), or share the same native subclass name across
all platforms (NativeStackSamplerImpl?) which seems contrary to common Chrome
platform-specific practice.

A forward declaration with the interface in it's own header is good in that it
moves irrelevant details out of this header, but feels like it elevates the
interface to something one would naively expect to be generally useful just
looking at the files in the directory.

I'm unsure what's considered the best option for Chrome practice, so please
suggest one. :)

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

I definitely agree that the consequences of a private declaration are bad, so
let's not go that way.

Personally, I don't mind the declaration in an external header.  After all, the
style guide says you should have file- and/or class-level comments in such
headers describing what the class is and how it's used; it's not hard to state
clearly in those comments that the class in question is only useful for (list of
narrow set of purposes or callers).

Most people are unlikely to be trying to use classes by browsing an unfamiliar
directory, picking an interesting-sounding header, and attempting to instantiate
the class, so anyone who comes upon your new file probably either understands
what the class is for, or will be inclined to read your descriptive comment.

As to which option is more frequent Chromium practice, I don't have a good feel
for that, but I do know we have a number of places in our codebase where classes
of narrow utility have been given their own files, so the idea of having such a
class isn't unknown.

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

OK, I've moved the class to its own file. I was able to remove the
native_sampler_ member variable also, since this is only used within one
function.

   class NativeStackSampler {
    public:
     virtual ~NativeStackSampler();
 
     // Create a stack sampler that records samples for |thread_handle|. Returns

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

All function comments should be declarative ("Creates") rather than imperative
("Create"); see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...
.  (Several places across multiple files)

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

I think I've updated all of these.

     // null if this platform does not support stack sampling.
     static scoped_ptr<NativeStackSampler> Create(PlatformThreadId thread_id);
 
     // Notify the sampler that we're starting to record a new profile. This
     // function is called on the SamplingThread.

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

You use "SamplingThread" in reference (presumably) to the class type
forward-declared below, but that declaration has no comments.

Is it sufficient to simply say "called on the thread being sampled" here and in
the next two comments?  Otherwise, if some specific aspect of the SamplingThread
type is important for the caller to understand, refer the reader to relevant
comments that explain it.

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

It's sufficient to refer to it as "the thread used for sampling" (different from
the sampled thread).

     virtual void ProfileRecordingStarting(Profile* profile) = 0;
 
     // Record a stack sample. This function is called on the SamplingThread.
     virtual void RecordStackSample(Sample* sample) = 0;
 
     // Notify the sampler that we've stopped recording the current profile. This
     // function is called on the SamplingThread.
     virtual void ProfileRecordingStopped() = 0;
 
    protected:
@@ skipping 11 matching lines @@
     TimeDelta initial_delay;
     // Number of sampling bursts to perform. Defaults to 1.
     int bursts;
     // Interval between sampling bursts. This is the desired duration from the
     // start of one burst to the start of the next burst. Defaults to 10s.
     TimeDelta burst_interval;
     // Number of samples to record per burst. Defaults to 300.
     int samples_per_burst;
     // Interval between samples during a sampling burst. This is the desired
     // duration from the start of one burst to the start of the next
     // burst. Defaults to 100ms.

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

I think you mean "sample" instead of "burst" in a couple places here.

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

Done.

     TimeDelta sampling_interval;
     // True if sample ordering is important and should be preserved if and when
     // this profile is compressed and processed. Defaults to false.
     bool preserve_sample_ordering;
   };
 
   StackSamplingProfiler(PlatformThreadId thread_id,
                         const SamplingParams& params);
   ~StackSamplingProfiler();
 
   // Initializes the profiler and starts sampling.
   void Start();
   // Stops the profiler and any ongoing sampling. Calling this function is

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

Nit: Blank line above this

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

Done.

   // optional; if not invoked profiling will terminate when all the profiling
   // bursts specified in the SamplingParams are completed.
   void Stop();
 
   // Gets the pending profiles into *|profiles| and clears the internal
   // storage. This function is thread safe.

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

Nit: Maybe "Moves all pending Profiles from internal storage to |profiles|"? 
This seems a little shorter and clearer (and avoids putting a '*' before
|profiles|, which is uncommon).

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

Done.

   //
   // ***This is intended for use only by UMA.*** Callers who want to process the
   // collected profiles should use SetCustomCompletedCallback.
   static void GetPendingProfiles(std::vector<Profile>* profiles);

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

Should we consider making this private and making the UMA code a friend, then?

If not, perhaps this should be named GetPendingProfilesForUMA() or the like?

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

I have a follow-on change to this code in https://crrev.com/1029653002 that
makes this interface more generic by allowing a static completed callback to be
specified for the UMA case. I believe that change addresses this issue, and does
so in a cleaner way. Is that an acceptable solution?

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

Yeah, that seems fine.  Dunno if you wanted to copy that into this CL or not.

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

I'd prefer to keep them separate to avoid mixing more functional changes into
this CL than is necessary. I'll merge the changes after the first of the two CLs
land.

 
   // By default, collected profiles are stored internally and can be retrieved
   // by GetPendingProfiles. If a callback is provided via this function,
   // however, it will be called with the collected profiles instead. Note that
   // this call to the callback occurs *on the profiler thread*.

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

Nit: this -> the

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

Done.

   void SetCustomCompletedCallback(
       Callback<void(const std::vector<Profile>&)> callback);

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

Consider a public type alias for the function type (and maybe callback type)
here to decrease verbosity (and reduce the number of places needing a change if
we ever modify the function signature):

  using CompletedFunc = void(const std::vector<Profile>&);
  using CompletedCallback = Callback<CompletedFunc>;  // optional
  ...
  void SetCustomCompletedCallback(CompletedCallback callback);
  ...
  CompletedCallback custom_completed_callback_;

  // In a file that forward-declares the function passed as the callback:
  base::StackSamplingProfiler::CompletedFunc MyFunc;
  // ...Now the full signature is only needed at the definition

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

I think CompletedCallback is a good idea (and updated to use it), but I don't
really see the value of CompletedFunc. A proper function that fits this
signature would only be able to modify global state. The expected use cases are
functions that take at least one other parameter in addition to the Profile
vector, with the additional parameters bound to create a callback with the
expected signature.

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

That makes sense.

 
  private:
   class SamplingThread;

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

Comment what this class is.

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

Done.

   struct SamplingThreadDeleter {

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

Why is this custom deleter necessary?  Your implementation of it seems to simply
be a standard delete.

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

It's not required, but allows the definition of SamplingThread to be kept out of
the header. Without the custom deleter defined in the .cc file, scoped_ptr
requires a complete type.

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

Urgh.  I think the cost is higher than the benefit here.  I would either go
ahead and declare SamplingThread here, or else move it out to an external header
similarly to the case we discussed above.  Especially since SamplingThread is
private, putting the full declaration here doesn't actually expose it to any
consumers, whereas defining a custom deleter is very unusual and immediately
makes me think that the class is special and has some sort of tricky usage.

(This seems a bit like the style guide rule to avoid going out of your way to
replace #includes with forward-decls.)

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

Moved the class definition into the class.

     void operator() (SamplingThread* thread) const;
   };
 
   // The thread whose stack will be sampled.
   PlatformThreadId thread_id_;
 
   const SamplingParams params_;
 
   scoped_ptr<SamplingThread, SamplingThreadDeleter> sampling_thread_;
   scoped_ptr<NativeStackSampler> native_sampler_;
 
   Callback<void(const std::vector<Profile>&)> custom_completed_callback_;
 
   DISALLOW_COPY_AND_ASSIGN(StackSamplingProfiler);
 };
 
 // Defined to allow equality check of Samples.

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

Where are these actually used?  I looked briefly for at least the == one and
didn't see a use, but maybe I missed it.

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

They're used by the metrics provider code to support testing Samples for
equality and ordering them:
https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics...
and
https://code.google.com/p/chromium/codesearch#chromium/src/components/metrics...

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

OK.  You may want to mention that in the comments here, e.g.:

// The metrics provider code wants to put Samples in a map and compare them,
// which requires us to define a few operators.
BASE_EXPORT bool operator==(...);
BASE_EXPORT bool operator<(...);

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

Done.

 BASE_EXPORT bool operator==(const StackSamplingProfiler::Frame& a,
                             const StackSamplingProfiler::Frame& b);
 // Defined to allow ordering of Samples.
 BASE_EXPORT bool operator<(const StackSamplingProfiler::Frame& a,
                            const StackSamplingProfiler::Frame& b);
 
 }  // namespace base
 
 #endif  // BASE_PROFILER_STACK_SAMPLING_PROFILER_H_
+