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 10 matching lines @@
 #include "content/public/browser/notification_details.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"
 
 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;
 
   // 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.

[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.
   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 initialized() { return initialized_; }
 
  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, DiskBytesRead);
+  FRIEND_TEST_ALL_PREFIXES(PerformanceMonitorBrowserTest, NetworkBytesRead);
 
   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
+  // 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_;
 
   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 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);
 };
 
 }  // namespace performance_monitor
 
 #endif  // CHROME_BROWSER_PERFORMANCE_MONITOR_PERFORMANCE_MONITOR_H_