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..c7188364dcd8c2fd78334b53b7bd49dafefc5ea1 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.
+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,46 @@ struct BytesTransferredParam {
   // to).
   int route_id;
 
+  BytesTransferredKey(int origin_pid, int child_id, int route_id)
+      : origin_pid(origin_pid), child_id(child_id), route_id(route_id) {}

[ncarter (slow), 2017/06/29 23:40:00]

If you erase this ctor, does everything still compile?

This struct appears to be POD (plain old data -- just a bag of int's), so you
should be able to get by with using simple struct-initialization syntax.

[cburn, 2017/06/30 18:04:38]

Done.

+  struct Hasher {
+    size_t operator()(const BytesTransferredKey& key) const {
+      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 {
+    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;

[ncarter (slow), 2017/06/29 23:40:01]

byte_read_count = 0;

[cburn, 2017/06/30 18:04:38]

Done.

 
   // The number of bytes sent.
   int64_t byte_sent_count;

[ncarter (slow), 2017/06/29 23:40:01]

byte_sent_count = 0;

[cburn, 2017/06/30 18:04:38]

Done.

-
-  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) {}
+  BytesTransferredParam() : byte_read_count(0), byte_sent_count(0) {}

[ncarter (slow), 2017/06/29 23:40:01]

If you use the inline member initialization (suggested in the previous 2
comments), I think you can omit this ctor.

[cburn, 2017/06/30 18:04:38]

Done.

+  BytesTransferredParam(int64_t byte_read_count, int64_t byte_sent_count)

[ncarter (slow), 2017/06/29 23:40:01]

I don't see the 2-arg ctor being used anywhere.

[cburn, 2017/06/30 18:04:38]

Removed this, I think it was created before I understood the default
initialization could be leveraged in the map to create the elegant running
total. 

+      : byte_read_count(byte_read_count), byte_sent_count(byte_sent_count) {}
 };

[ncarter (slow), 2017/06/29 23:40:01]

newline here

[cburn, 2017/06/30 18:04:38]

Done.

+using BytesTransferredMap = std::unordered_map<BytesTransferredKey,
+                                               BytesTransferredParam,
+                                               BytesTransferredKey::Hasher>;

[ncarter (slow), 2017/06/29 23:40:01]

newline here

[cburn, 2017/06/30 18:04:38]

Done.

+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();
+  IoThreadHelperManager(BytesTransferredCallback result_callback);

[ncarter (slow), 2017/06/29 23:40:01]

add 'explicit' before IoThreadHelperManager, since this is a single-argument
ctor.

https://google.github.io/styleguide/cppguide.html#Implicit_Conversions

[cburn, 2017/06/30 18:04:38]

Done.

   ~IoThreadHelperManager();
 
  private:
@@ -69,21 +92,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.
+  static void OnRawBytesTransferred(BytesTransferredKey key,
+                                    int64_t bytes_read,
+                                    int64_t bytes_sent);
 
  private:
-  TaskManagerIoThreadHelper();
+  TaskManagerIoThreadHelper(BytesTransferredCallback result_callback);

[ncarter (slow), 2017/06/29 23:40:00]

add 'explicit' here too

[cburn, 2017/06/30 18:04:38]

Done.

   ~TaskManagerIoThreadHelper();
 
   // We gather multiple notifications on the IO thread in one second before a
@@ -92,13 +112,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.
+
+  BytesTransferredMap bytes_transferred_unordered_map_;
+
+  BytesTransferredCallback result_callback_;
 
   base::WeakPtrFactory<TaskManagerIoThreadHelper> weak_factory_;