Add BytesRead metric to CPM

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 <string>
 
@@ skipping 13 matching lines @@
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/render_process_host.h"
 
 namespace extensions {
 class Extension;
 }
 
 namespace performance_monitor {
 class Database;
 
 class PerformanceMonitor : public content::NotificationObserver {

[battre, 2012/08/21 16:16:04]

Can you document the thread-safety of this class?

[Devlin, 2012/08/21 19:46:46]

Done (and very needed, thank you). Please let me know if this makes sense, or if
I should be more descriptive about any part.

  public:
   typedef std::map<base::ProcessHandle,
                    linked_ptr<base::ProcessMetrics> > 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 FilePath& path);
 
   // 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; increment the count
+  // accordingly.
+  void BytesRead(int bytes);
+
   // content::NotificationObserver
   // Wait for various notifications; insert events into the database upon
   // occurance.
   virtual void Observe(int type,
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
   Database* database() { return database_.get(); }
   FilePath database_path() { return database_path_; }
+  static bool enabled() { return enabled_; }
 
  private:
   friend struct DefaultSingletonTraits<PerformanceMonitor>;
   friend class PerformanceMonitorBrowserTest;
   FRIEND_TEST_ALL_PREFIXES(PerformanceMonitorUncleanExitBrowserTest,
                            OneProfileUncleanExit);
   FRIEND_TEST_ALL_PREFIXES(PerformanceMonitorUncleanExitBrowserTest,
                            TwoProfileUncleanExit);
+  FRIEND_TEST_ALL_PREFIXES(PerformanceMonitorBrowserTest, BytesRead);
 
   PerformanceMonitor();
   virtual ~PerformanceMonitor();
 
   // Perform any additional initialization which must be performed on a
   // background thread (e.g. constructing the database).
   void InitOnBackgroundThread();
 
   void FinishInit();
 
@@ skipping 50 matching lines @@
   // 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 CrashEvent for a renderer crash and insert it in
   // the database.
   void AddCrashEvent(
       const content::RenderProcessHost::RendererClosedDetails& details);
 
+  // A count of how many bytes have been read. This corresponds to the total
+  // number of bytes that have been read since PerformanceMonitor first started

[battre, 2012/08/21 16:16:04]

"have been read from where"? net/disk/...

[Devlin, 2012/08/21 19:46:46]

Good point. I found this through TaskManager's code, and didn't think to check
that the Network column would refer to multiple places (but it does). This has
now been separated into two metrics. Thanks :)

+  // recording, and persists across browser runs.
+  int64 bytes_read_;
+
   // The location at which the database files are stored; if empty, the database
   // will default to '<user_data_dir>/performance_monitor_dbs'.
   FilePath database_path_;
 
   scoped_ptr<Database> database_;
 
   // A map of currently running ProcessHandles to ProcessMetrics.
   MetricsMap metrics_map_;
 
   // The timer to signal PerformanceMonitor to perform its timed collections.
   base::RepeatingTimer<PerformanceMonitor> timer_;
 
   content::NotificationRegistrar registrar_;
 
+  // A flag indicating whether or not PerformanceMonitor is enabled. Any
+  // external sources accessing PerformanceMonitor should either wait for
+  // the PERFORMANCE_MONITOR_INITIALIZED notification or should check this
+  // flag.
+  static bool enabled_;

[battre, 2012/08/21 16:16:04]

static attributes don't use the _ notation but use a g_ prefix. See "static int
g_max_queueing_delay_ms = 0;" in predictor.cc. You would move the variable to
the .cc file.

[eaugusti, 2012/08/21 17:17:26]

Isn't the g_ prefix only for global data?

I think I have seen this pattern before:
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/time.h&exact_package=...
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/jankometer....

[Devlin, 2012/08/21 19:46:46]

Other examples:
http://code.google.com/searchframe#OAMlx_jo-ck/src/base/metrics/statistics_re...
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/browsing_da...
(examples where a static bool is returned in a accessor function)

+
   DISALLOW_COPY_AND_ASSIGN(PerformanceMonitor);
 };
 
 }  // namespace performance_monitor
 
 #endif  // CHROME_BROWSER_PERFORMANCE_MONITOR_PERFORMANCE_MONITOR_H_