Add BytesRead metric to CPM

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 b593ad943657423326db169f803258434d809cc5..265c49fc2862f05292bd0e1e320d8d03a0d5936f 100644
--- a/chrome/browser/performance_monitor/performance_monitor.h
+++ b/chrome/browser/performance_monitor/performance_monitor.h
@@ -28,11 +28,37 @@ namespace extensions {
 class Extension;
 }
 
+namespace net {
+class URLRequest;
+}
+
 namespace performance_monitor {
 class Database;
 
+// 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). When performing an action which should be done on a certain thread,

[Yoyo Zhou, 2012/08/21 21:29:16]

You don't need this bit about DCHECKing threads; it's well-understood and
obvious.

[Devlin, 2012/08/21 22:51:52]

Done.

+// a (D)CHECK is in place to ensure that it is always safe.
 class PerformanceMonitor : public content::NotificationObserver {
  public:
+  struct ByteCount {
+    ByteCount();
+
+    int64 disk;

[eaugusti, 2012/08/21 21:19:32]

Should these be uint64?

[Devlin, 2012/08/21 22:51:52]

Done.

+    int64 network;
+  };

[battre, 2012/08/21 20:37:33]

I haven't read all the code, but I would probably implement this the following
way:

struct PerformanceDataForIOThread {
  int64 network;
}
PerformanceDataForIOThread performance_data_for_io_thread_;

BytesRead() modifies performance_data_for_io_thread_ directly.

To gather this metric for reporting, you would post a task to the IOThread that
posts a callback tasks with a copy of the data structure to the background
thread in order to persist it.

[Devlin, 2012/08/21 21:49:02]

I understand your suggestion, but am still confused as to why it's necessary...
- bytes_read_ can only be modified on one thread, IO (no assignment race
condition).
- if the io thread is updating bytes read, we don't care if we insert the old
value into the database, since the new value will be inserted in the next
iteration, and it was close enough to be justified in either time slot.
- the data structure will persist so long as PerformanceMonitor lives (and if
PerformanceMonitor dies, so does the database, and the insertion is moot
anyway).

I have no problem making the change, but would like to understand the rationale
behind it.

[Devlin, 2012/08/21 22:10:33]

Ignore that.  Implementing change. (*grumbles something about using half-updated
values*)

[Devlin, 2012/08/21 22:51:52]

Done.

+
   typedef std::map<base::ProcessHandle,
                    linked_ptr<base::ProcessMetrics> > MetricsMap;
 
@@ -51,6 +77,10 @@ class PerformanceMonitor : public content::NotificationObserver {
   // start collecting data.
   void Start();
 
+  // Inform PerformanceMonitor that bytes have been read; increment the count
+  // accordingly.

[battre, 2012/08/21 20:37:33]

Sorry for my imprecise comment. I was asking to extend the comment with a
description that this is about number of bytes received from the network.

Usually system monitors distinguish between network traffic and disk traffic.
The latter would account for cache accesses, database operations, etc. I think I
would either drop the ByteCount::disk metric for now or try to catch all disk
access. This way it reports incomplete numbers.

[Yoyo Zhou, 2012/08/21 21:29:16]

Ooh, what about network file systems?
file:/// URLs can still fetch things from the network.

[Devlin, 2012/08/21 22:51:52]

Got rid of the disk metric, as requested. Per Yoyo's comment, is there a way of
distinguishing a network file system access from any other file access (I assume
that should still fall into network traffic)?

+  void BytesRead(const net::URLRequest& request, const int bytes);
+
   // content::NotificationObserver
   // Wait for various notifications; insert events into the database upon
   // occurance.
@@ -60,6 +90,7 @@ class PerformanceMonitor : public content::NotificationObserver {
 
   Database* database() { return database_.get(); }
   FilePath database_path() { return database_path_; }
+  static bool initialized() { return initialized_; }
 
  private:
   friend struct DefaultSingletonTraits<PerformanceMonitor>;
@@ -68,6 +99,8 @@ class PerformanceMonitor : public content::NotificationObserver {
                            OneProfileUncleanExit);
   FRIEND_TEST_ALL_PREFIXES(PerformanceMonitorUncleanExitBrowserTest,
                            TwoProfileUncleanExit);
+  FRIEND_TEST_ALL_PREFIXES(PerformanceMonitorBrowserTest, DiskBytesRead);
+  FRIEND_TEST_ALL_PREFIXES(PerformanceMonitorBrowserTest, NetworkBytesRead);
 
   PerformanceMonitor();
   virtual ~PerformanceMonitor();
@@ -138,6 +171,12 @@ class PerformanceMonitor : public content::NotificationObserver {
   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
+  // recording, and persists across browser runs. There are two separate counts:
+  // one for disk reads, and one for network reads.
+  ByteCount 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_;
@@ -152,6 +191,12 @@ class PerformanceMonitor : public content::NotificationObserver {
 
   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_;
+
   DISALLOW_COPY_AND_ASSIGN(PerformanceMonitor);
 };