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 5ed870f5ddeac0eeaedc10be167a7a6509dec9af..b7fad01d193c3ad53f8080ec777bfcfa9e9b36e0 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"
@@ -52,8 +54,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);
@@ -98,7 +98,14 @@ 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) {}

[Devlin, 2013/09/12 00:44:47]

nit: please place { and } on separate lines, e.g.
disable_timer_autostart_(false) {
}
I know this is different throughout the codebase, but I think in p_m, we do it
this way.

[oystein (OOO til 10th of July), 2013/09/12 15:02:31]

Done.

 
 PerformanceMonitor::~PerformanceMonitor() {
   BrowserThread::PostBlockingPoolSequencedTask(
@@ -125,11 +132,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);
+  has_initialized = true;
+
+  if (CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kPerformanceMonitorGathering)) {
+    database_logging_enabled_ = true;
+
+    if (!CommandLine::ForCurrentProcess()
+             ->GetSwitchValueASCII(switches::kPerformanceMonitorGathering)

[Devlin, 2013/09/12 00:44:47]

nit: though arbitrary, I've always seen -> on the preceding line. Please
conform. :)

[oystein (OOO til 10th of July), 2013/09/12 15:02:31]

Done.

+             .empty()) {
+      int specified_interval = 0;
+      if (!base::StringToInt(
+               CommandLine::ForCurrentProcess()->GetSwitchValueASCII(

[Devlin, 2013/09/12 00:44:47]

nit: operator ! does not count as a separate scope - i.e., indent should be:
if (!base::StringToInt(
        CommandLine::....

[oystein (OOO til 10th of July), 2013/09/12 15:02:31]

Done.

+                   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(

[Devlin, 2013/09/12 00:44:47]

nit: To me, this just looks odd (though I understand the reasoning). Prefer:
next_collection_time_ =
    base::Time::Now() +
    base::TimeDelta::FromSeconds(gather_interval_in_seconds_);
or
next_collection_time_ = base::Time::Now() +
    base::TimeDelta::FromSeconds(gather_interval_in_seconds_);

[oystein (OOO til 10th of July), 2013/09/12 15:02:31]

Done.

+                                                  gather_interval_in_seconds_);
 
-  g_started_initialization = true;
   util::PostTaskToDatabaseThreadAndReply(
       FROM_HERE,
       base::Bind(&PerformanceMonitor::InitOnBackgroundThread,
@@ -140,59 +174,44 @@ 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

[Devlin, 2013/09/12 00:44:47]

line wrapping is off.

[oystein (OOO til 10th of July), 2013/09/12 15:02:31]

Done.

+  // 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_) {

[Devlin, 2013/09/12 00:44:47]

nit: for single-line if-statements, prefer no brackets.

[oystein (OOO til 10th of July), 2013/09/12 15:02:31]

Done.

+    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
@@ -209,6 +228,8 @@ void PerformanceMonitor::FinishInit() {
 }
 
 void PerformanceMonitor::RegisterForNotifications() {
+  DCHECK(database_logging_enabled_);

[Devlin, 2013/09/12 00:44:47]

given that this is a private method called from one place, this seems redundant.
 Probably safe to remove. :)

[oystein (OOO til 10th of July), 2013/09/12 15:02:31]

Safe with the current code paths sure, but why remove it?

+
   // Extensions
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_INSTALLED,
       content::NotificationService::AllSources());
@@ -240,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_);

[Devlin, 2013/09/12 00:44:47]

same as above; probably safe to remove.

+
   std::vector<Profile*> profiles =
       g_browser_process->profile_manager()->GetLoadedProfiles();
 
@@ -257,7 +280,9 @@ void PerformanceMonitor::CheckForUncleanExits() {
 }
 
 void PerformanceMonitor::AddUncleanExitEventOnBackgroundThread(
-  const std::string& profile_name) {
+    const std::string& profile_name) {
+
+  DCHECK(database_logging_enabled_);

[Devlin, 2013/09/12 00:44:47]

If you want to have a check for database logging, perhaps it makes sense to have
a wrapper around database->Add[Metric|Event]()?  Most of these checks are
extraneous, but if you want to have some extra security, let's put it all in one
place.

[oystein (OOO til 10th of July), 2013/09/12 15:02:31]

Reorganizing release code to account for debug-only asserts feels a bit fishy.
I'll remove these if you want, but my general principle with regards to
debug-only asserts has always been "more is better".

   std::string database_key = kStateProfilePrefix + profile_name;
   std::string last_active_string = database_->GetStateValue(database_key);
 
@@ -278,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());
@@ -304,6 +330,8 @@ void PerformanceMonitor::CheckForVersionUpdateOnBackgroundThread() {
 }
 
 void PerformanceMonitor::AddEvent(scoped_ptr<Event> event) {
+  DCHECK(database_logging_enabled_);
+
   BrowserThread::PostBlockingPoolSequencedTask(
       Database::kDatabaseSequenceToken,
       FROM_HERE,
@@ -318,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);
 }
 
@@ -330,88 +360,64 @@ 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_) {

[Devlin, 2013/09/12 00:44:47]

nit: no brackets.

[oystein (OOO til 10th of July), 2013/09/12 15:02:31]

Sure, though out of curiosity: Is this an actual Chrome rule? The only guideline
I could find was "do whatever you prefer" (and I definitely prefer braces for
safety).

+    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();
-    }
+void PerformanceMonitor::GatherMetricsMapOnUIThread() {

[Devlin, 2013/09/12 00:44:47]

here, a DCHECK (or even CHECK) for On UI Thread would be good. *Really* bad
things happen if we're not, and thread checking is generally more prone to
mistakes than synchronous function calls.

[oystein (OOO til 10th of July), 2013/09/12 15:02:31]

Done.

+  static int current_update_sequence = 0;
+  // Even in the "somewhat" unlikely event this wraps around,
+  // it doesn't matter. We just check it for inequality.
+  current_update_sequence++;
 
-    database_->AddMetric(Metric(METRIC_CPU_USAGE,
-                                base::Time::Now(),
-                                cpu_usage));
-  }
-}
+  // Find all render child processes; has to be done on the UI thread.
+  for (content::RenderProcessHost::iterator i =

[Devlin, 2013/09/12 00:44:47]

i is very nondescriptive for a non-standard iterator like this. Prefer something
like rph_iter.

[oystein (OOO til 10th of July), 2013/09/12 15:02:31]

Done.

+           content::RenderProcessHost::AllHostsIterator();
+       !i.IsAtEnd();
+       i.Advance()) {

[Devlin, 2013/09/12 00:44:47]

nit: put this on the previous line.

[oystein (OOO til 10th of July), 2013/09/12 15:02:31]

Done.

 
-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)";
-    }
+    base::ProcessHandle handle = i.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)));
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&PerformanceMonitor::GatherMetricsMapOnIOThread,
+                 base::Unretained(this),
+                 current_update_sequence));
 }
 
-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
-
-      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;

[Devlin, 2013/09/12 00:44:47]

We don't typically use non-const references if we can help it. Prefer a pointer.

[oystein (OOO til 10th of July), 2013/09/12 15:02:31]

Demoting the reference to a pointer loses important type information, and adds
ambiguity. The rules regarding non-const references apply to function arguments,
not local scoped variables as far as I can see.

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

I don't feel real strongly about it, but I'm not sure I see why demoting it to a
pointer loses any type information.  Isn't this the same as:
ProcessMetricsHistory* process_metrics = &process_metrics_iter->second;
process_metrics->SetLastUpdateSequence(current_update_sequence);
?

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

What's lost is the constraint that the variable can't be NULL, basically. What
would you gain from demoting this to a pointer?

+    process_metrics.SetLastUpdateSequence(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(
@@ -433,48 +439,125 @@ 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),

[Devlin, 2013/09/12 00:44:47]

nit: line wrapping.

[oystein (OOO til 10th of July), 2013/09/12 15:02:31]

Not sure what you mean here :). The comment can't fit on one line.

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

If a comment doesn't fit on one line, it should line-wrap when it reaches the
end of the line, e.g. prefer:
xxxxxx
xx
over
xxxx
xxxx

Here:
// Find all child processes (does not include renderers), which has to be done
// on the IO thread.

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

Done.

+  // 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()) {
+    ProcessMetricsHistory& process_metrics = iter->second;
+    if (process_metrics.GetLastUpdateSequence() != 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();
+
+  // Time for a collection to be made, of previously sampled metrics.

[Devlin, 2013/09/12 00:44:47]

nit: "Make a collection of previously-sampled metrics." Move the "timing may be
off" part to the header file (in a comment for next_collection_time_).

[oystein (OOO til 10th of July), 2013/09/12 15:02:31]

Why the move? That seems to take information away from the only point where it's
relevant.

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

Again, no strong feelings, so if you think it makes more sense here that's fine.
But I think it's relevant enough to the variable (next_collection_time_) to
include in the header. It lets me know that |next_collection_time_| is not a
hard guarantee.

+  // 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 00:44:47]

nit: put this on the previous line.

+
+        ProcessMetricsHistory& process_metrics = iter->second;

[Devlin, 2013/09/12 00:44:47]

nit: avoid non-const references when possible.

+        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));

[Devlin, 2013/09/12 00:44:47]

This seems bad to me.  I don't think we should be doing this much work if we
don't have a database to log it.

[oystein (OOO til 10th of July), 2013/09/12 15:02:31]

EndOfCycle() always needs to be called. I can call it in a separate loop, away
from the metrics summing, but that will actually be less efficient (likely by an
order of magnitude) than the above approach due to the cache misses you incur
from iterating through an std::map.

+        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_) {

[Devlin, 2013/09/12 00:44:47]

nit: no brackets

[oystein (OOO til 10th of July), 2013/09/12 15:02:31]

Done.

+    timer_.Reset();
+  }
 }
 
 void PerformanceMonitor::BytesReadOnIOThread(const net::URLRequest& request,
@@ -488,6 +571,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(