Introducing phased profiling framework

base/tracked_objects.cc

Index: base/tracked_objects.cc
diff --git a/base/tracked_objects.cc b/base/tracked_objects.cc
index 5359d892116577750cacff82a8522322c0b0b04b..8f06c1d4e9302da72d164076ec29b7a868ea73bf 100644
--- a/base/tracked_objects.cc
+++ b/base/tracked_objects.cc
@@ -390,23 +390,10 @@ void ThreadData::OnThreadTerminationCleanup() {
 }
 
 // static
-void ThreadData::Snapshot(ProcessDataSnapshot* process_data) {
-  // 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, &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 (BirthCountMap::const_iterator it = birth_counts.begin();
-       it != birth_counts.end(); ++it) {
-    if (it->second > 0) {
-      process_data->tasks.push_back(
-          TaskSnapshot(*it->first, DeathData(it->second), "Still_Alive"));
-    }
-  }
+void ThreadData::GetProcessDataSnapshot(
+    ProcessDataSnapshot* process_data_snapshot) {
+  ThreadData::Snapshot(
+      &process_data_snapshot->phased_process_data_snapshots[0]);

[Ilya Sherman, 2015/03/10 00:48:10]

What guarantees that the vector is non-empty?

[vadimt, 2015/03/13 23:18:00]

It's a map, and indexing it creates an entry.

 }
 
 Births* ThreadData::TallyABirth(const Location& location) {
@@ -578,8 +565,9 @@ void ThreadData::TallyRunInAScopedRegionIfTracking(
 }
 
 // static
-void ThreadData::SnapshotAllExecutedTasks(ProcessDataSnapshot* process_data,
-                                          BirthCountMap* birth_counts) {
+void ThreadData::SnapshotAllExecutedTasks(
+    ProcessDataPhaseSnapshot* process_data_phase,
+    BirthCountMap* birth_counts) {
   if (!kTrackAllTaskObjects)
     return;  // Not compiled in.
 
@@ -595,12 +583,32 @@ void ThreadData::SnapshotAllExecutedTasks(ProcessDataSnapshot* process_data,
   for (ThreadData* thread_data = my_list;
        thread_data;
        thread_data = thread_data->next()) {
-    thread_data->SnapshotExecutedTasks(process_data, birth_counts);
+    thread_data->SnapshotExecutedTasks(process_data_phase, birth_counts);
+  }
+}
+
+// static
+void ThreadData::Snapshot(ProcessDataPhaseSnapshot* process_data_phase) {

[Ilya Sherman, 2015/03/10 00:48:10]

nit: I'd prefer to give this method a new name, and preserve the name of the
pre-existing method that was named "Snapshot" (i.e. the one that you've
currently renamed to "GetProcessDataSnapshot").

[vadimt, 2015/03/13 23:18:00]

Done.

+  // 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& i : birth_counts) {

[Ilya Sherman, 2015/03/10 00:48:10]

nit: Please use a more semantically contentful name for the variable, or at
least "it" for an iterator rather than "i".  My preference in this case would
probably be "birth" or "entry".

[vadimt, 2015/03/13 23:18:00]

Done.

+    if (i.second > 0) {
+      process_data_phase->tasks.push_back(
+          TaskSnapshot(*i.first, DeathData(i.second), "Still_Alive"));
+    }
   }
 }
 
-void ThreadData::SnapshotExecutedTasks(ProcessDataSnapshot* process_data,
-                                       BirthCountMap* birth_counts) {
+void ThreadData::SnapshotExecutedTasks(
+    ProcessDataPhaseSnapshot* process_data_phase,
+    BirthCountMap* birth_counts) {
   // Get copy of data, so that the data will not change during the iterations
   // and processing.
   ThreadData::BirthMap birth_map;
@@ -608,24 +616,21 @@ void ThreadData::SnapshotExecutedTasks(ProcessDataSnapshot* process_data,
   ThreadData::ParentChildSet parent_child_set;
   SnapshotMaps(&birth_map, &death_map, &parent_child_set);
 
-  for (ThreadData::DeathMap::const_iterator it = death_map.begin();
-       it != death_map.end(); ++it) {
-    process_data->tasks.push_back(
-        TaskSnapshot(*it->first, it->second, thread_name()));
-    (*birth_counts)[it->first] -= it->first->birth_count();
+  for (const auto& i : death_map) {

[Ilya Sherman, 2015/03/10 00:48:10]

Same comment applies here, and for all other loops.

[vadimt, 2015/03/13 23:18:01]

Done.

+    process_data_phase->tasks.push_back(
+        TaskSnapshot(*i.first, i.second, thread_name()));
+    (*birth_counts)[i.first] -= i.first->birth_count();
   }
 
-  for (ThreadData::BirthMap::const_iterator it = birth_map.begin();
-       it != birth_map.end(); ++it) {
-    (*birth_counts)[it->second] += it->second->birth_count();
+  for (const auto& i : birth_map) {
+    (*birth_counts)[i.second] += i.second->birth_count();
   }
 
   if (!kTrackParentChildLinks)
     return;
 
-  for (ThreadData::ParentChildSet::const_iterator it = parent_child_set.begin();
-       it != parent_child_set.end(); ++it) {
-    process_data->descendants.push_back(ParentChildPairSnapshot(*it));
+  for (const auto& i : parent_child_set) {
+    process_data_phase->descendants.push_back(ParentChildPairSnapshot(i));
   }
 }
 
@@ -634,20 +639,16 @@ void ThreadData::SnapshotMaps(BirthMap* birth_map,
                               DeathMap* death_map,
                               ParentChildSet* parent_child_set) {
   base::AutoLock lock(map_lock_);
-  for (BirthMap::const_iterator it = birth_map_.begin();
-       it != birth_map_.end(); ++it)
-    (*birth_map)[it->first] = it->second;
-  for (DeathMap::iterator it = death_map_.begin();
-       it != death_map_.end(); ++it) {
-    (*death_map)[it->first] = it->second;
-  }
+  for (const auto& i : birth_map_)
+    (*birth_map)[i.first] = i.second;
+  for (auto& i : death_map_)
+    (*death_map)[i.first] = i.second;
 
   if (!kTrackParentChildLinks)
     return;
 
-  for (ParentChildSet::iterator it = parent_child_set_.begin();
-       it != parent_child_set_.end(); ++it)
-    parent_child_set->insert(*it);
+  for (const auto& i : parent_child_set_)
+    parent_child_set->insert(i);

[Ilya Sherman, 2015/03/10 00:48:10]

I'd prefer to see the C++11-ization of the for loops in this file, and any other
purely stylistic cleanups", as a separate CL.  This is important for revision
history reasons -- it's misleading to see this line modified as part of a CL
that seems to be doing something major, when it's just an incidental style
change to slightly-related code.

[vadimt, 2015/03/13 23:18:00]

At some point, I've done C++11-ization by asvitkine request.
Now, it will take considerable time to separate it for a pretty small benefit.

I highly respect the time and convenience of reviewers, and the perfection of
the CL, but we should also, if possible, use the time of the reviewee
efficiently too. This way, we'll likely achieve maximal overall performance of
the review process.

Besides, there are no substantial changes in base/ in this CL, so we can
consider the base/ changes as mostly renaming and cleanup.

 }
 
 static void OptionallyInitializeAlternateTimer() {
@@ -959,7 +960,16 @@ ParentChildPairSnapshot::~ParentChildPairSnapshot() {
 }
 
 //------------------------------------------------------------------------------
-// ProcessDataSnapshot
+// ProcessDataPhaseSnapshot
+
+ProcessDataPhaseSnapshot::ProcessDataPhaseSnapshot() {
+}
+
+ProcessDataPhaseSnapshot::~ProcessDataPhaseSnapshot() {
+}
+
+//------------------------------------------------------------------------------
+// ProcessDataPhaseSnapshot
 
 ProcessDataSnapshot::ProcessDataSnapshot()
 #if !defined(OS_NACL)