PerformanceMonitor: UMA alert for high browser CPU usage.

chrome/browser/performance_monitor/performance_monitor.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 "chrome/browser/performance_monitor/performance_monitor.h"
 
 #include <set>
 #include <vector>
 
 #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"
 #include "base/threading/worker_pool.h"
 #include "base/time/time.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_shutdown.h"
 #include "chrome/browser/chrome_notification_types.h"
 #include "chrome/browser/extensions/crx_installer.h"
 #include "chrome/browser/performance_monitor/constants.h"
 #include "chrome/browser/performance_monitor/performance_monitor_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/browser_iterator.h"
 #include "chrome/common/chrome_switches.h"
 #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"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/render_widget_host.h"
 #include "content/public/browser/web_contents.h"
 #include "net/url_request/url_request.h"
 
 using content::BrowserThread;
 using extensions::Extension;
 
 namespace performance_monitor {
 
 namespace {
 
 const uint32 kAccessFlags = base::kProcessAccessDuplicateHandle |
                             base::kProcessAccessQueryInformation |
                             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);
 }
 
 bool StringToTime(std::string time, base::Time* output) {
   int64 time_int64 = 0;
   if (!base::StringToInt64(time, &time_int64))
     return false;
   *output = base::Time::FromInternalValue(time_int64);
@@ skipping 24 matching lines @@
 }
 
 }  // namespace
 
 bool PerformanceMonitor::initialized_ = false;
 
 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(
       Database::kDatabaseSequenceToken,
       FROM_HERE,
       base::Bind(&DeleteDatabaseOnBackgroundThread, database_.release()));
 }
 
 bool PerformanceMonitor::SetDatabasePath(const base::FilePath& path) {
   if (!database_.get()) {
     database_path_ = path;
     return true;
   }
 
   // PerformanceMonitor already initialized with another path.
   return false;
 }
 
 // static
 PerformanceMonitor* PerformanceMonitor::GetInstance() {
   return Singleton<PerformanceMonitor>::get();
 }
 
 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;
 
-  g_started_initialization = 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_);
+
   util::PostTaskToDatabaseThreadAndReply(
       FROM_HERE,
       base::Bind(&PerformanceMonitor::InitOnBackgroundThread,
                  base::Unretained(this)),
       base::Bind(&PerformanceMonitor::FinishInit,
                  base::Unretained(this)));
 }
 
 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
   // the reply being sent, since they use the same thread.
   //
   // Important! Make sure that methods in FinishInit() only rely on posting
   // to the background thread, and do not rely upon a reply from the background
   // thread; this is necessary for this notification to be valid.
   util::PostTaskToDatabaseThreadAndReply(
       FROM_HERE,
       base::Bind(&base::DoNothing),
       base::Bind(&PerformanceMonitor::NotifyInitialized,
                  base::Unretained(this)));
 }
 
 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());
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_ENABLED,
       content::NotificationService::AllSources());
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNLOADED,
       content::NotificationService::AllSources());
   registrar_.Add(this, chrome::NOTIFICATION_CRX_INSTALLER_DONE,
       content::NotificationService::AllSources());
   registrar_.Add(this, chrome::NOTIFICATION_EXTENSION_UNINSTALLED,
@@ skipping 11 matching lines @@
 
   // Page load times
   registrar_.Add(this, content::NOTIFICATION_LOAD_STOP,
       content::NotificationService::AllSources());
 }
 
 // We check if profiles exited cleanly initialization time in case they were
 // 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();
 
   for (std::vector<Profile*>::const_iterator iter = profiles.begin();
        iter != profiles.end(); ++iter) {
     if ((*iter)->GetLastSessionExitType() == Profile::EXIT_CRASHED) {
       BrowserThread::PostBlockingPoolSequencedTask(
           Database::kDatabaseSequenceToken,
           FROM_HERE,
           base::Bind(&PerformanceMonitor::AddUncleanExitEventOnBackgroundThread,
                      base::Unretained(this),
                      (*iter)->GetDebugName()));
     }
   }
 }
 
 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);
 
   // Check if there was no previous time; this should only happen if the profile
   // was last used prior to PerformanceMonitor's integration. Do nothing in this
   // case, since the event was prior to the beginning of our recording.
   if (last_active_string.empty())
     return;
 
   base::Time last_active_time;
   CHECK(StringToTime(last_active_string, &last_active_time));
 
   scoped_ptr<Event> event =
       util::CreateUncleanExitEvent(last_active_time, profile_name);
 
   database_->AddEvent(*event.get());
 }
 
 void PerformanceMonitor::CheckForVersionUpdateOnBackgroundThread() {
   CHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(database_logging_enabled_);
 
   chrome::VersionInfo version;
   DCHECK(version.is_valid());
   std::string current_version = version.Version();
 
   std::string previous_version = database_->GetStateValue(kStateChromeVersion);
 
   // We should never have a current_version which is older than the
   // previous_version.
   DCHECK(current_version >= previous_version);
 
   // If this is the first run, there will not be a stored value for Chrome
   // version; we insert the current version and will insert an event for the
   // next update of Chrome. If the previous version is older than the current
   // version, update the state in the database and insert an event.
   if (current_version > previous_version) {
     database_->AddStateValue(kStateChromeVersion, current_version);
     if (!previous_version.empty()) {
       scoped_ptr<Event> event = util::CreateChromeUpdateEvent(
           base::Time::Now(), previous_version, current_version);
       database_->AddEvent(*event.get());
     }
   }
 }
 
 void PerformanceMonitor::AddEvent(scoped_ptr<Event> event) {
+  DCHECK(database_logging_enabled_);
+
   BrowserThread::PostBlockingPoolSequencedTask(
       Database::kDatabaseSequenceToken,
       FROM_HERE,
       base::Bind(&PerformanceMonitor::AddEventOnBackgroundThread,
                  base::Unretained(this),
                  base::Passed(&event)));
 }
 
 void PerformanceMonitor::AddEventOnBackgroundThread(scoped_ptr<Event> event) {
   database_->AddEvent(*event.get());
 }
 
 void PerformanceMonitor::AddMetricOnBackgroundThread(const Metric& metric) {
   DCHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
+  DCHECK(database_logging_enabled_);
+
   database_->AddMetric(metric);
 }
 
 void PerformanceMonitor::NotifyInitialized() {
   content::NotificationService::current()->Notify(
       chrome::NOTIFICATION_PERFORMANCE_MONITOR_INITIALIZED,
       content::Source<PerformanceMonitor>(this),
       content::NotificationService::NoDetails());
 
   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.

+
+    base::ProcessHandle handle = i.GetCurrentValue()->GetHandle();
+    ProcessIsAlive(
+        handle, content::PROCESS_TYPE_RENDERER, current_update_sequence);
+  }
+
+  BrowserThread::PostTask(
+      BrowserThread::IO,
+      FROM_HERE,
+      base::Bind(&PerformanceMonitor::GatherMetricsMapOnIOThread,
+                 base::Unretained(this),
+                 current_update_sequence));
+}
+
+void PerformanceMonitor::ProcessIsAlive(const base::ProcessHandle& handle,
+                                        int process_type,
+                                        int current_update_sequence) {
+
+  if (handle == 0) {
+    // Process may not be valid yet.
+    return;
+  }
+
+  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);
   }
 }
 
-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)";
-    }
-  }
-
-  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
-
-      process_metrics->GetCPUUsage();
-
-      (*new_map)[handle] = process_metrics;
-    }
-  }
-
-  metrics_map_.reset(new_map);
-}
-
+#if !defined(OS_ANDROID)
 void PerformanceMonitor::UpdateLiveProfiles() {
   std::string time = TimeToString(base::Time::Now());
   scoped_ptr<std::set<std::string> > active_profiles(
       new std::set<std::string>());
 
   for (chrome::BrowserIterator it; !it.done(); it.Next())
     active_profiles->insert(it->profile()->GetDebugName());
 
   BrowserThread::PostBlockingPoolSequencedTask(
       Database::kDatabaseSequenceToken,
       FROM_HERE,
       base::Bind(&PerformanceMonitor::UpdateLiveProfilesHelper,
                  base::Unretained(this),
                  base::Passed(&active_profiles),
                  time));
 }
 
 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();
+void PerformanceMonitor::GatherMetricsMapOnIOThread(
+    int current_update_sequence) {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  // 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);
+  }
+
+  // Add the current (browser) process.
+  ProcessIsAlive(base::GetCurrentProcessHandle(),
+                 content::PROCESS_TYPE_BROWSER,
+                 current_update_sequence);
 
   BrowserThread::PostBlockingPoolSequencedTask(
       Database::kDatabaseSequenceToken,
       FROM_HERE,
-      base::Bind(&PerformanceMonitor::GatherStatisticsOnBackgroundThread,
-                 base::Unretained(this)));
+      base::Bind(&PerformanceMonitor::StoreMetricsOnBackgroundThread,
+                 base::Unretained(this),
+                 current_update_sequence,
+                 performance_data_for_io_thread_));
+}
+
+void PerformanceMonitor::StoreMetricsOnBackgroundThread(
+    int current_update_sequence,
+    const PerformanceDataForIOThread& performance_data_for_io_thread) {
+  CHECK(!BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  // 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::IO,
+      BrowserThread::UI,
       FROM_HERE,
-      base::Bind(&PerformanceMonitor::CallInsertIOData,
+      base::Bind(&PerformanceMonitor::ResetMetricsTimerOnUIThread,
                  base::Unretained(this)));
 }
 
-void PerformanceMonitor::CallInsertIOData() {
-  CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+void PerformanceMonitor::ResetMetricsTimerOnUIThread() {
+  CHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  BrowserThread::PostBlockingPoolSequencedTask(
-      Database::kDatabaseSequenceToken,
-      FROM_HERE,
-      base::Bind(&PerformanceMonitor::InsertIOData,
-                 base::Unretained(this),
-                 performance_data_for_io_thread_));
-}
-
-void PerformanceMonitor::InsertIOData(
-    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)));
+  // 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,
                                              const int bytes_read) {
   CHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   if (initialized_ && !request.url().SchemeIsFile())
     performance_data_for_io_thread_.network_bytes_read += bytes_read;
 }
 
 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(
           EVENT_EXTENSION_INSTALL,
           content::Details<const extensions::InstalledExtensionInfo>(details)->
               extension);
       break;
     }
     case chrome::NOTIFICATION_EXTENSION_ENABLED: {
       AddExtensionEvent(EVENT_EXTENSION_ENABLE,
@@ skipping 137 matching lines @@
     if (!url_list.empty())
       url_list += ", ";
 
     url_list += url;
   }
 
   AddEvent(util::CreateRendererFailureEvent(base::Time::Now(), type, url_list));
 }
 
 }  // namespace performance_monitor