Delivering the FIRST_NONEMPTY_PAINT phase changing event to base/

base/tracked_objects.h

Index: base/tracked_objects.h
diff --git a/base/tracked_objects.h b/base/tracked_objects.h
index 00bdb2dfc5392631cfe0206fc08107406f073fe3..dc39ed7e60dc025082faf9dac232cc06700075cc 100644
--- a/base/tracked_objects.h
+++ b/base/tracked_objects.h
@@ -245,6 +245,9 @@ class BASE_EXPORT Births: public BirthOnThread {
   // When we have a birth we update the count for this birthplace.
   void RecordBirth();
 
+  // Subtracts a value from the birth count.
+  void SubtractBirths(int count);
+
  private:
   // The number of births on this thread for our location_.
   int birth_count_;
@@ -282,7 +285,7 @@ class BASE_EXPORT DeathData {
   int32 queue_duration_max() const;
   int32 queue_duration_sample() const;
 
-  // Reset all tallies to zero. This is used as a hack on realtime data.
+  // Reset all tallies to zero.
   void Clear();
 
  private:
@@ -369,7 +372,7 @@ class BASE_EXPORT ThreadData {
   };
 
   typedef std::map<Location, Births*> BirthMap;
-  typedef std::map<const Births*, DeathData> DeathMap;
+  typedef std::map<Births*, DeathData> DeathMap;
   typedef std::pair<const Births*, const Births*> ParentChildPair;
   typedef std::set<ParentChildPair> ParentChildSet;
   typedef std::stack<const Births*> ParentStack;
@@ -387,8 +390,21 @@ class BASE_EXPORT ThreadData {
   static ThreadData* Get();
 
   // Fills |process_data_snapshot| with phased snapshots of all profiling
-  // phases, including the current one.
-  static void Snapshot(ProcessDataSnapshot* process_data_snapshot);
+  // phases, including the current one, identified by |current_profiling_phase|.
+  // |current_profiling_phase| is necessary because a child process can start
+  // after several phase-changing events, so it needs to receive the current
+  // phase number from the browser process to fill the correct entry for the
+  // current phase in the |process_data_snapshot| map.
+  static void Snapshot(int current_profiling_phase,
+                       ProcessDataSnapshot* process_data_snapshot);
+
+  // Called when the current profiling phase, identified by |profiling_phase|,
+  // ends.
+  // |profiling_phase| is necessary because a child process can start after
+  // several phase-changing events, so it needs to receive the phase number from
+  // the browser process to fill the correct entry in the
+  // completed_phases_snapshots_ map.
+  static void OnProfilingPhaseCompletion(int profiling_phase);
 
   // Finds (or creates) a place to count births from the given location in this
   // thread, and increment that tally.
@@ -414,14 +430,20 @@ 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);
+  // We don't modify anything in |birth| in this method. We store the pointer as

[Alexei Svitkine (slow), 2015/03/30 22:05:21]

Nit: Rephrase without "we". Same below.

[vadimt, 2015/04/01 00:45:40]

N/A anymore.

+  // a key in death_map_, and later, on a phase change event, will call
+  // SubtractBirths() on it.

[Alexei Svitkine (slow), 2015/03/30 22:05:21]

I think this comment needs to discuss the ownership guarantees of |birth| the
caller is expected to make for this to be safe.

I guess the assumption is that the birth should be something returned by
ThreadData::TallyABirthIfActive() and thus owned by ThreadData? If so, this
should be mentioned - as this API allows calling with any birth. In fact, I also
suggest adding a DCHECK_EQ(birth,
ThreadData::TallyABirthIfActive(birth->location()); to validate the contract.

Also, shouldn't the param be called |births| rather than birth?

Same for the other function below.

[vadimt, 2015/04/01 00:45:40]

Renamed |birth| to |births|.

On "the ownership guarantees of |birth| the caller is expected to make for this
to be safe.":

Thanks for raising this!

Long story short, I realized I had a race condition. I undone removing consts
from Births. Resetting death counts become more complicated, and now requires 2
passes: 1 to set death counts to 0 under their thread' locks, and the other - to
subtract death counts from birth counts under correct locks.

Note that a thread, under its lock, can modify birth counts from its birth map
and death counts from its death map. It cannot modify birth counts from its
death map (and that's why we have const there).

Also note that a Births instance, which is a value in the birth map of one
thread, can be a (const) key in the death map of another thread (which means
that the task was posted by one thread to the other).

We'd have less complexities if we could eliminate birth counts. They are
necessary for little-known and unused feature called "Still_Alive tasks". But I
couldn't prove to jar@ that we should remove it, so, we are keeping birth counts
in this CL.

+  static void TallyRunOnWorkerThreadIfTracking(const TrackedTime& time_posted,
+                                               const TaskStopwatch& stopwatch,
+                                               Births* birth);
 
   // Record the end of execution in region, generally corresponding to a scope
   // being exited.
-  static void TallyRunInAScopedRegionIfTracking(const Births* birth,
-                                                const TaskStopwatch& stopwatch);
+  // We don't modify anything in |birth| in this method. We store the pointer as
+  // a key in death_map_, and later, on a phase change event, will call
+  // SubtractBirths() on it.
+  static void TallyRunInAScopedRegionIfTracking(const TaskStopwatch& stopwatch,
+                                                Births* birth);
 
   const std::string& thread_name() const { return thread_name_; }
 
@@ -516,19 +538,26 @@ class BASE_EXPORT ThreadData {
   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);
+  // We don't modify anything in |birth| in this method. We store the pointer as
+  // a key in death_map_, and later, on a phase change event, will call
+  // SubtractBirths() on it.
+  void TallyADeath(int32 queue_duration,
+                   const TaskStopwatch& stopwatch,
+                   Births* birth);
 
   // 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.
+  // track of the number of living instances of the task. If |reset| is true,
+  // remove all recorded deaths.
   static void SnapshotAllExecutedTasks(
+      bool reset,
       ProcessDataPhaseSnapshot* process_data_phase,
       BirthCountMap* birth_counts);
 
   // Fills |process_data_phase| with all the recursive results in our process.
+  // If |reset| is true, remove all recorded deaths.
   static void SnapshotCurrentPhase(
+      bool reset,
       ProcessDataPhaseSnapshot* process_data_phase);
 
   // Snapshots (under a lock) the profiled data for the tasks for this thread
@@ -536,14 +565,18 @@ class BASE_EXPORT ThreadData {
   // 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(ProcessDataPhaseSnapshot* process_data_phase,
+  // births for the task that have not yet been balanced by a death. If |reset|
+  // is true, remove all recorded deaths.
+  void SnapshotExecutedTasks(bool reset,
+                             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,
+  // the map(s) from being reallocated while they are copied. If |reset| is
+  // true, remove all recorded deaths.
+  void SnapshotMaps(bool reset,
+                    BirthMap* birth_map,
                     DeathMap* death_map,
                     ParentChildSet* parent_child_set);
 
@@ -611,6 +644,10 @@ class BASE_EXPORT ThreadData {
   // We set status_ to SHUTDOWN when we shut down the tracking service.
   static Status status_;
 
+  // Process data snapshots for completed profiling phases.
+  static base::LazyInstance<PhasedProcessDataSnapshotMap>
+      completed_phases_snapshots_;
+
   // Link to next instance (null terminated list). Used to globally track all
   // registered instances (corresponds to all registered threads where we keep
   // data).