TaskManager: use an unordered_map for tracking network usage

chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc

Index: chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc
diff --git a/chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc b/chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..9d88329bde61c69ccf4757623503926b1e5b098f
--- /dev/null
+++ b/chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc
@@ -0,0 +1,392 @@
+// Copyright 2017 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.
+
+#include "base/containers/container_test_utils.h"

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

Do you use this?

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

I do not, cruft from when I was experimenting with EXPECT_THAT's

+#include "base/run_loop.h"
+#include "base/test/scoped_mock_time_message_loop_task_runner.h"
+#include "chrome/browser/task_manager/sampling/task_manager_impl.h"
+#include "content/public/test/test_browser_thread_bundle.h"
+#include "testing/gmock/include/gmock/gmock.h"

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

I don't think you need gmock (I see no EXPECT_THAT's)

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

I do not, cruft from when I was experimenting with EXPECT_THAT's

+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace task_manager {
+
+class TaskManagerIoThreadHelperTest : public testing::Test {
+ public:
+  TaskManagerIoThreadHelperTest()
+      : helper_manager_(
+            base::BindRepeating(&TaskManagerIoThreadHelperTest::GotData,
+                                base::Unretained(this))) {
+    base::RunLoop().RunUntilIdle();
+  }
+  ~TaskManagerIoThreadHelperTest() override {}
+
+  void GotData(BytesTransferredMap params) { returned_map_ = params; }

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

Might be a good idea for GotData() to include the following expectations before
the assignment:

EXPECT_TRUE(returned_map_.empty()) << "GotData() delivered twice";
EXPECT_FALSE(params.empty()) << "GotData() called with empty map";

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

Done.

+

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

Make a "protected:" section here, just for returned_map_. This indicates to the
reader that it's designed to be referenced by the test bodies.

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

Done.

+  BytesTransferredMap returned_map_;
+
+ private:
+  content::TestBrowserThreadBundle thread_bundle_;
+  IoThreadHelperManager helper_manager_;
+
+  DISALLOW_COPY_AND_ASSIGN(TaskManagerIoThreadHelperTest);
+};
+
+// Tests that the origin_id is used in the map correctly.
+TEST_F(TaskManagerIoThreadHelperTest, PidData) {
+  base::ScopedMockTimeMessageLoopTaskRunner mock_main_runner;
+
+  BytesTransferredKey key = {123, -1, -1};
+
+  int correct_read_bytes = 0;
+  int correct_sent_bytes = 0;
+
+  int read_bytes_array[] = {200, 400, 800};
+  int sent_bytes_array[] = {100, 123, 234};
+
+  for (int i : read_bytes_array) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key, i, 0);
+    correct_read_bytes += i;
+  }
+  for (int i : sent_bytes_array) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key, 0, i);
+    correct_sent_bytes += i;
+  }
+
+  ASSERT_TRUE(mock_main_runner->HasPendingTask());

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

Replace every ASSERT in this file with EXPECT. In general, we use EXPECT, unless
the condition failing would cause the other assertions to crash (e.g., you can
ASSERT that a 

Both ASSERT and EXPECT will cause a test to fail. The difference is that ASSERT
embeds a 'return' statement to abort the rest of the test. The convention is to
use EXPECT wherever possible so that when you see a failure on the bot, you get
as much info about the mismatch as possible (you see the output from all failing
EXPECT macros since execution continues after failure). 

[ncarter (slow), 2017/06/30 20:10:07]

Finishing this thought: a good use of ASSERT would be to assert that a pointer
is non-null before dereferencing it, since the ASSERT failure is often more
readable than the resulting crash stack.

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

Done.

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

Done.

+  mock_main_runner->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  base::RunLoop().RunUntilIdle();
+  ASSERT_FALSE(mock_main_runner->HasPendingTask());
+  ASSERT_EQ(returned_map_[key].byte_sent_count, correct_sent_bytes);

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

Swap the order of these parameters. The convention with gtest is to:

  ASSERT_EQ(expected, actual);

This is important because if you do see a failure, it'll print something like:

  "Expected X but got Y instead"

(You can use sublime to do a mass edit of these. Ask me how!)

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

Done.

+  ASSERT_EQ(returned_map_[key].byte_read_count, correct_read_bytes);
+}
+
+// Tests that the |child_id| and |route_id| are used in the map correctly.
+TEST_F(TaskManagerIoThreadHelperTest, ChildRouteData) {
+  base::ScopedMockTimeMessageLoopTaskRunner mock_main_runner;
+
+  BytesTransferredKey key = {0, 100, 190};
+
+  int correct_read_bytes = 0;
+  int correct_sent_bytes = 0;
+
+  int read_bytes_array[] = {900, 300, 100};
+  int sent_bytes_array[] = {130, 153, 934};
+
+  for (int i : read_bytes_array) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key, i, 0);
+    correct_read_bytes += i;
+  }
+  for (int i : sent_bytes_array) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key, 0, i);
+    correct_sent_bytes += i;
+  }
+
+  ASSERT_TRUE(mock_main_runner->HasPendingTask());
+  mock_main_runner->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  base::RunLoop().RunUntilIdle();
+  ASSERT_FALSE(mock_main_runner->HasPendingTask());

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

EXPECT_EQ(returned_map_.size(), 1);

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

Done.

+  ASSERT_EQ(returned_map_[key].byte_sent_count, correct_sent_bytes);
+  ASSERT_EQ(returned_map_[key].byte_read_count, correct_read_bytes);
+}
+
+// Tests that two distinct |origin_pid| are tracked separately in the unordered
+// map.
+TEST_F(TaskManagerIoThreadHelperTest, TwoPidData) {
+  base::ScopedMockTimeMessageLoopTaskRunner mock_main_runner;
+
+  BytesTransferredKey key1 = {23, -1, -1};
+  BytesTransferredKey key2 = {33, -1, -1};
+
+  int correct_read_bytes1 = 0;
+  int correct_sent_bytes1 = 0;
+
+  int correct_read_bytes2 = 0;
+  int correct_sent_bytes2 = 0;
+
+  int read_bytes_array1[] = {900, 300, 100};
+  int sent_bytes_array1[] = {130, 153, 934};
+
+  int read_bytes_array2[] = {903, 340, 150};
+  int sent_bytes_array2[] = {138, 157, 964};
+
+  for (int i : read_bytes_array1) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key1, i, 0);
+    correct_read_bytes1 += i;
+  }
+  for (int i : sent_bytes_array1) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key1, 0, i);
+    correct_sent_bytes1 += i;
+  }
+
+  for (int i : read_bytes_array2) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key2, i, 0);
+    correct_read_bytes2 += i;
+  }
+  for (int i : sent_bytes_array2) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key2, 0, i);
+    correct_sent_bytes2 += i;
+  }
+
+  mock_main_runner->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  base::RunLoop().RunUntilIdle();

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

EXPECT_EQ(returned_map_.size(), 2);

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

Done.

+  ASSERT_EQ(returned_map_[key1].byte_sent_count, correct_sent_bytes1);
+  ASSERT_EQ(returned_map_[key1].byte_read_count, correct_read_bytes1);
+  ASSERT_EQ(returned_map_[key2].byte_sent_count, correct_sent_bytes2);
+  ASSERT_EQ(returned_map_[key2].byte_read_count, correct_read_bytes2);
+}
+
+// Tests that two keys with the same |origin_pid| are tracked together in the
+// unordered map.
+TEST_F(TaskManagerIoThreadHelperTest, TwoSamePidData) {
+  base::ScopedMockTimeMessageLoopTaskRunner mock_main_runner;
+
+  BytesTransferredKey key1 = {23, -1, -1};
+  BytesTransferredKey key2 = {23, -1, -1};
+
+  int correct_read_bytes = 0;
+  int correct_sent_bytes = 0;
+
+  int read_bytes_array[] = {900, 300, 100};
+  int sent_bytes_array[] = {130, 153, 934};
+
+  for (int i : read_bytes_array) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key1, i, 0);
+    correct_read_bytes += i;
+  }
+  for (int i : sent_bytes_array) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key1, 0, i);
+    correct_sent_bytes += i;
+  }
+
+  for (int i : read_bytes_array) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key2, i, 0);
+    correct_read_bytes += i;
+  }
+  for (int i : sent_bytes_array) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key2, 0, i);
+    correct_sent_bytes += i;
+  }
+
+  mock_main_runner->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  base::RunLoop().RunUntilIdle();

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

EXPECT_EQ(returned_map_.size(), 2);

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

Done.

+  ASSERT_EQ(returned_map_[key1].byte_sent_count, correct_sent_bytes);
+  ASSERT_EQ(returned_map_[key1].byte_read_count, correct_read_bytes);
+  ASSERT_EQ(returned_map_[key2].byte_sent_count, correct_sent_bytes);
+  ASSERT_EQ(returned_map_[key2].byte_read_count, correct_read_bytes);
+}
+
+// Tests that two distinct |child_id| and |route_id| pairs are tracked
+// separately in the unordered map.
+TEST_F(TaskManagerIoThreadHelperTest, TwoChildRouteData) {
+  base::ScopedMockTimeMessageLoopTaskRunner mock_main_runner;
+
+  BytesTransferredKey key1 = {0, 32, 1};
+  BytesTransferredKey key2 = {0, 17, 2};
+
+  int correct_read_bytes1 = 0;
+  int correct_sent_bytes1 = 0;
+
+  int correct_read_bytes2 = 0;
+  int correct_sent_bytes2 = 0;
+
+  int read_bytes_array1[] = {453, 987654, 946650};
+  int sent_bytes_array1[] = {138450, 1556473, 954434};
+
+  int read_bytes_array2[] = {905643, 324340, 654150};
+  int sent_bytes_array2[] = {1232138, 157312, 965464};
+
+  for (int i : read_bytes_array1) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key1, i, 0);
+    correct_read_bytes1 += i;
+  }
+  for (int i : sent_bytes_array1) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key1, 0, i);
+    correct_sent_bytes1 += i;
+  }
+
+  for (int i : read_bytes_array2) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key2, i, 0);
+    correct_read_bytes2 += i;
+  }
+  for (int i : sent_bytes_array2) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key2, 0, i);
+    correct_sent_bytes2 += i;
+  }
+
+  ASSERT_TRUE(mock_main_runner->HasPendingTask());
+  mock_main_runner->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  base::RunLoop().RunUntilIdle();
+  ASSERT_FALSE(mock_main_runner->HasPendingTask());

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

EXPECT_EQ(returned_map_.size(), 2);

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

Done.

+  ASSERT_EQ(returned_map_[key1].byte_sent_count, correct_sent_bytes1);
+  ASSERT_EQ(returned_map_[key1].byte_read_count, correct_read_bytes1);
+  ASSERT_EQ(returned_map_[key2].byte_sent_count, correct_sent_bytes2);
+  ASSERT_EQ(returned_map_[key2].byte_read_count, correct_read_bytes2);
+}
+
+// Tests that two keys with the same |child_id| and |route_id| are tracked
+// together in the unordered map.
+TEST_F(TaskManagerIoThreadHelperTest, TwoSameChildRouteData) {
+  base::ScopedMockTimeMessageLoopTaskRunner mock_main_runner;
+
+  BytesTransferredKey key1 = {0, 123, 456};
+  BytesTransferredKey key2 = {0, 123, 456};
+
+  int correct_read_bytes = 0;
+  int correct_sent_bytes = 0;
+
+  int read_bytes_array[] = {90440, 12300, 103420};
+  int sent_bytes_array[] = {44130, 12353, 93234};
+
+  for (int i : read_bytes_array) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key1, i, 0);
+    correct_read_bytes += i;
+  }
+  for (int i : sent_bytes_array) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key1, 0, i);
+    correct_sent_bytes += i;
+  }
+
+  for (int i : read_bytes_array) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key2, i, 0);
+    correct_read_bytes += i;
+  }
+  for (int i : sent_bytes_array) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key2, 0, i);
+    correct_sent_bytes += i;
+  }
+
+  mock_main_runner->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  base::RunLoop().RunUntilIdle();

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

EXPECT_EQ(returned_map_.size(), 2);

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

Done.

+  ASSERT_EQ(returned_map_[key1].byte_sent_count, correct_sent_bytes);
+  ASSERT_EQ(returned_map_[key1].byte_read_count, correct_read_bytes);
+  ASSERT_EQ(returned_map_[key2].byte_sent_count, correct_sent_bytes);
+  ASSERT_EQ(returned_map_[key2].byte_read_count, correct_read_bytes);
+}
+
+// Tests that the unordered_map can hold both types of key in one map.
+TEST_F(TaskManagerIoThreadHelperTest, OnePidOneChildRouteData) {
+  base::ScopedMockTimeMessageLoopTaskRunner mock_main_runner;
+
+  BytesTransferredKey key1 = {0, 12, 143};
+  BytesTransferredKey key2 = {32, -1, -1};

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

BytesTransferredKey key1 = {0, 12, 143};
BytesTransferredKey key2 = {static_cast<int>(base::HashInts(12, 143)), -1, -1};

I *think* this might only be a collision test on 32-bit builds, but some
coverage of the pathological case might be good.

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

Thank you! I was trying to figure out how to cause a collision and could not
figure out a decent way to do it. 

+
+  int correct_read_bytes1 = 0;
+  int correct_sent_bytes1 = 0;
+
+  int correct_read_bytes2 = 0;
+  int correct_sent_bytes2 = 0;
+
+  int read_bytes_array1[] = {453, 98754, 94650};
+  int sent_bytes_array1[] = {1350, 15643, 95434};
+
+  int read_bytes_array2[] = {905643, 3243, 654150};
+  int sent_bytes_array2[] = {12338, 157312, 9664};
+
+  for (int i : read_bytes_array1) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key1, i, 0);
+    correct_read_bytes1 += i;
+  }
+  for (int i : sent_bytes_array1) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key1, 0, i);
+    correct_sent_bytes1 += i;
+  }
+
+  for (int i : read_bytes_array2) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key2, i, 0);
+    correct_read_bytes2 += i;
+  }
+  for (int i : sent_bytes_array2) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key2, 0, i);
+    correct_sent_bytes2 += i;
+  }
+
+  mock_main_runner->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  base::RunLoop().RunUntilIdle();

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

EXPECT_EQ(returned_map_.size(), 2);

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

Done.

+  ASSERT_EQ(returned_map_[key1].byte_sent_count, correct_sent_bytes1);
+  ASSERT_EQ(returned_map_[key1].byte_read_count, correct_read_bytes1);
+  ASSERT_EQ(returned_map_[key2].byte_sent_count, correct_sent_bytes2);
+  ASSERT_EQ(returned_map_[key2].byte_read_count, correct_read_bytes2);
+}
+
+// Tests that the map can store both types of keys and that it does update after
+// 1 mocked second passes and there is new traffic.
+TEST_F(TaskManagerIoThreadHelperTest, MultipleWavesMixedData) {
+  base::ScopedMockTimeMessageLoopTaskRunner mock_main_runner;
+
+  BytesTransferredKey key1 = {0, 12, 143};
+  BytesTransferredKey key2 = {32, -1, -1};
+
+  int correct_read_bytes1 = 0;
+  int correct_sent_bytes1 = 0;
+
+  int correct_read_bytes2 = 0;
+  int correct_sent_bytes2 = 0;
+
+  int read_bytes_array1[] = {453, 98754, 94650};
+  int sent_bytes_array1[] = {1350, 15643, 95434};
+
+  for (int i : read_bytes_array1) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key1, i, 0);
+    correct_read_bytes1 += i;
+  }
+  for (int i : sent_bytes_array1) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key1, 0, i);
+    correct_sent_bytes1 += i;
+  }
+
+  mock_main_runner->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  base::RunLoop().RunUntilIdle();
+  correct_sent_bytes1 = 0;
+  correct_read_bytes1 = 0;
+
+  TaskManagerIoThreadHelper::OnRawBytesTransferred(key1, 0, 10);
+  correct_sent_bytes1 += 10;
+
+  mock_main_runner->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  base::RunLoop().RunUntilIdle();

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

EXPECT_EQ(returned_map_.size(), 2);

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

I have this as EXPECT_EQ(1, returned_map_.size()) because we haven't used the
second key yet. I moved a few things around to hopefully make this more clear
reading it. 

+  ASSERT_EQ(returned_map_[key1].byte_sent_count, correct_sent_bytes1);
+  ASSERT_EQ(returned_map_[key1].byte_read_count, correct_read_bytes1);
+  ASSERT_EQ(returned_map_[key2].byte_sent_count, correct_sent_bytes2);
+  ASSERT_EQ(returned_map_[key2].byte_read_count, correct_read_bytes2);

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

Maybe:

returned_map_.clear(); 

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

Done.

+
+  correct_read_bytes1 = 0;
+  correct_sent_bytes1 = 0;
+
+  correct_read_bytes2 = 0;
+  correct_sent_bytes2 = 0;
+
+  int read_bytes_array_second_1[] = {4153, 987154, 946501};
+  int sent_bytes_array_second_1[] = {13510, 115643, 954134};
+
+  int read_bytes_array2[] = {9056243, 32243, 6541250};
+  int sent_bytes_array2[] = {123238, 1527312, 96624};
+
+  for (int i : read_bytes_array_second_1) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key1, i, 0);
+    correct_read_bytes1 += i;
+  }
+  for (int i : sent_bytes_array_second_1) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key1, 0, i);
+    correct_sent_bytes1 += i;
+  }
+
+  for (int i : read_bytes_array2) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key2, i, 0);
+    correct_read_bytes2 += i;
+  }
+  for (int i : sent_bytes_array2) {
+    TaskManagerIoThreadHelper::OnRawBytesTransferred(key2, 0, i);
+    correct_sent_bytes2 += i;
+  }
+
+  mock_main_runner->FastForwardBy(base::TimeDelta::FromSeconds(1));
+  base::RunLoop().RunUntilIdle();

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

EXPECT_EQ(returned_map_.size(), 2);

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

Done.

+  ASSERT_EQ(returned_map_[key1].byte_sent_count, correct_sent_bytes1);
+  ASSERT_EQ(returned_map_[key1].byte_read_count, correct_read_bytes1);
+  ASSERT_EQ(returned_map_[key2].byte_sent_count, correct_sent_bytes2);
+  ASSERT_EQ(returned_map_[key2].byte_read_count, correct_read_bytes2);
+}
+
+}  // namespace task_manager