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 964326511b23355f0bca71b1671919d8cb3e8548..4cffbfff4b4f608b1e5e29939241613bb23cce20 100644
--- a/base/tracked_objects.h
+++ b/base/tracked_objects.h
@@ -22,6 +22,7 @@
 #include "base/profiler/alternate_timer.h"
 #include "base/profiler/tracked_time.h"
 #include "base/synchronization/lock.h"
+#include "base/threading/thread_checker.h"
 #include "base/threading/thread_local_storage.h"
 
 namespace base {
@@ -254,19 +255,78 @@ class BASE_EXPORT Births: public BirthOnThread {
 };
 
 //------------------------------------------------------------------------------
-// Basic info summarizing multiple destructions of a tracked object with a
-// single birthplace (fixed Location).  Used both on specific threads, and also
-// in snapshots when integrating assembled data.
+// A "snapshotted" representation of the DeathData class.
+
+struct BASE_EXPORT DeathDataSnapshot {
+  DeathDataSnapshot();
+
+  // Constructs the snapshot from individual values.
+  // The alternative would be taking a DeathData parameter, but this would
+  // create a loop since DeathData indirectly refers DeathDataSnapshot. Passing
+  // a wrapper structure as a param or using an empty constructor for
+  // snapshotting DeathData would be less efficient.
+  DeathDataSnapshot(int count,
+                    int32 run_duration_sum,
+                    int32 run_duration_max,
+                    int32 run_duration_sample,
+                    int32 queue_duration_sum,
+                    int32 queue_duration_max,
+                    int32 queue_duration_sample);
+  ~DeathDataSnapshot();
+
+  // Calculates and returns the delta between this snapshot and an earlier
+  // snapshot of the same task |older|.
+  DeathDataSnapshot Delta(const DeathDataSnapshot& older) const;
+
+  int count;
+  int32 run_duration_sum;
+  int32 run_duration_max;
+  int32 run_duration_sample;
+  int32 queue_duration_sum;
+  int32 queue_duration_max;
+  int32 queue_duration_sample;
+};
+
+//------------------------------------------------------------------------------
+// A "snapshotted" representation of the DeathData for a particular profiling
+// phase. Used as an element of the list of phase snapshots owned by DeathData.
+
+struct DeathDataPhaseSnapshot {
+  DeathDataPhaseSnapshot(int profiling_phase,
+                         int count,
+                         int32 run_duration_sum,
+                         int32 run_duration_max,
+                         int32 run_duration_sample,
+                         int32 queue_duration_sum,
+                         int32 queue_duration_max,
+                         int32 queue_duration_sample,
+                         DeathDataPhaseSnapshot* prev);
+
+  // Profiling phase at which completion this snapshot was taken.
+  int profiling_phase;
+
+  // Death data snapshot.
+  DeathDataSnapshot death_data;
+
+  // Pointer to a snapshot from the previous phase.
+  DeathDataPhaseSnapshot* prev;
+};
+
+//------------------------------------------------------------------------------
+// Information about deaths of a task on a given thread, called "death thread".
+// Access to members of this class is never protected by the lock. The fields

[Ilya Sherman, 2015/04/17 02:14:27]

nit: s/the lock/a lock

[vadimt, 2015/04/17 16:15:51]

Done.

+// are accessed in such a way that corruptions resulting from race conditions
+// are not significant, and don't accumulate as a result of multiple accesses.
+// All snapshots and phase change notifications must be called from the same
+// thread. It doesn't matter what thread it is, but it's important the same

[Ilya Sherman, 2015/04/17 02:14:26]

I find the sentence "All snapshot and phase change notifications ..." confusing.
 It looks like this refers to precisely two methods, OnProfilingPhaseCompleted()
and last_phase_snapshot().  Is that accurate?  If so, it might be clearer to
simply refer to the methods by name.

[vadimt, 2015/04/17 16:15:51]

Done.

+// thread is used as a snapshot thread during the whole process lifetime.
+// All fields except sample_probability_count_ can be snapshotted.
 
 class BASE_EXPORT DeathData {
  public:
-  // Default initializer.
   DeathData();
-
-  // When deaths have not yet taken place, and we gather data from all the
-  // threads, we create DeathData stats that tally the number of births without
-  // a corresponding death.
-  explicit DeathData(int count);
+  DeathData(const DeathData& other);
+  ~DeathData();
 
   // Update stats for a task destruction (death) that had a Run() time of
   // |duration|, and has had a queueing delay of |queue_duration|.
@@ -274,7 +334,8 @@ class BASE_EXPORT DeathData {
                    const int32 run_duration,
                    const uint32 random_number);
 
-  // Metrics accessors, used only for serialization and in tests.
+  // Metrics and past snapshots accessors, used only for serialization and in
+  // tests.
   int count() const;
   int32 run_duration_sum() const;
   int32 run_duration_max() const;
@@ -282,43 +343,48 @@ class BASE_EXPORT DeathData {
   int32 queue_duration_sum() const;
   int32 queue_duration_max() const;
   int32 queue_duration_sample() const;
+  DeathDataPhaseSnapshot* last_phase_snapshot() const;

[Ilya Sherman, 2015/04/17 02:14:27]

nit: Const methods should never return non-const pointers.  Please either return
a const pointer or mark this method as non-const.

[vadimt, 2015/04/17 16:15:51]

Done.

 
-  // Reset all tallies to zero. This is used as a hack on realtime data.
-  void Clear();
+  // Called when the current profiling phase, identified by |profiling_phase|,
+  // ends.
+  // Must be called only on the snapshot thread.
+  void OnProfilingPhaseCompleted(int profiling_phase);
 
  private:
   // Members are ordered from most regularly read and updated, to least
   // frequently used.  This might help a bit with cache lines.
   // Number of runs seen (divisor for calculating averages).
+  // Can be incremented only on the death thread.
   int count_;
-  // Basic tallies, used to compute averages.
+
+  // Count used in determining probability of selecting exec/queue times from a
+  // recorded death as samples.
+  // Gets incremented only on the death thread, but can be set to 0 by
+  // OnProfilingPhaseCompleted() on the snapshot thread.
+  int sample_probability_count_;
+
+  // Basic tallies, used to compute averages. Can be incremented only on the
+  // death thread.
   int32 run_duration_sum_;
   int32 queue_duration_sum_;
   // Max values, used by local visualization routines.  These are often read,
-  // but rarely updated.
+  // but rarely updated. The max values get assigned only on the death thread,
+  // but these fields can be set to 0 by OnProfilingPhaseCompleted() on the
+  // snapshot thread.
   int32 run_duration_max_;
   int32 queue_duration_max_;
   // Samples, used by crowd sourcing gatherers.  These are almost never read,
-  // and rarely updated.
+  // and rarely updated. They can be modified only on the death thread.

[Ilya Sherman, 2015/04/17 02:14:27]

nit: The existing comments in this file seem to use two periods between
sentences.  Please maintain consistency with that precedent.  (Applies
throughout.)

[vadimt, 2015/04/17 16:15:51]

Done.

   int32 run_duration_sample_;
   int32 queue_duration_sample_;
-};
 
-//------------------------------------------------------------------------------
-// A "snapshotted" representation of the DeathData class.
+  // Snapshot of this death data made at the last profiling phase completion, if
+  // any. DeathData owns the whole list starting with this pointer.
+  // Can be accessed only on the snapshot thread.
+  DeathDataPhaseSnapshot* last_phase_snapshot_;
 
-struct BASE_EXPORT DeathDataSnapshot {
-  DeathDataSnapshot();
-  explicit DeathDataSnapshot(const DeathData& death_data);
-  ~DeathDataSnapshot();
-
-  int count;
-  int32 run_duration_sum;
-  int32 run_duration_max;
-  int32 run_duration_sample;
-  int32 queue_duration_sum;
-  int32 queue_duration_max;
-  int32 queue_duration_sample;
+  // Private assignment operator to disallow assignments.
+  void operator=(DeathData const&);

[Ilya Sherman, 2015/04/17 02:14:26]

nit: Please use DISALLOW_ASSIGN.

 };
 
 //------------------------------------------------------------------------------
@@ -330,12 +396,14 @@ struct BASE_EXPORT DeathDataSnapshot {
 
 struct BASE_EXPORT TaskSnapshot {
   TaskSnapshot();
-  TaskSnapshot(const BirthOnThread& birth,
-               const DeathData& death_data,
+  TaskSnapshot(const BirthOnThreadSnapshot& birth,
+               const DeathDataSnapshot& death_data,
                const std::string& death_thread_name);
   ~TaskSnapshot();
 
   BirthOnThreadSnapshot birth;
+  // Delta between death data for a thread for a certain profiling phase and the
+  // snapshot for the pervious phase, if any. Otherwise, just a snapshot.
   DeathDataSnapshot death_data;
   std::string death_thread_name;
 };
@@ -388,8 +456,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 OnProfilingPhaseCompleted(int profiling_phase);
 
   // Finds (or creates) a place to count births from the given location in this
   // thread, and increment that tally.
@@ -415,13 +496,13 @@ 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,
+  static void TallyRunOnWorkerThreadIfTracking(const Births* births,
                                                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,
+  static void TallyRunInAScopedRegionIfTracking(const Births* births,
                                                 const TaskStopwatch& stopwatch);
 
   const std::string& thread_name() const { return thread_name_; }
@@ -493,6 +574,9 @@ class BASE_EXPORT ThreadData {
 
   typedef std::map<const BirthOnThread*, int> BirthCountMap;
 
+  typedef std::vector<std::pair<const Births*, DeathDataPhaseSnapshot>>
+      DeathsSnapshot;
+
   // Worker thread construction creates a name since there is none.
   explicit ThreadData(int thread_number);
 
@@ -517,37 +601,33 @@ class BASE_EXPORT ThreadData {
   Births* TallyABirth(const Location& location);
 
   // Find a place to record a death on this thread.
-  void TallyADeath(const Births& birth,
+  void TallyADeath(const Births& births,
                    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(
-      ProcessDataPhaseSnapshot* process_data_phase,
-      BirthCountMap* birth_counts);
-
-  // Fills |process_data_phase| with all the recursive results in our process.
-  static void SnapshotCurrentPhase(
-      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_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,
+  // and writes all of the executed tasks' data -- i.e. the data for all
+  // profiling phases (including the current one: |current_profiling_phase|) for
+  // the tasks with with entries in the death_map_ -- into |phased_snapshots|.
+  // 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(int current_profiling_phase,
+                             PhasedProcessDataSnapshotMap* phased_snapshots,
                              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,
+  void SnapshotMaps(int profiling_phase,
+                    BirthMap* birth_map,
+                    DeathsSnapshot* deaths,
                     ParentChildSet* parent_child_set);
 
+  // Called for this thread when the current profiling phase, identified by
+  // |profiling_phase|, ends.
+  void OnProfilingPhaseCompletedOnThread(int profiling_phase);
+
   // This method is called by the TLS system when a thread terminates.
   // The argument may be NULL if this thread has never tracked a birth or death.
   static void OnThreadTermination(void* thread_data);
@@ -609,6 +689,11 @@ class BASE_EXPORT ThreadData {
   // instance and be safe.
   static base::LazyInstance<base::Lock>::Leaky list_lock_;
 
+  // Checker that all snapshots and phase change notifications happen in the
+  // same thread.
+  static base::LazyInstance<base::ThreadChecker>::Leaky
+      snapshot_thread_checker_;
+
   // We set status_ to SHUTDOWN when we shut down the tracking service.
   static Status status_;
 
@@ -783,7 +868,7 @@ struct BASE_EXPORT ProcessDataSnapshot {
   ProcessDataSnapshot();
   ~ProcessDataSnapshot();
 
-  PhasedProcessDataSnapshotMap phased_process_data_snapshots;
+  PhasedProcessDataSnapshotMap phased_snapshots;
   base::ProcessId process_id;
 };