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/render_widget_host_iterator.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) {
+}
 
 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);

[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;
 
-  g_started_initialization = 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_);
+
   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 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
   // 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_);
+
   // 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_);
+
   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) {
+

[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);
 
   // 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_)
+    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() {
+  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++;
 
-    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 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);
+  }
+
+  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;
+    process_metrics.set_last_update_sequence(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),
+  // 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()) {

[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::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_)
+    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