Introducing phased profiling framework

base/tracked_objects.h

Index: base/tracked_objects.h
diff --git a/base/tracked_objects.h b/base/tracked_objects.h
index 7840fecbd1459c10f551a83386b91d43f0e7eaeb..0e71fa05b15c79141dc60987a49ef2d3a6644733 100644
--- a/base/tracked_objects.h
+++ b/base/tracked_objects.h
@@ -146,10 +146,14 @@ struct TrackingInfo;
 // 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
@@ -341,6 +345,11 @@ struct BASE_EXPORT TaskSnapshot {
 // 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;
 
@@ -375,8 +384,10 @@ class BASE_EXPORT ThreadData {
   // 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.
@@ -402,16 +413,14 @@ class BASE_EXPORT ThreadData {
   // 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_; }
 
@@ -513,16 +522,20 @@ class BASE_EXPORT ThreadData {
   // 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
@@ -746,15 +759,28 @@ struct BASE_EXPORT ParentChildPairSnapshot {
 };
 
 //------------------------------------------------------------------------------
-// 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;
 };