Introducing phased profiling framework

base/tracked_objects.h

 // Copyright (c) 2012 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_TRACKED_OBJECTS_H_
 #define BASE_TRACKED_OBJECTS_H_
 
 #include <map>
 #include <set>
 #include <stack>
@@ skipping 128 matching lines @@
 // (3) the snapshotted data.
 //
 // For a given birth location, information about births is spread across data
 // structures that are asynchronously changing on various threads.  For
 // serialization and display purposes, we need to construct TaskSnapshot
 // instances for each combination of birth thread, death thread, and location,
 // along with the count of such lifetimes.  We gather such data into a
 // TaskSnapshot instances, so that such instances can be sorted and
 // aggregated (and remain frozen during our processing).
 //
-// The ProcessDataSnapshot struct is a serialized representation of the list
-// of ThreadData objects for a process.  It holds a set of TaskSnapshots
-// and tracks parent/child relationships for the executed tasks.  The statistics
-// in a snapshot are gathered asynhcronously relative to their ongoing updates.
+// Profiling consists of phases. The concrete phase in the sequence of phases is
+// identified by its 0-based nubmer.

[Ilya Sherman, 2015/03/10 00:48:10]

nit: "0-based" -> "0-index"; "number" -> "id number" or "sequence number" or
something like that.

[vadimt, 2015/03/13 23:18:01]

Done; the new terminology looks the most common. 

+//
+// The ProcessDataPhaseSnapshot struct is a serialized representation of the
+// list of ThreadData objects for a process for a concrete profiling phase. It
+// holds a set of TaskSnapshots and tracks parent/child relationships for the
+// executed tasks. The statistics in a snapshot are gathered asynhcronously
+// relative to their ongoing updates.
 // It is possible, though highly unlikely, that stats could be incorrectly
 // recorded by this process (all data is held in 32 bit ints, but we are not
 // atomically collecting all data, so we could have count that does not, for
 // example, match with the number of durations we accumulated).  The advantage
 // to having fast (non-atomic) updates of the data outweighs the minimal risk of
 // a singular corrupt statistic snapshot (only the snapshot could be corrupt,
 // not the underlying and ongoing statistic).  In contrast, pointer data that
 // is accessed during snapshotting is completely invariant, and hence is
 // perfectly acquired (i.e., no potential corruption, and no risk of a bad
 // memory reference).
@@ skipping 171 matching lines @@
   std::string death_thread_name;
 };
 
 //------------------------------------------------------------------------------
 // For each thread, we have a ThreadData that stores all tracking info generated
 // on this thread.  This prevents the need for locking as data accumulates.
 // We use ThreadLocalStorage to quickly identfy the current ThreadData context.
 // We also have a linked list of ThreadData instances, and that list is used to
 // harvest data from all existing instances.
 
+struct ProcessDataPhaseSnapshot;
+// Map from profiling phase number to the process-wide snapshotted
+// representation of the list of ThreadData objects that died during the given
+// phase.
+typedef std::map<int, ProcessDataPhaseSnapshot> PhasedProcessDataSnapshots;

[Ilya Sherman, 2015/03/10 00:48:10]

As elsewhere, I'd prefer that you not use a typedef.  However, if you do use a
typedef, please include "Map" in the name.  By convention, a plural name means a
list.

Though, I wonder: Why do we need a map?  How can phases appear out of order?

[vadimt, 2015/03/13 23:18:01]

Done.
A process may start after several phase changes, i.e. phases will not start from
0.
Having this in mind, it's easiest to work with maps.

[Ilya Sherman, 2015/03/14 00:20:34]

I guess I was thinking that the phase number -- or even just the event that
triggered a phase change -- could be stored within the ProcessDataPhaseSnapshot
struct, since they're always passed together anyway.

[vadimt, 2015/03/14 01:39:15]

The event type is declared outside of base, and we can't refer it from base. We
can type-unsafely pass events converted to ints between base/ and consuming
code, but it's unreadable and error-prone.

Besides, for the current phase there is no event that closes it, so we'd have to
have to introduce a special value just for this case.

Besides besides, a process can start after some phases already completed, and if
we just include the event in this data structure, we'd not be able to to build
the set of prior events for each phase.

Hence, the current approach, where the consumer speaks with base/ in terms of
phase numbers, and centrally keeps track of the sequence of all events.

Speaking of the idea of storing the phase number inside
PhasedProcessDataSnapshot, I'm wonder why we'd need to store it inside while
it's already available as a key in the map. It's not quite aesthetically
attractive, is it?

[Ilya Sherman, 2015/03/17 23:57:38]

Fair enough.
I'm suggesting not using a map, and instead just storing the phase number in the
snapshot.  I'm suggesting this because the two tend to be passed around
together, so they might as well be wrapped in the same struct.

[vadimt, 2015/03/19 00:20:44]

I'm using phase number to quickly find the entry in the map in
TrackingSynchronizer::SendData:
    tracked_objects::PhasedProcessDataSnapshotMap::const_iterator it =
        profiler_data.phased_process_data_snapshots.find(phase);

The could would look worse if I didn't have it as a key.

[Ilya Sherman, 2015/03/19 01:00:51]

I'm pretty unconvinced by this explanation -- it's pretty easy to call .find()
on a vector (which is also faster for small maps)... but I don't think it's
worth bikeshedding more on this, so I'll drop it.

 struct ProcessDataSnapshot;
 class BASE_EXPORT TaskStopwatch;
 
 class BASE_EXPORT ThreadData {
  public:
   // Current allowable states of the tracking system.  The states can vary
   // between ACTIVE and DEACTIVATED, but can never go back to UNINITIALIZED.
   enum Status {
     UNINITIALIZED,              // PRistine, link-time state before running.
     DORMANT_DURING_TESTS,       // Only used during testing.
@@ skipping 14 matching lines @@
   // set *before* any births on the threads have taken place.  It is generally
   // only used by the message loop, which has a well defined thread name.
   static void InitializeThreadContext(const std::string& suggested_name);
 
   // Using Thread Local Store, find the current instance for collecting data.
   // If an instance does not exist, construct one (and remember it for use on
   // this thread.
   // This may return NULL if the system is disabled for any reason.
   static ThreadData* Get();
 
-  // Fills |process_data| with all the recursive results in our process.
-  static void Snapshot(ProcessDataSnapshot* process_data);
+  // Fills |process_data_snapshot| with phased snapshots of all profiling
+  // phases, including the current one.
+  static void GetProcessDataSnapshot(
+      ProcessDataSnapshot* process_data_snapshot);
 
   // Finds (or creates) a place to count births from the given location in this
   // thread, and increment that tally.
   // TallyABirthIfActive will returns NULL if the birth cannot be tallied.
   static Births* TallyABirthIfActive(const Location& location);
 
   // Records the end of a timed run of an object.  The |completed_task| contains
   // a pointer to a Births, the time_posted, and a delayed_start_time if any.
   // The |start_of_run| indicates when we started to perform the run of the
   // task.  The delayed_start_time is non-null for tasks that were posted as
   // delayed tasks, and it indicates when the task should have run (i.e., when
   // it should have posted out of the timer queue, and into the work queue.
   // The |end_of_run| was just obtained by a call to Now() (just after the task
   // finished). It is provided as an argument to help with testing.
   static void TallyRunOnNamedThreadIfTracking(
       const base::TrackingInfo& completed_task,
       const TaskStopwatch& stopwatch);
 
   // Record the end of a timed run of an object.  The |birth| is the record for
   // the instance, the |time_posted| records that instant, which is presumed to
   // be when the task was posted into a queue to run on a worker thread.
   // The |start_of_run| is when the worker thread started to perform the run of
   // the task.
   // The |end_of_run| was just obtained by a call to Now() (just after the task
   // finished).
-  static void TallyRunOnWorkerThreadIfTracking(
-      const Births* birth,
-      const TrackedTime& time_posted,
-      const TaskStopwatch& stopwatch);
+  static void TallyRunOnWorkerThreadIfTracking(const Births* birth,
+                                               const TrackedTime& time_posted,
+                                               const TaskStopwatch& stopwatch);
 
   // Record the end of execution in region, generally corresponding to a scope
   // being exited.
-  static void TallyRunInAScopedRegionIfTracking(
-      const Births* birth,
-      const TaskStopwatch& stopwatch);
+  static void TallyRunInAScopedRegionIfTracking(const Births* birth,
+                                                const TaskStopwatch& stopwatch);

[Ilya Sherman, 2015/03/10 00:48:10]

nit: Are these two blocks just unrelated formatting changes?  Can they be split
off to a separate CL?

[vadimt, 2015/03/13 23:18:01]

I'm using git cl format, which wants to format this...

I believe it's OK to have formatting changes on base/, since there are
practically no substantial changes there to be confused with.

 
   const std::string& thread_name() const { return thread_name_; }
 
   // Initializes all statics if needed (this initialization call should be made
   // while we are single threaded). Returns false if unable to initialize.
   static bool Initialize();
 
   // Sets internal status_.
   // If |status| is false, then status_ is set to DEACTIVATED.
   // If |status| is true, then status_ is set to, PROFILING_ACTIVE, or
@@ skipping 81 matching lines @@
   Births* TallyABirth(const Location& location);
 
   // Find a place to record a death on this thread.
   void TallyADeath(const Births& birth,
                    int32 queue_duration,
                    const TaskStopwatch& stopwatch);
 
   // Snapshot (under a lock) the profiled data for the tasks in each ThreadData
   // instance.  Also updates the |birth_counts| tally for each task to keep
   // track of the number of living instances of the task.
-  static void SnapshotAllExecutedTasks(ProcessDataSnapshot* process_data,
-                                       BirthCountMap* birth_counts);
+  static void SnapshotAllExecutedTasks(
+      ProcessDataPhaseSnapshot* process_data_phase,
+      BirthCountMap* birth_counts);
+
+  // Fills |process_data_phase| with all the recursive results in our process.
+  static void Snapshot(ProcessDataPhaseSnapshot* process_data_phase);
 
   // Snapshots (under a lock) the profiled data for the tasks for this thread
   // and writes all of the executed tasks' data -- i.e. the data for the tasks
-  // with with entries in the death_map_ -- into |process_data|.  Also updates
-  // the |birth_counts| tally for each task to keep track of the number of
-  // living instances of the task -- that is, each task maps to the number of
+  // with with entries in the death_map_ -- into |process_data_phase|.  Also
+  // updates the |birth_counts| tally for each task to keep track of the number
+  // of living instances of the task -- that is, each task maps to the number of
   // births for the task that have not yet been balanced by a death.
-  void SnapshotExecutedTasks(ProcessDataSnapshot* process_data,
+  void SnapshotExecutedTasks(ProcessDataPhaseSnapshot* process_data_phase,
                              BirthCountMap* birth_counts);
 
   // Using our lock, make a copy of the specified maps.  This call may be made
   // on  non-local threads, which necessitate the use of the lock to prevent
   // the map(s) from being reallocated while they are copied.
   void SnapshotMaps(BirthMap* birth_map,
                     DeathMap* death_map,
                     ParentChildSet* parent_child_set);
 
   // This method is called by the TLS system when a thread terminates.
@@ skipping 203 matching lines @@
   ParentChildPairSnapshot();
   explicit ParentChildPairSnapshot(
       const ThreadData::ParentChildPair& parent_child);
   ~ParentChildPairSnapshot();
 
   BirthOnThreadSnapshot parent;
   BirthOnThreadSnapshot child;
 };
 
 //------------------------------------------------------------------------------
-// A snapshotted representation of the list of ThreadData objects for a process.
+// A snapshotted representation of the list of ThreadData objects for a process,
+// for a single profiling phase.
+
+struct BASE_EXPORT ProcessDataPhaseSnapshot {
+ public:
+  ProcessDataPhaseSnapshot();
+  ~ProcessDataPhaseSnapshot();
+
+  std::vector<TaskSnapshot> tasks;
+  std::vector<ParentChildPairSnapshot> descendants;
+};
+
+//------------------------------------------------------------------------------
+// A snapshotted representation of the list of ThreadData objects for a process,
+// for all profiling phases, including the current one.
 
 struct BASE_EXPORT ProcessDataSnapshot {
  public:
   ProcessDataSnapshot();
   ~ProcessDataSnapshot();
 
-  std::vector<TaskSnapshot> tasks;
-  std::vector<ParentChildPairSnapshot> descendants;
+  PhasedProcessDataSnapshots phased_process_data_snapshots;
   int process_id;
 };
 
 }  // namespace tracked_objects
 
 #endif  // BASE_TRACKED_OBJECTS_H_