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..12cbdadd316402a785ded6f4e138099df9ae95cb 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,79 @@ 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, bout this would

[Ilya Sherman, 2015/04/15 00:37:35]

nit: s/bout/but

[vadimt, 2015/04/15 19:14:31]

Done.

+  // create a loop since DeathData indirectly refers DeathDataSnapshot. Passing
+  // wrapper structure as a param or using an empty constructor for snapshotting

[Ilya Sherman, 2015/04/15 00:37:35]

nit: "Passing wrapper structure" -> "Passing a wrapper structure"

[vadimt, 2015/04/15 19:14:31]

Done.

+  // DeathData would be less efficient.

[Ilya Sherman, 2015/04/15 00:37:35]

It looks like DeathData only refers to this class via a pointer.  Why not
forward-declare these classes, so that DeathData can refer to them, but define
them below?

[vadimt, 2015/04/15 19:14:32]

We spent lots of effort breaking the loop between DeathDataSnapshot and
DeathData (asvitkine@ preference).
I think it would be in the spirit of these changes to now order the classes
correspondingly, otherwise, what was the point of breaking the loop?

+  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 delta between this snapshot and an earlier snapshot of the same
+  // task |older| and assigns it back to this object.
+  void SubtractOlderSnapshot(const DeathDataSnapshot& older);
+
+  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;

[Ilya Sherman, 2015/04/15 00:37:35]

Why not use a scoped_ptr, both here and below?

[vadimt, 2015/04/15 19:14:31]

DeathData owns the whole list of DeathDataPhaseSnapshot's and deletes everything
in its destructor in a loop.
However, we happen to create copies of DeathDataPhaseSnapshot, and we don't want
to deleted the list pointed by 'prev' when the copy is destructed.

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

Hmm, where do we create copies?  This seems like a somewhat fragile memory
management situation.  What guarantees that the memory won't be deleted out from
under the copy?

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

Terminological clarification: not quite a 'copy', but an instance of
DeathDataPhaseSnapshot gets created in ThreadData::SnapshotMaps.

DeathDatas are simply leaked in normal Chrome, i.e. ThreadDatas are never
deleted. This is by design, or at least, how it always was. Note that DeathDatas
are never removed from the death map.

In tests, ThreadData, and all DeathDatas that it owns inside its death map, are
deleted at the end of each test. Just in case, I added a thread check in
~ThreadData to totally rule out races now and for all.

+};
+
+//------------------------------------------------------------------------------
+// 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
+// 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
+// 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.

[Ilya Sherman, 2015/04/15 00:37:35]

nit: Please omit this comment.

[vadimt, 2015/04/15 19:14:31]

Done.

   DeathData();
 

[Ilya Sherman, 2015/04/15 00:37:35]

nit: Please omit this newline.

[vadimt, 2015/04/15 19:14:31]

Done.

-  // 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();
 
   // 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 +335,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 +344,47 @@ class BASE_EXPORT DeathData {
   int32 queue_duration_sum() const;
   int32 queue_duration_max() const;
   int32 queue_duration_sample() const;
+  DeathDataPhaseSnapshot* last_phase_snapshot() const;
 
-  // 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.
   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;
+  DISALLOW_COPY_AND_ASSIGN(DeathData);
 };
 
 //------------------------------------------------------------------------------
@@ -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,34 @@ 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
+  // |return_of_snapshot_hrenina|. Also updates the |birth_counts| tally for

[Ilya Sherman, 2015/04/15 00:37:35]

return_of_snapshot_hrenina?

[vadimt, 2015/04/15 19:14:31]

Done :)
One of the primordial names leaked into the final code :)

+  // 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 OnProfilingPhaseCompletionOnThread(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 +690,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 +869,7 @@ struct BASE_EXPORT ProcessDataSnapshot {
   ProcessDataSnapshot();
   ~ProcessDataSnapshot();
 
-  PhasedProcessDataSnapshotMap phased_process_data_snapshots;
+  PhasedProcessDataSnapshotMap phased_snapshots;
   base::ProcessId process_id;
 };