PerformanceMonitor: UMA alert for high browser CPU usage.

chrome/browser/performance_monitor/performance_monitor.h

 // 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.
 
 #ifndef CHROME_BROWSER_PERFORMANCE_MONITOR_PERFORMANCE_MONITOR_H_
 #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;
 }
 
 namespace net {
 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
 // pertaining to performance-oriented areas (e.g. CPU usage, memory usage, and
 // network usage) and will also store information about significant events which
 // are related to performance, either being indicative (e.g. crashes) or
 // potentially causal (e.g. extension installation/uninstallation).
 //
 // Thread Safety: PerformanceMonitor lives on multiple threads. When interacting
 // with the Database, PerformanceMonitor acts on a background thread (which has
 // the sequence guaranteed by a token, Database::kDatabaseSequenceToken). At
 // other times, the PerformanceMonitor will act on the appropriate thread for
 // the task (for instance, gathering statistics about CPU and memory usage
 // is done on the background thread, but most notifications occur on the UI
 // thread).
 class PerformanceMonitor : public content::NotificationObserver {
  public:
   struct PerformanceDataForIOThread {
     PerformanceDataForIOThread();
 
     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
   // PerformanceMonitor. Returns true on success, false on failure (failure
   // likely indicates that PerformanceMonitor has already been started at the
   // 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();
 
   // Begins the initialization process for the PerformanceMonitor in order to
   // start collecting data.
   void Start();
 
   // Inform PerformanceMonitor that bytes have been read; if these came across
   // the network (and PerformanceMonitor is initialized), then increment the
@@ skipping 55 matching lines @@
   void AddMetricOnBackgroundThread(const Metric& metric);
 
   // Notify any listeners that PerformanceMonitor has finished the initializing.
   void NotifyInitialized();
 
   // Perform any collections that are done on a timed basis.
   void DoTimedCollections();
 
   // 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.
   void AddExtensionEvent(EventType type,
                          const extensions::Extension* extension);
 
   // Generate an appropriate RendererFailure for a renderer crash and insert it
   // in the database.
   void AddRendererClosedEvent(
       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_;
 
   // The location at which the database files are stored; if empty, the database
   // will default to '<user_data_dir>/performance_monitor_dbs'.
   base::FilePath database_path_;
 
   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_;
 
   // A flag indicating whether or not PerformanceMonitor is initialized. Any
   // external sources accessing PerformanceMonitor should either wait for
   // the PERFORMANCE_MONITOR_INITIALIZED notification or should check this
   // flag.
   static bool initialized_;
 
+  // Disable auto-starting the collection timer; used for tests.
+  bool disable_timer_autostart_;
+
   DISALLOW_COPY_AND_ASSIGN(PerformanceMonitor);
 };
 
 }  // namespace performance_monitor
 
 #endif  // CHROME_BROWSER_PERFORMANCE_MONITOR_PERFORMANCE_MONITOR_H_