PerformanceMonitor: UMA alert for high browser CPU usage.

chrome/browser/performance_monitor/performance_monitor.h

Index: chrome/browser/performance_monitor/performance_monitor.h
diff --git a/chrome/browser/performance_monitor/performance_monitor.h b/chrome/browser/performance_monitor/performance_monitor.h
index a054f5832914769e2ba4ca671567e9f21e1d7e9a..c60baf986ea3caa819d8aa588bb4e357830f89aa 100644
--- a/chrome/browser/performance_monitor/performance_monitor.h
+++ b/chrome/browser/performance_monitor/performance_monitor.h
@@ -6,23 +6,24 @@
 #define CHROME_BROWSER_PERFORMANCE_MONITOR_PERFORMANCE_MONITOR_H_
 
 #include <map>
+#include <set>
 #include <string>
 
-#include "base/callback.h"
-#include "base/files/file_path.h"

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

still need this one.

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

Done

-#include "base/memory/linked_ptr.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/memory/singleton.h"
-#include "base/process/process_metrics.h"
-#include "base/timer/timer.h"
-#include "chrome/browser/performance_monitor/database.h"
-#include "chrome/browser/performance_monitor/event.h"
-#include "content/public/browser/notification_details.h"
+#include "base/process/process_handle.h"
+#include "chrome/browser/performance_monitor/event_type.h"
+#include "chrome/browser/performance_monitor/process_metrics_history.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
-#include "content/public/browser/notification_source.h"
 #include "content/public/browser/render_process_host.h"
 
+template <typename Type>
+struct DefaultSingletonTraits;
+
+namespace content {

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

you #include this, and have an instance in PerformanceMonitor. No need to
forward declare the class.

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

Done.

+class NotificationRegistrar;
+}
+
 namespace extensions {
 class Extension;
 }
@@ -33,6 +34,8 @@ class URLRequest;
 
 namespace performance_monitor {
 class Database;
+class Event;
+struct Metric;
 
 // PerformanceMonitor is a tool which will allow the user to view information
 // about Chrome's performance over a period of time. It will gather statistics
@@ -56,8 +59,7 @@ class PerformanceMonitor : public content::NotificationObserver {
     uint64 network_bytes_read;
   };
 
-  typedef std::map<base::ProcessHandle,
-                   linked_ptr<base::ProcessMetrics> > MetricsMap;
+  typedef std::map<base::ProcessHandle, ProcessMetricsHistory> MetricsMap;
 
   // Set the path which the PerformanceMonitor should use for the database files
   // constructed. This must be done prior to the initialization of the
@@ -66,6 +68,8 @@ class PerformanceMonitor : public content::NotificationObserver {
   // time of the call).
   bool SetDatabasePath(const base::FilePath& path);
 
+  bool database_logging_enabled() const { return database_logging_enabled_; }
+
   // Returns the current PerformanceMonitor instance if one exists; otherwise
   // constructs a new PerformanceMonitor.
   static PerformanceMonitor* GetInstance();
@@ -141,22 +145,30 @@ class PerformanceMonitor : public content::NotificationObserver {
 
   // Update the database record of the last time the active profiles were
   // running; this is used in determining when an unclean exit occurred.
+#if !defined(OS_ANDROID)
   void UpdateLiveProfiles();
   void UpdateLiveProfilesHelper(
       scoped_ptr<std::set<std::string> > active_profiles, std::string time);
+#endif
 
-  // Gathers CPU usage and memory usage of all Chrome processes in order to.
-  void GatherStatisticsOnBackgroundThread();
+  // Stores CPU/memory usage metrics to the database.
+  void StoreMetricsOnBackgroundThread(
+      int current_update_sequence,
+      const PerformanceDataForIOThread& performance_data_for_io_thread);
 
-  // Gathers the CPU usage of every Chrome process that has been running since
-  // the last call to GatherStatistics().
-  void GatherCPUUsageOnBackgroundThread();
+  // Mark the given process as alive in the current update iteration.
+  void ProcessIsAlive(const base::ProcessHandle& handle,

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

Please mention in the comment that this will begin tracking the process if we
are not already.

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

Why does that matter in terms of the API?

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

To me, it's otherwise unclear when we may or may not add any new processes to
the map.  Anyone looking at the code itself will understand, but we generally
try to practice having descriptive method names that accurately describe what we
are doing.  While external callers won't care, that could be said of any private
method.

[Yoyo Zhou, 2013/09/18 23:01:42]

As a comment on a private method, you're not just documenting for people who
want to know how to use it (the API expectations), but for people who may want
to understand or modify this code.

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

Done.

+                      int process_type,
+                      int current_update_sequence);
 
-  // Gathers the memory usage of every process in the current list of processes.
-  void GatherMemoryUsageOnBackgroundThread();
+  // Updates the ProcessMetrics map with the current list of processes and
+  // gathers metrics from each entry.
+  void GatherMetricsMapOnUIThread();
+  void GatherMetricsMapOnIOThread(int current_update_sequence);
 
-  // Updates the ProcessMetrics map with the current list of processes.
-  void UpdateMetricsMapOnBackgroundThread();
+  // Called at the end of the metrics gathering process, to start
+  // our timer for the next run.
+  void ResetMetricsTimerOnUIThread();
 
   // Generate an appropriate ExtensionEvent for an extension-related occurrance
   // and insert it in the database.
@@ -169,15 +181,6 @@ class PerformanceMonitor : public content::NotificationObserver {
       content::RenderProcessHost* host,
       const content::RenderProcessHost::RendererClosedDetails& details);
 
-  // Called on the IO thread, this will call InsertIOData on the background
-  // thread with a copy of the PerformanceDataForIOThread object to prevent
-  // any possible race conditions.
-  void CallInsertIOData();
-
-  // Insert the collected IO data into the database.
-  void InsertIOData(
-      const PerformanceDataForIOThread& performance_data_for_io_thread);
-
   // The store for all performance data that must be gathered from the IO
   // thread.
   PerformanceDataForIOThread performance_data_for_io_thread_;
@@ -189,10 +192,20 @@ class PerformanceMonitor : public content::NotificationObserver {
   scoped_ptr<Database> database_;
 
   // A map of currently running ProcessHandles to ProcessMetrics.
-  scoped_ptr<MetricsMap> metrics_map_;
+  MetricsMap metrics_map_;
+
+  // The next time we should collect averages from the performance metrics
+  // and act on them.
+  base::Time next_collection_time_;

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

#include "base/time/time.h"

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

Done.

+
+  // How long to wait between collections.
+  int gather_interval_in_seconds_;
+
+  // Enable persistent logging of performance metrics to a database.
+  bool database_logging_enabled_;
 
   // The timer to signal PerformanceMonitor to perform its timed collections.
-  base::RepeatingTimer<PerformanceMonitor> timer_;
+  base::DelayTimer<PerformanceMonitor> timer_;

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

#include "base/timer/timer.h"

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

This really just feels like unnecessary clutter, but sure.

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

Matter of opinion, but yeah. In some cases, I happen to agree (for instance, I
think that scoped_ptr or <string> is all but standard, and should be exempt). 
But we have an agreed-upon style, which says we "include what we use".  This
makes refactors that kill transitive includes much easier, and makes it clear
which files use what.

 
   content::NotificationRegistrar registrar_;
 
@@ -202,6 +215,9 @@ class PerformanceMonitor : public content::NotificationObserver {
   // flag.
   static bool initialized_;
 
+  // Disable auto-starting the collection timer; used for tests.
+  bool disable_timer_autostart_;
+
   DISALLOW_COPY_AND_ASSIGN(PerformanceMonitor);
 };