Introducing phased profiling framework

base/tracked_objects.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/tracked_objects.h"
 
 #include <limits.h>
 #include <stdlib.h>
 
 #include "base/atomicops.h"
@@ skipping 372 matching lines @@
     return;
   }
   // We must NOT do any allocations during this callback.
   // Using the simple linked lists avoids all allocations.
   DCHECK_EQ(this->next_retired_worker_, reinterpret_cast<ThreadData*>(NULL));
   this->next_retired_worker_ = first_retired_worker_;
   first_retired_worker_ = this;
 }
 
 // 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) {
   BirthMap::iterator it = birth_map_.find(location);
   Births* child;
   if (it != birth_map_.end()) {
     child =  it->second;
     child->RecordBirth();
   } else {
     child = new Births(location, *this);  // Leak this.
@@ skipping 151 matching lines @@
 
   ThreadData* current_thread_data = stopwatch.GetThreadData();
   if (!current_thread_data)
     return;
 
   int32 queue_duration = 0;
   current_thread_data->TallyADeath(*birth, queue_duration, stopwatch);
 }
 
 // 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.
 
   // 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, birth_counts);
+    thread_data->SnapshotExecutedTasks(process_data_phase, birth_counts);
   }
 }
 
-void ThreadData::SnapshotExecutedTasks(ProcessDataSnapshot* process_data,
-                                       BirthCountMap* 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(
+    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;
   ThreadData::DeathMap death_map;
   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));
   }
 }
 
 // This may be called from another thread.
 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() {
   NowFunction* alternate_time_source = GetAlternateTimeSource();
   if (alternate_time_source)
     ThreadData::SetAlternateTimeSource(alternate_time_source);
 }
 
 bool ThreadData::Initialize() {
   if (!kTrackAllTaskObjects)
@@ skipping 291 matching lines @@
 ParentChildPairSnapshot::ParentChildPairSnapshot(
     const ThreadData::ParentChildPair& parent_child)
     : parent(*parent_child.first),
       child(*parent_child.second) {
 }
 
 ParentChildPairSnapshot::~ParentChildPairSnapshot() {
 }
 
 //------------------------------------------------------------------------------
-// ProcessDataSnapshot
+// ProcessDataPhaseSnapshot
+
+ProcessDataPhaseSnapshot::ProcessDataPhaseSnapshot() {
+}
+
+ProcessDataPhaseSnapshot::~ProcessDataPhaseSnapshot() {
+}
+
+//------------------------------------------------------------------------------
+// ProcessDataPhaseSnapshot
 
 ProcessDataSnapshot::ProcessDataSnapshot()
 #if !defined(OS_NACL)
     : process_id(base::GetCurrentProcId()) {
 #else
     : process_id(0) {
 #endif
 }
 
 ProcessDataSnapshot::~ProcessDataSnapshot() {
 }
 
 }  // namespace tracked_objects