TaskManager: use an unordered_map for tracking network usage

chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h

Index: chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h
diff --git a/chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h b/chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h
index 321025fa5f4d781bc2c6e88919824e0aa9b51215..b07ffc450bfe80418182f27f6bca6a066a68d3e7 100644
--- a/chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h
+++ b/chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h
@@ -7,8 +7,10 @@
 
 #include <stdint.h>
 
-#include <vector>
+#include <unordered_map>
 
+#include "base/callback.h"
+#include "base/hash.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 
@@ -18,9 +20,13 @@ class URLRequest;
 
 namespace task_manager {
 
-// Defines a wrapper of values that will be sent from IO to UI thread upon
-// reception and transmission of bytes notifications.
-struct BytesTransferredParam {
+// This will be the key that is used in an unordered_map to track the number of
+// bytes sent and read by a task. The hash function either does a hash of the
+// |child_id| and the |route_id| if the task is distinguished that way or it is
+// hashed by the |origin_pid|. The operator== is defined to be true when all
+// |origin_pid|, |child_id|, and |route_id| are all the same between the
+// compared keys.

[ncarter (slow), 2017/06/30 20:08:28]

This comment falls somewhat into the trap of "describing what the code does"
instead of "what the code is for".
https://google.github.io/styleguide/cppguide.html#Class_Comments

I would simplify this to:

// Identifies the initiator of a network request, either by a (child_id,
route_id) tuple, and/or via an OS process id.
//
// BytesTransferredKey supports hashing and may be used as an unordered_map key.

[cburn, 2017/07/05 16:30:06]

Done.

+struct BytesTransferredKey {
   // The PID of the originating process of the URLRequest, if the request is
   // sent on behalf of another process. Otherwise it's 0.
   int origin_pid;
@@ -33,29 +39,43 @@ struct BytesTransferredParam {
   // to).
   int route_id;
 
+  struct Hasher {
+    size_t operator()(const BytesTransferredKey& key) const {

[ncarter (slow), 2017/06/30 20:08:28]

This is nontrivial, I'd move it to the cc file instead of having the inline
definition. The rule in general is, "when in doubt, don't inline"
https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i...

[cburn, 2017/07/05 16:30:06]

Done.

+      if (key.child_id != -1) {
+        return base::HashInts(key.child_id, key.route_id);
+      } else {
+        return std::hash<int>()(key.origin_pid);
+      }
+    }
+  };
+
+  bool operator==(const BytesTransferredKey& other) const {

[ncarter (slow), 2017/06/30 20:08:28]

Move this to the cc file too

[cburn, 2017/07/05 16:30:06]

Done.

+    return origin_pid == other.origin_pid && child_id == other.child_id &&
+           route_id == other.route_id;
+  }
+};
+
+// This is the entry of the unordered map that tracks bytes transfered by task.
+struct BytesTransferredParam {
   // The number of bytes read.
-  int64_t byte_read_count;
+  int64_t byte_read_count = 0;
 
   // The number of bytes sent.
-  int64_t byte_sent_count;
-
-  BytesTransferredParam(int origin_pid,
-                        int child_id,
-                        int route_id,
-                        int64_t byte_read_count,
-                        int64_t byte_sent_count)
-      : origin_pid(origin_pid),
-        child_id(child_id),
-        route_id(route_id),
-        byte_read_count(byte_read_count),
-        byte_sent_count(byte_sent_count) {}
+  int64_t byte_sent_count = 0;
 };
 
+using BytesTransferredMap = std::unordered_map<BytesTransferredKey,
+                                               BytesTransferredParam,
+                                               BytesTransferredKey::Hasher>;
+
+using BytesTransferredCallback =
+    base::RepeatingCallback<void(BytesTransferredMap)>;
+
 // Defines a utility class used to schedule the creation and removal of the
 // TaskManagerIoThreadHelper on the IO thread.
 class IoThreadHelperManager {
  public:
-  IoThreadHelperManager();
+  explicit IoThreadHelperManager(BytesTransferredCallback result_callback);
   ~IoThreadHelperManager();
 
  private:
@@ -69,21 +89,18 @@ class TaskManagerIoThreadHelper {
  public:
   // Create and delete the instance of this class. They must be called on the IO
   // thread.
-  static void CreateInstance();
+  static void CreateInstance(BytesTransferredCallback result_callback);
   static void DeleteInstance();
 
-  // This is used to forward the call to update the network bytes with read
-  // bytes from the TaskManagerInterface if the new task manager is enabled.
-  static void OnRawBytesRead(const net::URLRequest& request,
-                             int64_t bytes_read);
-
-  // This is used to forward the call to update the network bytes with sent
-  // bytes from the TaskManagerInterface if the new task manager is enabled.
-  static void OnRawBytesSent(const net::URLRequest& request,
-                             int64_t bytes_sent);
+  // This is used to forward the call to update the network bytes with
+  // transferred bytes from the TaskManagerInterface if the new task manager is
+  // enabled.

[ncarter (slow), 2017/06/30 20:08:28]

not your code but while we're here, let's delete the qualification "if the new
task manager is enabled" -- it's cruft. [we used to have a "new task manager"
and a legacy task manager, and that's the distinction this was drawing]

[cburn, 2017/07/05 16:30:06]

Done.

+  static void OnRawBytesTransferred(BytesTransferredKey key,
+                                    int64_t bytes_read,
+                                    int64_t bytes_sent);
 
  private:
-  TaskManagerIoThreadHelper();
+  explicit TaskManagerIoThreadHelper(BytesTransferredCallback result_callback);
   ~TaskManagerIoThreadHelper();
 
   // We gather multiple notifications on the IO thread in one second before a
@@ -92,13 +109,16 @@ class TaskManagerIoThreadHelper {
   void OnMultipleBytesTransferredIO();
 
   // This will update the task manager with the network bytes read.
-  void OnNetworkBytesTransferred(const net::URLRequest& request,
+  void OnNetworkBytesTransferred(BytesTransferredKey key,
                                  int64_t bytes_read,
                                  int64_t bytes_sent);
 
-  // This buffer will be filled on IO thread with information about the number
-  // of bytes transferred from URLRequests.
-  std::vector<BytesTransferredParam> bytes_transferred_buffer_;
+  // This unordered_map will be filled on IO thread with information about the
+  // number of bytes transferred from URLRequests.
+

[ncarter (slow), 2017/06/30 20:08:29]

Remove this newline; the comment should adhere to the data member.

[cburn, 2017/07/05 16:30:06]

Done.

+  BytesTransferredMap bytes_transferred_unordered_map_;
+
+  BytesTransferredCallback result_callback_;
 
   base::WeakPtrFactory<TaskManagerIoThreadHelper> weak_factory_;