PerformanceMonitor: UMA alert for high browser CPU usage.

chrome/browser/performance_monitor/performance_monitor.cc

Index: chrome/browser/performance_monitor/performance_monitor.cc
diff --git a/chrome/browser/performance_monitor/performance_monitor.cc b/chrome/browser/performance_monitor/performance_monitor.cc
index b800e7f902af12739662b6916f136d0d7605313b..993f6c2adc30cf224a1ee331ec37874ab7c02573 100644
--- a/chrome/browser/performance_monitor/performance_monitor.cc
+++ b/chrome/browser/performance_monitor/performance_monitor.cc
@@ -10,6 +10,7 @@
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
+#include "base/memory/singleton.h"
 #include "base/process/process_iterator.h"
 #include "base/stl_util.h"
 #include "base/strings/string_number_conversions.h"
@@ -29,9 +30,10 @@
 #include "chrome/common/chrome_version_info.h"
 #include "chrome/common/extensions/extension.h"
 #include "chrome/common/extensions/extension_constants.h"
-#include "chrome/test/base/chrome_process_util.h"
 #include "content/public/browser/browser_child_process_host.h"
+#include "content/public/browser/browser_child_process_host_iterator.h"
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/child_process_data.h"
 #include "content/public/browser/load_notification_details.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
@@ -53,8 +55,6 @@ const uint32 kAccessFlags = base::kProcessAccessDuplicateHandle |
                             base::kProcessAccessTerminate |
                             base::kProcessAccessWaitForTermination;
 
-bool g_started_initialization = false;
-
 std::string TimeToString(base::Time time) {
   int64 time_int64 = time.ToInternalValue();
   return base::Int64ToString(time_int64);
@@ -99,7 +99,15 @@ PerformanceMonitor::PerformanceDataForIOThread::PerformanceDataForIOThread()
     : network_bytes_read(0) {
 }
 
-PerformanceMonitor::PerformanceMonitor() : metrics_map_(new MetricsMap) {}
+PerformanceMonitor::PerformanceMonitor()
+    : gather_interval_in_seconds_(kDefaultGatherIntervalInSeconds),
+      database_logging_enabled_(false),
+      timer_(FROM_HERE,
+             base::TimeDelta::FromSeconds(kSampleInterval),
+             this,
+             &PerformanceMonitor::DoTimedCollections),
+      disable_timer_autostart_(false) {
+}
 
 PerformanceMonitor::~PerformanceMonitor() {
   BrowserThread::PostBlockingPoolSequencedTask(
@@ -126,11 +134,38 @@ PerformanceMonitor* PerformanceMonitor::GetInstance() {
 void PerformanceMonitor::Start() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  // Avoid responding to multiple calls to Start().
-  if (g_started_initialization)
-    return;
+  static bool has_initialized = false;
+  DCHECK(!has_initialized);

[Devlin, 2013/09/12 18:20:42]

this should probably be:
if (has_initialized) {
  NOTREACHED();
  return;
}

To gracefully handle anything weird in release.

[oystein (OOO til 10th of July), 2013/09/12 19:03:24]

Done.

+  has_initialized = true;
+
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kPerformanceMonitorGathering)) {
+    database_logging_enabled_ = true;

[Devlin, 2013/09/12 18:20:42]

This can potentially introduce race conditions.  It should only be set once the
database is actually ready.

[oystein (OOO til 10th of July), 2013/09/12 19:03:24]

That's not the meaning of the flag, it's more of the database logging *system*
is enabled rather than the database itself being ready for logging. There could
only be race conditions here if they were already present before this CL, as far
as I can tell.

+
+    if (!CommandLine::ForCurrentProcess()->

[Devlin, 2013/09/12 18:20:42]

nit: operator ! doesn't count for indentation level. Should be
if (!CommandLine::ForCurrentProcess()->HasSwitch(
        switches::kPerformanceMonitorGathering)) {

[oystein (OOO til 10th of July), 2013/09/12 19:03:24]

Done.

+             GetSwitchValueASCII(switches::kPerformanceMonitorGathering)
+             .empty()) {
+      int specified_interval = 0;
+      if (!base::StringToInt(

[Devlin, 2013/09/12 18:20:42]

nit: indendation.

[oystein (OOO til 10th of July), 2013/09/12 19:03:24]

Done.

+             CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
+                 switches::kPerformanceMonitorGathering),
+             &specified_interval) ||
+          specified_interval <= 0) {
+        LOG(ERROR) << "Invalid value for switch: '"
+                   << switches::kPerformanceMonitorGathering
+                   << "'; please use an integer greater than 0.";
+      } else {
+        gather_interval_in_seconds_ = specified_interval;
+      }
+    }
+  }
+
+  gather_interval_in_seconds_ =
+      std::max(gather_interval_in_seconds_, kSampleInterval);
+
+  next_collection_time_ = base::Time::Now() +
+      base::TimeDelta::FromSeconds(gather_interval_in_seconds_);
 
-  g_started_initialization = true;
   util::PostTaskToDatabaseThreadAndReply(
       FROM_HERE,
       base::Bind(&PerformanceMonitor::InitOnBackgroundThread,
@@ -141,59 +176,42 @@ void PerformanceMonitor::Start() {
 
 void PerformanceMonitor::InitOnBackgroundThread() {
   CHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
-  database_ = Database::Create(database_path_);
-  if (!database_) {
-    LOG(ERROR) << "Could not initialize database; aborting initialization.";
-    return;
-  }
 
-  // Initialize the io thread's performance data to the value in the database;
-  // if there isn't a recording in the database, the value stays at 0.
-  Metric metric;
-  if (database_->GetRecentStatsForActivityAndMetric(METRIC_NETWORK_BYTES_READ,
-                                                    &metric)) {
-    performance_data_for_io_thread_.network_bytes_read = metric.value;
+  if (database_logging_enabled_) {
+    database_ = Database::Create(database_path_);
+    if (!database_) {
+      LOG(ERROR) << "Could not initialize database; aborting initialization.";
+      database_logging_enabled_ = false;
+      return;
+    }
+
+    // Initialize the io thread's performance data to the value in the database;
+    // if there isn't a recording in the database, the value stays at 0.
+    Metric metric;
+    if (database_->GetRecentStatsForActivityAndMetric(METRIC_NETWORK_BYTES_READ,
+                                                      &metric)) {
+      performance_data_for_io_thread_.network_bytes_read = metric.value;
+    }
   }
 }
 
 void PerformanceMonitor::FinishInit() {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
-  // Short-circuit the initialization process if the database wasn't properly
-  // created. This will prevent PerformanceMonitor from performing any actions,
-  // including observing events.
-  if (!database_)
-    return;
-
-  RegisterForNotifications();
-  CheckForUncleanExits();
-  BrowserThread::PostBlockingPoolSequencedTask(
-      Database::kDatabaseSequenceToken,
-      FROM_HERE,
-      base::Bind(&PerformanceMonitor::CheckForVersionUpdateOnBackgroundThread,
-                 base::Unretained(this)));
 
-  int gather_interval_in_seconds = kDefaultGatherIntervalInSeconds;
-  if (CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kPerformanceMonitorGathering) &&
-      !CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-          switches::kPerformanceMonitorGathering).empty()) {
-    int specified_interval = 0;
-    if (!base::StringToInt(
-            CommandLine::ForCurrentProcess()->GetSwitchValueASCII(
-                switches::kPerformanceMonitorGathering),
-            &specified_interval) || specified_interval <= 0) {
-      LOG(ERROR) << "Invalid value for switch: '"
-                 << switches::kPerformanceMonitorGathering
-                 << "'; please use an integer greater than 0.";
-    } else {
-      gather_interval_in_seconds = specified_interval;
-    }
+  // Events and notifications are only useful if we're logging to the database.
+  if (database_logging_enabled_) {
+    RegisterForNotifications();
+    CheckForUncleanExits();
+    BrowserThread::PostBlockingPoolSequencedTask(
+        Database::kDatabaseSequenceToken,
+        FROM_HERE,
+        base::Bind(&PerformanceMonitor::CheckForVersionUpdateOnBackgroundThread,
+                   base::Unretained(this)));
   }
 
-  timer_.Start(FROM_HERE,
-               base::TimeDelta::FromSeconds(gather_interval_in_seconds),
-               this,
-               &PerformanceMonitor::DoTimedCollections);
+  // Start our periodic gathering of metrics.
+  if (!disable_timer_autostart_)
+    timer_.Reset();
 
   // Post a task to the background thread to a function which does nothing.
   // This will force any tasks the database is performing to finish prior to
@@ -210,6 +228,8 @@ void PerformanceMonitor::FinishInit() {
 }
 
 void PerformanceMonitor::RegisterForNotifications() {
+  DCHECK(database_logging_enabled_);
+
   // Extensions
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_INSTALLED,
       content::NotificationService::AllSources());
@@ -241,6 +261,8 @@ void PerformanceMonitor::RegisterForNotifications() {
 // loaded prior to PerformanceMonitor's initialization. Later profiles will be
 // checked through the PROFILE_ADDED notification.
 void PerformanceMonitor::CheckForUncleanExits() {
+  DCHECK(database_logging_enabled_);
+
   std::vector<Profile*> profiles =
       g_browser_process->profile_manager()->GetLoadedProfiles();
 
@@ -258,7 +280,9 @@ void PerformanceMonitor::CheckForUncleanExits() {
 }
 
 void PerformanceMonitor::AddUncleanExitEventOnBackgroundThread(
-  const std::string& profile_name) {
+    const std::string& profile_name) {
+

[Devlin, 2013/09/12 18:20:42]

no newline

[oystein (OOO til 10th of July), 2013/09/12 19:03:24]

Done.

+  DCHECK(database_logging_enabled_);
   std::string database_key = kStateProfilePrefix + profile_name;
   std::string last_active_string = database_->GetStateValue(database_key);
 
@@ -279,6 +303,7 @@ void PerformanceMonitor::AddUncleanExitEventOnBackgroundThread(
 
 void PerformanceMonitor::CheckForVersionUpdateOnBackgroundThread() {
   CHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(database_logging_enabled_);
 
   chrome::VersionInfo version;
   DCHECK(version.is_valid());
@@ -305,6 +330,8 @@ void PerformanceMonitor::CheckForVersionUpdateOnBackgroundThread() {
 }
 
 void PerformanceMonitor::AddEvent(scoped_ptr<Event> event) {
+  DCHECK(database_logging_enabled_);
+
   BrowserThread::PostBlockingPoolSequencedTask(
       Database::kDatabaseSequenceToken,
       FROM_HERE,
@@ -319,6 +346,8 @@ void PerformanceMonitor::AddEventOnBackgroundThread(scoped_ptr<Event> event) {
 
 void PerformanceMonitor::AddMetricOnBackgroundThread(const Metric& metric) {
   DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(database_logging_enabled_);
+
   database_->AddMetric(metric);
 }
 
@@ -331,88 +360,63 @@ void PerformanceMonitor::NotifyInitialized() {
   initialized_ = true;
 }
 
-void PerformanceMonitor::GatherStatisticsOnBackgroundThread() {
-  CHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
+void PerformanceMonitor::DoTimedCollections() {
+#if !defined(OS_ANDROID)
+  // The profile list is only useful for the logged events.
+  if (database_logging_enabled_)
+    UpdateLiveProfiles();
+#endif
 
-  // Because the CPU usage is gathered as an average since the last time the
-  // function was called, while the memory usage is gathered as an instantaneous
-  // usage, the CPU usage is gathered before the metrics map is wiped.
-  GatherCPUUsageOnBackgroundThread();
-  UpdateMetricsMapOnBackgroundThread();
-  GatherMemoryUsageOnBackgroundThread();
+  GatherMetricsMapOnUIThread();
 }
 
-void PerformanceMonitor::GatherCPUUsageOnBackgroundThread() {
-  if (metrics_map_->size()) {
-    double cpu_usage = 0;
-    for (MetricsMap::const_iterator iter = metrics_map_->begin();
-         iter != metrics_map_->end(); ++iter) {
-      cpu_usage += iter->second->GetCPUUsage();
-    }
-
-    database_->AddMetric(Metric(METRIC_CPU_USAGE,
-                                base::Time::Now(),
-                                cpu_usage));
-  }
-}
-
-void PerformanceMonitor::GatherMemoryUsageOnBackgroundThread() {
-  size_t private_memory_sum = 0;
-  size_t shared_memory_sum = 0;
-  for (MetricsMap::const_iterator iter = metrics_map_->begin();
-       iter != metrics_map_->end(); ++iter) {
-    size_t private_memory = 0;
-    size_t shared_memory = 0;
-    if (iter->second->GetMemoryBytes(&private_memory, &shared_memory)) {
-      private_memory_sum += private_memory;
-      shared_memory_sum += shared_memory;
-    } else {
-      LOG(WARNING) << "GetMemoryBytes returned NULL (platform-specific error)";
-    }
+void PerformanceMonitor::GatherMetricsMapOnUIThread() {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  static int current_update_sequence = 0;

[Devlin, 2013/09/12 18:20:42]

Prefer a member variable over static int for something like this.

[oystein (OOO til 10th of July), 2013/09/12 19:03:24]

The idea is that the sequence ID should be carried along in the chain of jobs as
an argument rather than being encoded in object state, as an added safeguard in
case of concurrent runs (less shared state between threads). Moving this to a
member var increases the likelihood of the member being used by the
background/IO threads rather than the argument.

[Devlin, 2013/09/13 18:14:09]

Is this really a concern, though?  GatherMetricsMapOnIOThread() is only called
from GatherMetricsMapOnUIThread(), and we don't reset the timer until that task
is completed, right?  So aren't we guaranteed that only one of these will be
running at a time (either GatherMetricsOnUI or GatherMetricsOnIO)?

If not, you have a much bigger problem than a boolean variable - metrics_map_ is
referenced in both. :)

[oystein (OOO til 10th of July), 2013/09/13 20:41:55]

Oh I'll readily admit it's mainly an artifact from an earlier version of the
code which was still using RepeatingTimer rather than manually starting a
DelayTimer, but as moving this to a member variable gives no real benefit (4
bytes size increase in PerformanceMonitor vs 4 bytes extra per temporary job)
and *does* make the code less robust, however slightly, it doesn't seem sensible
to change..?

[Devlin, 2013/09/13 21:38:08]

It wouldn't be for performance benefits - the 4 bytes it saves by having only
one copy (in PM) vs a permanent and a temporary copy are inconsequential
overall.  It's mostly just because the variable is used like a member variable -
it's referenced in multiple methods of the object, and persists across method
calls.  But if you feel strongly about it, it's fine to stay as-is.

[oystein (OOO til 10th of July), 2013/09/13 21:57:50]

Mainly it's because the value is a property of the update sequence and not the
PerformanceMonitor instance, hence it makes sense to follow the job chain. This
also helps signal that only the original function should/can modify the
generator of the sequence (the variable itself).

+  // Even in the "somewhat" unlikely event this wraps around,
+  // it doesn't matter. We just check it for inequality.
+  current_update_sequence++;
+
+  // Find all render child processes; has to be done on the UI thread.
+  for (content::RenderProcessHost::iterator rph_iter =
+           content::RenderProcessHost::AllHostsIterator();
+       !rph_iter.IsAtEnd(); rph_iter.Advance()) {
+

[Devlin, 2013/09/12 18:20:42]

no newline

[oystein (OOO til 10th of July), 2013/09/12 19:03:24]

Done.

+    base::ProcessHandle handle = rph_iter.GetCurrentValue()->GetHandle();
+    ProcessIsAlive(
+        handle, content::PROCESS_TYPE_RENDERER, current_update_sequence);
   }
 
-  database_->AddMetric(Metric(METRIC_PRIVATE_MEMORY_USAGE,
-                              base::Time::Now(),
-                              static_cast<double>(private_memory_sum)));
-  database_->AddMetric(Metric(METRIC_SHARED_MEMORY_USAGE,
-                              base::Time::Now(),
-                              static_cast<double>(shared_memory_sum)));
-}
-
-void PerformanceMonitor::UpdateMetricsMapOnBackgroundThread() {
-  MetricsMap* new_map = new MetricsMap;
-
-  base::ProcessId browser_pid = base::GetCurrentProcId();
-  ChromeProcessList chrome_processes(GetRunningChromeProcesses(browser_pid));
-  for (ChromeProcessList::const_iterator pid_iter = chrome_processes.begin();
-       pid_iter != chrome_processes.end(); ++pid_iter) {
-    base::ProcessHandle handle;
-    if (base::OpenProcessHandleWithAccess(*pid_iter, kAccessFlags, &handle)) {
-      // If we were already watching this process, transfer it to the new map.
-      if (ContainsKey(*metrics_map_, handle)) {
-        (*new_map)[handle] = (*metrics_map_)[handle];
-        continue;
-      }
-
-      // Otherwise, gather information and prime the CPU usage to be gathered.
-#if defined (OS_MACOSX)
-      linked_ptr<base::ProcessMetrics> process_metrics(
-          base::ProcessMetrics::CreateProcessMetrics(
-              handle, content::BrowserChildProcessHost::GetPortProvider()));
-#else
-      linked_ptr<base::ProcessMetrics> process_metrics(
-          base::ProcessMetrics::CreateProcessMetrics(handle));
-#endif
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&PerformanceMonitor::GatherMetricsMapOnIOThread,
+                 base::Unretained(this),
+                 current_update_sequence));
+}
 
-      process_metrics->GetCPUUsage();
+void PerformanceMonitor::ProcessIsAlive(const base::ProcessHandle& handle,
+                                        int process_type,
+                                        int current_update_sequence) {
 
-      (*new_map)[handle] = process_metrics;
-    }
+  if (handle == 0) {
+    // Process may not be valid yet.
+    return;
   }
 
-  metrics_map_.reset(new_map);
+  MetricsMap::iterator process_metrics_iter = metrics_map_.find(handle);
+  if (process_metrics_iter == metrics_map_.end()) {
+    // If we're not already watching the process, let's initialize it.
+    metrics_map_[handle]
+        .Initialize(handle, process_type, current_update_sequence);
+  } else {
+    // If we are watching the process, touch it to keep it alive.
+    ProcessMetricsHistory& process_metrics = process_metrics_iter->second;
+    process_metrics.set_last_update_sequence(current_update_sequence);
+  }
 }
 
+#if !defined(OS_ANDROID)
 void PerformanceMonitor::UpdateLiveProfiles() {
   std::string time = TimeToString(base::Time::Now());
   scoped_ptr<std::set<std::string> > active_profiles(
@@ -434,48 +438,123 @@ void PerformanceMonitor::UpdateLiveProfilesHelper(
     scoped_ptr<std::set<std::string> > active_profiles,
     std::string time) {
   CHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(database_logging_enabled_);
 
   for (std::set<std::string>::const_iterator iter = active_profiles->begin();
        iter != active_profiles->end(); ++iter) {
     database_->AddStateValue(kStateProfilePrefix + *iter, time);
   }
 }
+#endif
 
-void PerformanceMonitor::DoTimedCollections() {
-  UpdateLiveProfiles();
-
-  BrowserThread::PostBlockingPoolSequencedTask(
-      Database::kDatabaseSequenceToken,
-      FROM_HERE,
-      base::Bind(&PerformanceMonitor::GatherStatisticsOnBackgroundThread,
-                 base::Unretained(this)));
+void PerformanceMonitor::GatherMetricsMapOnIOThread(
+    int current_update_sequence) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
-  BrowserThread::PostTask(
-      BrowserThread::IO,
-      FROM_HERE,
-      base::Bind(&PerformanceMonitor::CallInsertIOData,
-                 base::Unretained(this)));
-}
+  // Find all child processes (does not include renderers),
+  // which has to be done on the IO thread.
+  for (content::BrowserChildProcessHostIterator iter; !iter.Done(); ++iter) {
+    const content::ChildProcessData& child_process_data = iter.GetData();
+    base::ProcessHandle handle = child_process_data.handle;
+    ProcessIsAlive(
+        handle, child_process_data.process_type, current_update_sequence);
+  }
 
-void PerformanceMonitor::CallInsertIOData() {
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  // Add the current (browser) process.
+  ProcessIsAlive(base::GetCurrentProcessHandle(),
+                 content::PROCESS_TYPE_BROWSER,
+                 current_update_sequence);
 
   BrowserThread::PostBlockingPoolSequencedTask(
       Database::kDatabaseSequenceToken,
       FROM_HERE,
-      base::Bind(&PerformanceMonitor::InsertIOData,
+      base::Bind(&PerformanceMonitor::StoreMetricsOnBackgroundThread,
                  base::Unretained(this),
+                 current_update_sequence,
                  performance_data_for_io_thread_));
 }
 
-void PerformanceMonitor::InsertIOData(
+void PerformanceMonitor::StoreMetricsOnBackgroundThread(
+    int current_update_sequence,
     const PerformanceDataForIOThread& performance_data_for_io_thread) {
   CHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
-  database_->AddMetric(
-      Metric(METRIC_NETWORK_BYTES_READ,
-             base::Time::Now(),
-             static_cast<double>(
-                 performance_data_for_io_thread.network_bytes_read)));
+
+  // Update metrics for all watched processes; remove dead entries from the map.
+  MetricsMap::iterator iter = metrics_map_.begin();
+  while (iter != metrics_map_.end()) {

[Devlin, 2013/09/12 18:20:42]

Don't need two separate loops for this and determining metrics. They can easily
be combined.

[oystein (OOO til 10th of July), 2013/09/12 19:03:24]

This loop is run through on every iteration; the below loop isn't. You don't
want to make the common case more expensive (by adding a time comparison in the
inner loop) to optimize away the rare case (the below loop).

[Devlin, 2013/09/13 18:14:09]

  base::Time time_now = base::Time::Now();
  bool gather_metrics = time_now > next_collection_time_;
  double cpu_usage = 0.0;
  size_t private_memory_sum = 0;
  size_t shared_memory_sum = 0;

  for (MetricsMap::iterator iter = metrics_map_.begin();
       iter != metrics_map_.end(); /* no increment */) {
    // Update metrics for all watched processes; remove dead entries from
    // the map.
    ProcessMetricsHistory* process_metrics = &iter->second;
    if (process_metrics->last_update_sequence() != current_update_sequence) {
      // Not touched this iteration; let's get rid of it.
      metrics_map_.erase(iter++);
    } else {
      process_metrics.SampleMetrics();
      if (gather_metrics) {
        cpu_usage += process_metrics.GetAverageCPUUsage();

        size_t private_memory = 0;
        size_t shared_memory = 0;
        process_metrics.GetAverageMemoryBytes(&private_memory, &shared_memory);
        private_memory_sum += private_memory;
        shared_memory_sum += shared_memory;

        process_metrics->EndOfCycle();
      }
    }
  }

  if (gather_metrics && database_logging_enabled_) {
    database_->AddMetric(Metric(METRIC_CPU_USAGE, time_now, cpu_usage));
    database_->AddMetric(Metric(METRIC_PRIVATE_MEMORY_USAGE,
                                time_now,
                                static_cast<double>(private_memory_sum)));
    database_->AddMetric(Metric(METRIC_SHARED_MEMORY_USAGE,
                                time_now,
                                static_cast<double>(shared_memory_sum)));

    database_->AddMetric(
        Metric(METRIC_NETWORK_BYTES_READ,
               time_now,
               static_cast<double>(
                   performance_data_for_io_thread.network_bytes_read)));
  }

The increased complexity in the loop is equivalent to checking a boolean
expression (or practically 0) - far less than the cost of having a second loop,
even if it's only entered on occasion (not to mention the code simplification).

[oystein (OOO til 10th of July), 2013/09/13 20:41:55]

Right, I see what you mean now. Note that introducing a branch has a pretty big
impact on CPU instruction pipelining and is in no way "practically 0", but in
this case it's dwarfed by the std::map iteration anyway so it's fine.

+    ProcessMetricsHistory& process_metrics = iter->second;
+    if (process_metrics.last_update_sequence() != current_update_sequence) {
+      // Not touched this iteration; let's get rid of it.
+      metrics_map_.erase(iter++);
+    } else {
+      process_metrics.SampleMetrics();
+      ++iter;
+    }
+  }
+
+  base::Time time_now = base::Time::Now();
+
+  // Perform a collection of previously-sampled metrics.
+  // The timing can be off by kSampleInterval during any one particular run,
+  // but will be correct over time.
+  if (time_now > next_collection_time_) {
+    next_collection_time_ +=
+        base::TimeDelta::FromSeconds(gather_interval_in_seconds_);
+
+    if (!metrics_map_.empty()) {
+      double cpu_usage = 0.0;
+      size_t private_memory_sum = 0;
+      size_t shared_memory_sum = 0;
+
+      for (MetricsMap::iterator iter = metrics_map_.begin();
+           iter != metrics_map_.end(); ++iter) {
+

[Devlin, 2013/09/12 18:20:42]

no newline

[oystein (OOO til 10th of July), 2013/09/12 19:03:24]

Done.

+        ProcessMetricsHistory& process_metrics = iter->second;
+        cpu_usage += process_metrics.GetAverageCPUUsage();
+
+        size_t private_memory = 0;
+        size_t shared_memory = 0;
+        process_metrics.GetAverageMemoryBytes(&private_memory, &shared_memory);
+        private_memory_sum += private_memory;
+        shared_memory_sum += shared_memory;
+
+        process_metrics.EndOfCycle();
+      }
+
+      if (database_logging_enabled_) {
+        database_->AddMetric(Metric(METRIC_CPU_USAGE, time_now, cpu_usage));
+        database_->AddMetric(Metric(METRIC_PRIVATE_MEMORY_USAGE,
+                                    time_now,
+                                    static_cast<double>(private_memory_sum)));
+        database_->AddMetric(Metric(METRIC_SHARED_MEMORY_USAGE,
+                                    time_now,
+                                    static_cast<double>(shared_memory_sum)));
+      }
+    }
+
+    if (database_logging_enabled_) {
+      database_->AddMetric(
+          Metric(METRIC_NETWORK_BYTES_READ,
+                 time_now,
+                 static_cast<double>(
+                     performance_data_for_io_thread.network_bytes_read)));
+    }
+  }
+
+  BrowserThread::PostTask(
+      BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(&PerformanceMonitor::ResetMetricsTimerOnUIThread,
+                 base::Unretained(this)));
+}
+
+void PerformanceMonitor::ResetMetricsTimerOnUIThread() {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  // Start up the timer again; we avoid the use of a repeating timer
+  // to not have concurrent metrics gathering running.
+  if (!disable_timer_autostart_)
+    timer_.Reset();
 }
 
 void PerformanceMonitor::BytesReadOnIOThread(const net::URLRequest& request,
@@ -489,6 +568,8 @@ void PerformanceMonitor::BytesReadOnIOThread(const net::URLRequest& request,
 void PerformanceMonitor::Observe(int type,
                                  const content::NotificationSource& source,
                                  const content::NotificationDetails& details) {
+  DCHECK(database_logging_enabled_);
+
   switch (type) {
     case chrome::NOTIFICATION_EXTENSION_INSTALLED: {
       AddExtensionEvent(