Delivering the FIRST_NONEMPTY_PAINT phase changing event to base/

base/tracked_objects.cc

Index: base/tracked_objects.cc
diff --git a/base/tracked_objects.cc b/base/tracked_objects.cc
index a60e260b23660b31140593f6b7e0b63652a18f8b..7996a1d49c255041cd13cfae8da90ca003070485 100644
--- a/base/tracked_objects.cc
+++ b/base/tracked_objects.cc
@@ -93,12 +93,23 @@ inline bool IsProfilerTimingEnabled() {
 // DeathData tallies durations when a death takes place.
 
 DeathData::DeathData() {
-  Clear();
+  count_ = 0;
+  sample_probability_count_ = 0;
+  run_duration_sum_ = 0;
+  run_duration_max_ = 0;
+  run_duration_sample_ = 0;
+  queue_duration_sum_ = 0;
+  queue_duration_max_ = 0;
+  queue_duration_sample_ = 0;
+  last_phase_snapshot_ = nullptr;

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

Why aren't these being set as part of the initializer list?

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

Done.

 }
 
-DeathData::DeathData(int count) {
-  Clear();
-  count_ = count;
+DeathData::~DeathData() {
+  while (last_phase_snapshot_) {
+    DeathDataPhaseSnapshot* snapshot = last_phase_snapshot_;
+    last_phase_snapshot_ = snapshot->prev;
+    delete snapshot;
+  }
 }
 
 // TODO(jar): I need to see if this macro to optimize branching is worth using.
@@ -117,6 +128,8 @@ void DeathData::RecordDeath(const int32 queue_duration,
   // We'll just clamp at INT_MAX, but we should note this in the UI as such.
   if (count_ < INT_MAX)
     ++count_;
+  if (sample_probability_count_ < INT_MAX)
+    ++sample_probability_count_;
   queue_duration_sum_ += queue_duration;
   run_duration_sum_ += run_duration;
 
@@ -125,14 +138,16 @@ void DeathData::RecordDeath(const int32 queue_duration,
   if (run_duration_max_ < run_duration)
     run_duration_max_ = run_duration;
 
-  // Take a uniformly distributed sample over all durations ever supplied.
-  // The probability that we (instead) use this new sample is 1/count_.  This
-  // results in a completely uniform selection of the sample (at least when we
-  // don't clamp count_... but that should be inconsequentially likely).
-  // We ignore the fact that we correlated our selection of a sample to the run
-  // and queue times (i.e., we used them to generate random_number).
-  CHECK_GT(count_, 0);
-  if (0 == (random_number % count_)) {
+  // Take a uniformly distributed sample over all durations ever supplied during
+  // currrent profiling phase.

[Ilya Sherman, 2015/04/15 00:59:12]

nit: "currrent" -> "the current" (add "the", drop an "r")

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

Done.

+  // The probability that we (instead) use this new sample is
+  // 1/sample_probability_count_. This results in a completely uniform selection
+  // of the sample (at least when we don't clamp sample_probability_count_...
+  // but that should be inconsequentially likely). We ignore the fact that we
+  // correlated our selection of a sample to the run and queue times (i.e., we
+  // used them to generate random_number).
+  CHECK_GT(sample_probability_count_, 0);
+  if (0 == (random_number % sample_probability_count_)) {
     queue_duration_sample_ = queue_duration;
     run_duration_sample_ = run_duration;
   }
@@ -160,14 +175,43 @@ int32 DeathData::queue_duration_sample() const {
   return queue_duration_sample_;
 }
 
-void DeathData::Clear() {
-  count_ = 0;
-  run_duration_sum_ = 0;
+DeathDataPhaseSnapshot* DeathData::last_phase_snapshot() const {
+  return last_phase_snapshot_;
+}
+
+void DeathData::OnProfilingPhaseCompleted(int profiling_phase) {
+  // Snapshotting and storing current state.
+  last_phase_snapshot_ = new DeathDataPhaseSnapshot(
+      profiling_phase, count_, run_duration_sum_, run_duration_max_,
+      run_duration_sample_, queue_duration_sum_, queue_duration_max_,
+      queue_duration_sample_, last_phase_snapshot_);
+
+  // Not touching fields for which a delta can be computed by comparing with a
+  // snapshot from previos phase. Resetting other fields. Sample values will be
+  // reset upon next death recording because sample_probability_count_ is set to
+  // 0.
+  // We avoid resetting to 0 in favor of deltas whenever possible. The reason is
+  // that for incrementable fields, resetting to 0 from the snapshot thread
+  // potentially in parallel with incrementing in the death thread may result in
+  // significant data corruption that has a potential to grow with time. Not
+  // resetting incrementable fields and using deltas will cause any
+  // off-by-little corruptions to be likely fixed at the next snapshot.
+  // The max values are not incrementable, and cannot be deduced using deltas
+  // for a given phase. Hence, we have to reset them to 0. But the potential
+  // damage is limited to getting the previous phase's max to apply for the next
+  // phase, and the error doesn't have a potential to keep growing with new
+  // resets.
+  // sample_probability_count_ is incrementable, but must be reset to 0 at the
+  // phase end, so that we start a new uniformly randomized sample selection
+  // after the reset. Corruptions due to race conditions are possible, but the
+  // damage is limited to selecting a wrong sample, which is not something that
+  // can cause accumulating or cascading effects.
+  // If there were no corruptions caused by race conditions, we never send a
+  // sample for the previous phase in the next phase's snapshot because
+  // ThreadData::SnapshotExecutedTasks doesn't send deltas with 0 count.
+  sample_probability_count_ = 0;
   run_duration_max_ = 0;
-  run_duration_sample_ = 0;
-  queue_duration_sum_ = 0;
   queue_duration_max_ = 0;
-  queue_duration_sample_ = 0;
 }
 
 //------------------------------------------------------------------------------
@@ -181,20 +225,31 @@ DeathDataSnapshot::DeathDataSnapshot()
       queue_duration_sample(-1) {
 }
 
-DeathDataSnapshot::DeathDataSnapshot(
-    const tracked_objects::DeathData& death_data)
-    : count(death_data.count()),
-      run_duration_sum(death_data.run_duration_sum()),
-      run_duration_max(death_data.run_duration_max()),
-      run_duration_sample(death_data.run_duration_sample()),
-      queue_duration_sum(death_data.queue_duration_sum()),
-      queue_duration_max(death_data.queue_duration_max()),
-      queue_duration_sample(death_data.queue_duration_sample()) {
+DeathDataSnapshot::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)
+    : count(count),
+      run_duration_sum(run_duration_sum),
+      run_duration_max(run_duration_max),
+      run_duration_sample(run_duration_sample),
+      queue_duration_sum(queue_duration_sum),
+      queue_duration_max(queue_duration_max),
+      queue_duration_sample(queue_duration_sample) {
 }
 
 DeathDataSnapshot::~DeathDataSnapshot() {
 }
 
+void DeathDataSnapshot::SubtractOlderSnapshot(const DeathDataSnapshot& older) {
+  count -= older.count;
+  run_duration_sum -= older.run_duration_sum;
+  queue_duration_sum -= older.queue_duration_sum;
+}
+
 //------------------------------------------------------------------------------
 BirthOnThread::BirthOnThread(const Location& location,
                              const ThreadData& current)
@@ -206,8 +261,7 @@ BirthOnThread::BirthOnThread(const Location& location,
 BirthOnThreadSnapshot::BirthOnThreadSnapshot() {
 }
 
-BirthOnThreadSnapshot::BirthOnThreadSnapshot(
-    const tracked_objects::BirthOnThread& birth)
+BirthOnThreadSnapshot::BirthOnThreadSnapshot(const BirthOnThread& birth)
     : location(birth.location()),
       thread_name(birth.birth_thread()->thread_name()) {
 }
@@ -264,6 +318,10 @@ base::LazyInstance<base::Lock>::Leaky
     ThreadData::list_lock_ = LAZY_INSTANCE_INITIALIZER;
 
 // static
+base::LazyInstance<base::ThreadChecker>::Leaky
+    ThreadData::snapshot_thread_checker_ = LAZY_INSTANCE_INITIALIZER;
+
+// static
 ThreadData::Status ThreadData::status_ = ThreadData::UNINITIALIZED;
 
 ThreadData::ThreadData(const std::string& suggested_name)
@@ -385,9 +443,58 @@ void ThreadData::OnThreadTerminationCleanup() {
 }
 
 // static
-void ThreadData::Snapshot(ProcessDataSnapshot* process_data_snapshot) {
-  ThreadData::SnapshotCurrentPhase(
-      &process_data_snapshot->phased_process_data_snapshots[0]);
+void ThreadData::Snapshot(int current_profiling_phase,
+                          ProcessDataSnapshot* process_data_snapshot) {
+  DCHECK(snapshot_thread_checker_.Get().CalledOnValidThread());
+
+  // Get an unchanging copy of a ThreadData list.
+  ThreadData* my_list = ThreadData::first();
+
+  // Gather data serially.
+  // This hackish approach *can* get some slighly corrupt tallies, as we are

[Ilya Sherman, 2015/04/15 00:59:12]

nit: hackish -> hacky or hackyish?

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

Hackish is more correct than Hackyish (based on Web frequency).
As for Hacky, I believe it better reflects jar's idea that it's not very hacky,
just slightly...

+  // grabbing values without the protection of a lock, but it has the advantage
+  // of working even with threads that don't have message loops.  If a user
+  // sees any strangeness, they can always just run their stats gathering a
+  // second time.
+  BirthCountMap birth_counts;
+
+  for (ThreadData* thread_data = my_list; thread_data;
+       thread_data = thread_data->next()) {
+    thread_data->SnapshotExecutedTasks(current_profiling_phase,
+                                       &process_data_snapshot->phased_snapshots,
+                                       &birth_counts);
+  }
+
+  // Add births that are still active -- i.e. objects that have tallied a birth,
+  // but have not yet tallied a matching death, and hence must be either
+  // running, queued up, or being held in limbo for future posting.
+  auto current_phase_tasks =
+      &process_data_snapshot->phased_snapshots[current_profiling_phase].tasks;
+  for (const auto& birth_count : birth_counts) {
+    if (birth_count.second > 0) {
+      current_phase_tasks->push_back(
+          TaskSnapshot(BirthOnThreadSnapshot(*birth_count.first),
+                       DeathDataSnapshot(birth_count.second, 0, 0, 0, 0, 0, 0),
+                       "Still_Alive"));
+    }
+  }
+}
+
+// static
+void ThreadData::OnProfilingPhaseCompleted(int profiling_phase) {
+  DCHECK(snapshot_thread_checker_.Get().CalledOnValidThread());
+  // Get an unchanging copy of a ThreadData list.
+  ThreadData* my_list = ThreadData::first();
+
+  // Add snapshots for all death datas in all threads serially.

[Ilya Sherman, 2015/04/15 00:59:12]

nit: "data" is already plural.

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

Done.

+  // This hackish approach *can* get some slighly corrupt tallies, as we are
+  // grabbing values without the protection of a lock, but it has the advantage
+  // of working even with threads that don't have message loops. Any corruption
+  // shouldn't cause "cascading damage" to anything else (in later phases).
+  for (ThreadData* thread_data = my_list; thread_data;
+       thread_data = thread_data->next()) {
+    thread_data->OnProfilingPhaseCompletionOnThread(profiling_phase);
+  }
 }
 
 Births* ThreadData::TallyABirth(const Location& location) {
@@ -419,7 +526,7 @@ Births* ThreadData::TallyABirth(const Location& location) {
   return child;
 }
 
-void ThreadData::TallyADeath(const Births& birth,
+void ThreadData::TallyADeath(const Births& births,
                              int32 queue_duration,
                              const TaskStopwatch& stopwatch) {
   int32 run_duration = stopwatch.RunDurationMs();
@@ -428,7 +535,7 @@ void ThreadData::TallyADeath(const Births& birth,
   const uint32 kSomePrimeNumber = 2147483647;
   random_number_ += queue_duration + run_duration + kSomePrimeNumber;
   // An address is going to have some randomness to it as well ;-).
-  random_number_ ^= static_cast<uint32>(&birth - reinterpret_cast<Births*>(0));
+  random_number_ ^= static_cast<uint32>(&births - reinterpret_cast<Births*>(0));
 
   // We don't have queue durations without OS timer. OS timer is automatically
   // used for task-post-timing, so the use of an alternate timer implies all
@@ -440,20 +547,20 @@ void ThreadData::TallyADeath(const Births& birth,
     queue_duration = 0;
   }
 
-  DeathMap::iterator it = death_map_.find(&birth);
+  DeathMap::iterator it = death_map_.find(&births);
   DeathData* death_data;
   if (it != death_map_.end()) {
     death_data = &it->second;
   } else {
     base::AutoLock lock(map_lock_);  // Lock as the map may get relocated now.
-    death_data = &death_map_[&birth];
+    death_data = &death_map_[&births];
   }  // Release lock ASAP.
   death_data->RecordDeath(queue_duration, run_duration, random_number_);
 
   if (!kTrackParentChildLinks)
     return;
   if (!parent_stack_.empty()) {  // We might get turned off.
-    DCHECK_EQ(parent_stack_.top(), &birth);
+    DCHECK_EQ(parent_stack_.top(), &births);
     parent_stack_.pop();
   }
 }
@@ -475,8 +582,8 @@ void ThreadData::TallyRunOnNamedThreadIfTracking(
   // Even if we have been DEACTIVATED, we will process any pending births so
   // that our data structures (which counted the outstanding births) remain
   // consistent.
-  const Births* birth = completed_task.birth_tally;
-  if (!birth)
+  const Births* births = completed_task.birth_tally;
+  if (!births)
     return;
   ThreadData* current_thread_data = stopwatch.GetThreadData();
   if (!current_thread_data)
@@ -493,18 +600,18 @@ void ThreadData::TallyRunOnNamedThreadIfTracking(
     queue_duration = (start_of_run - completed_task.EffectiveTimePosted())
         .InMilliseconds();
   }
-  current_thread_data->TallyADeath(*birth, queue_duration, stopwatch);
+  current_thread_data->TallyADeath(*births, queue_duration, stopwatch);
 }
 
 // static
 void ThreadData::TallyRunOnWorkerThreadIfTracking(
-    const Births* birth,
+    const Births* births,
     const TrackedTime& time_posted,
     const TaskStopwatch& stopwatch) {
   // Even if we have been DEACTIVATED, we will process any pending births so
   // that our data structures (which counted the outstanding births) remain
   // consistent.
-  if (!birth)
+  if (!births)
     return;
 
   // TODO(jar): Support the option to coalesce all worker-thread activity under
@@ -525,17 +632,17 @@ void ThreadData::TallyRunOnWorkerThreadIfTracking(
   if (!start_of_run.is_null()) {
     queue_duration = (start_of_run - time_posted).InMilliseconds();
   }
-  current_thread_data->TallyADeath(*birth, queue_duration, stopwatch);
+  current_thread_data->TallyADeath(*births, queue_duration, stopwatch);
 }
 
 // static
 void ThreadData::TallyRunInAScopedRegionIfTracking(
-    const Births* birth,
+    const Births* births,
     const TaskStopwatch& stopwatch) {
   // Even if we have been DEACTIVATED, we will process any pending births so
   // that our data structures (which counted the outstanding births) remain
   // consistent.
-  if (!birth)
+  if (!births)
     return;
 
   ThreadData* current_thread_data = stopwatch.GetThreadData();
@@ -543,87 +650,74 @@ void ThreadData::TallyRunInAScopedRegionIfTracking(
     return;
 
   int32 queue_duration = 0;
-  current_thread_data->TallyADeath(*birth, queue_duration, stopwatch);
-}
-
-// static
-void ThreadData::SnapshotAllExecutedTasks(
-    ProcessDataPhaseSnapshot* process_data_phase,
-    BirthCountMap* birth_counts) {
-  // Get an unchanging copy of a ThreadData list.
-  ThreadData* my_list = ThreadData::first();
-
-  // Gather data serially.
-  // This hackish approach *can* get some slighly corrupt tallies, as we are
-  // grabbing values without the protection of a lock, but it has the advantage
-  // of working even with threads that don't have message loops.  If a user
-  // sees any strangeness, they can always just run their stats gathering a
-  // second time.
-  for (ThreadData* thread_data = my_list;
-       thread_data;
-       thread_data = thread_data->next()) {
-    thread_data->SnapshotExecutedTasks(process_data_phase, birth_counts);
-  }
-}
-
-// static
-void ThreadData::SnapshotCurrentPhase(
-    ProcessDataPhaseSnapshot* process_data_phase) {
-  // Add births that have run to completion to |collected_data|.
-  // |birth_counts| tracks the total number of births recorded at each location
-  // for which we have not seen a death count.
-  BirthCountMap birth_counts;
-  ThreadData::SnapshotAllExecutedTasks(process_data_phase, &birth_counts);
-
-  // Add births that are still active -- i.e. objects that have tallied a birth,
-  // but have not yet tallied a matching death, and hence must be either
-  // running, queued up, or being held in limbo for future posting.
-  for (const auto& birth_count : birth_counts) {
-    if (birth_count.second > 0) {
-      process_data_phase->tasks.push_back(TaskSnapshot(
-          *birth_count.first, DeathData(birth_count.second), "Still_Alive"));
-    }
-  }
+  current_thread_data->TallyADeath(*births, queue_duration, stopwatch);
 }
 
 void ThreadData::SnapshotExecutedTasks(
-    ProcessDataPhaseSnapshot* process_data_phase,
+    int current_profiling_phase,
+    PhasedProcessDataSnapshotMap* phased_snapshots,
     BirthCountMap* birth_counts) {
   // Get copy of data, so that the data will not change during the iterations
   // and processing.
-  ThreadData::BirthMap birth_map;
-  ThreadData::DeathMap death_map;
-  ThreadData::ParentChildSet parent_child_set;
-  SnapshotMaps(&birth_map, &death_map, &parent_child_set);
-
-  for (const auto& death : death_map) {
-    process_data_phase->tasks.push_back(
-        TaskSnapshot(*death.first, death.second, thread_name()));
-    (*birth_counts)[death.first] -= death.first->birth_count();
-  }
+  BirthMap birth_map;
+  DeathsSnapshot deaths;
+  ParentChildSet parent_child_set;
+  SnapshotMaps(current_profiling_phase, &birth_map, &deaths, &parent_child_set);
 
   for (const auto& birth : birth_map) {
     (*birth_counts)[birth.second] += birth.second->birth_count();
   }
 
-  if (!kTrackParentChildLinks)
-    return;
+  for (const auto& death : deaths) {
+    (*birth_counts)[death.first] -= death.first->birth_count();
 
-  for (const auto& parent_child : parent_child_set) {
-    process_data_phase->descendants.push_back(
-        ParentChildPairSnapshot(parent_child));
+    // For the current death data, walk through all its snapshots, starting from
+    // the current one, then from the previous profiling phase etc., and for
+    // each snapshot calculate the delta between the snapshot and the previous
+    // phase, if any. Store the deltas in the result.
+    for (const DeathDataPhaseSnapshot* phase = &death.second; phase;
+         phase = phase->prev) {
+      // Taking a temporary copy of the DeathDataSnapshot. We need this copy to
+      // avoid modification of the original snapshots in the DeathData’s list by
+      // calling SubtractOlderSnapshot on them. If we modified them, then future
+      // calls to SnapshotExecutedTasks would return increasingly corrupt
+      // results.

[Ilya Sherman, 2015/04/15 00:59:12]

It seems easier for SubtractSnapshot() to just return a copy, rather than
modifying the object in place.  Why did you decide not to follow that route?

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

Done.
The original idea was to reduce number of copies. But seems like return value
optimization can solve this.

+      DeathDataSnapshot death_data = phase->death_data;
+
+      if (phase->prev)
+        death_data.SubtractOlderSnapshot(phase->prev->death_data);

[Ilya Sherman, 2015/04/15 00:59:11]

It looks like you have to repeat this calculation multiple times.  Why not just
store the deltas?

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

For the majority of users who don't open chrome://profiler, we calculate deltas
only when/if we send profiler data, so I don't see perf issues.

Storing deltas would make the algorithms less clear. It would be less clear what
we store: actual values in DeathData, deltas in all chained snapshots except the
earliest one, actual numbers again for the first one.
We'd have to calculate deltas in 2 different places: when making snapshot - for
the current numbers in DeathData, and also when making phase change-related
snapshot. It's easier to mess with this...

+
+      if (death_data.count > 0) {
+        (*phased_snapshots)[phase->profiling_phase].tasks.push_back(
+            TaskSnapshot(BirthOnThreadSnapshot(*death.first), death_data,
+                         thread_name()));
+      }
+    }
   }
 }
 
 // This may be called from another thread.
-void ThreadData::SnapshotMaps(BirthMap* birth_map,
-                              DeathMap* death_map,
+void ThreadData::SnapshotMaps(int profiling_phase,
+                              BirthMap* birth_map,
+                              DeathsSnapshot* deaths,
                               ParentChildSet* parent_child_set) {
   base::AutoLock lock(map_lock_);
+
   for (const auto& birth : birth_map_)
     (*birth_map)[birth.first] = birth.second;
-  for (const auto& death : death_map_)
-    (*death_map)[death.first] = death.second;
+
+  for (const auto& death : death_map_) {
+    deaths->push_back(DeathsSnapshot::value_type(
+        death.first,
+        DeathDataPhaseSnapshot(profiling_phase, death.second.count(),
+                               death.second.run_duration_sum(),
+                               death.second.run_duration_max(),
+                               death.second.run_duration_sample(),
+                               death.second.queue_duration_sum(),
+                               death.second.queue_duration_max(),
+                               death.second.queue_duration_sample(),
+                               death.second.last_phase_snapshot())));
+  }
 
   if (!kTrackParentChildLinks)
     return;
@@ -632,6 +726,14 @@ void ThreadData::SnapshotMaps(BirthMap* birth_map,
     parent_child_set->insert(parent_child);
 }
 
+void ThreadData::OnProfilingPhaseCompletionOnThread(int profiling_phase) {

[Ilya Sherman, 2015/04/15 00:59:12]

nit: Should "Completion" be "Completed"?

[Ilya Sherman, 2015/04/15 00:59:12]

What does "OnThread" mean?

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

Done.

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

This is to differentiate the instance member from the static
ThreadData::OnProfilingPhaseCompleted.
Better ideas are welcome, but I'd dislike something like
OnProfilingPhaseCompletedNonStatic :)

+  base::AutoLock lock(map_lock_);
+
+  for (auto& death : death_map_) {
+    death.second.OnProfilingPhaseCompleted(profiling_phase);
+  }
+}
+
 static void OptionallyInitializeAlternateTimer() {
   NowFunction* alternate_time_source = GetAlternateTimeSource();
   if (alternate_time_source)
@@ -909,11 +1011,36 @@ ThreadData* TaskStopwatch::GetThreadData() const {
 }
 
 //------------------------------------------------------------------------------
+// DeathDataPhaseSnapshot
+
+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(profiling_phase),
+      death_data(count,
+                 run_duration_sum,
+                 run_duration_max,
+                 run_duration_sample,
+                 queue_duration_sum,
+                 queue_duration_max,
+                 queue_duration_sample),
+      prev(prev) {
+}
+
+//------------------------------------------------------------------------------
+// TaskSnapshot
+
 TaskSnapshot::TaskSnapshot() {
 }
 
-TaskSnapshot::TaskSnapshot(const BirthOnThread& birth,
-                           const DeathData& death_data,
+TaskSnapshot::TaskSnapshot(const BirthOnThreadSnapshot& birth,
+                           const DeathDataSnapshot& death_data,
                            const std::string& death_thread_name)
     : birth(birth),
       death_data(death_data),