Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2964543002:
TaskManager: use an unordered_map for tracking network usage (Closed)
DescriptionTaskManager: use an unordered_map for tracking network usage
This CL changes a vector to an unordered_map and splits
BytesTransferredParam into BytesTransferedKey and BytesTransferedParam.
Where the key contains the |origin_pid|, |child_id| and |route_id|. The
BytesTransferedParam contains |byte_read_count| and |byte_sent_count|
and will use those values to increment the entry in the unordered map.
This hoists |content::ResourceRequestInfo::ForRequest(request)| to the
task manager interface so the bytes transferred pipeline can be
simplified from the task manager interface to the task manager. This
also allows for unit tests to be written for
task_manager_io_thread_helper to test that the unordered map is being
filled out appropriately. This is done through dependency injection in
the IoThreadHelperManager.
BUG=720773
Review-Url: https://codereview.chromium.org/2964543002
Cr-Commit-Position: refs/heads/master@{#484652}
Committed: https://chromium.googlesource.com/chromium/src/+/c0a595329cf5a58ed1beb487a84c3213b0247061
Patch Set 1 #Patch Set 2 : changed some formatting back #Patch Set 3 : fixed more formatting #Patch Set 4 : edited comments #
Total comments: 22
Patch Set 5 : Updated from first CR #
Total comments: 50
Patch Set 6 : 2nd code review #Patch Set 7 : updated unit tests and changed some names #
Total comments: 6
Patch Set 8 : fixed nits from lgtm #
Depends on Patchset: Messages
Total messages: 43 (30 generated)
The CQ bit was checked by cburn@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cburn@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Changes a vector to map and hoists call to network delegate added test added unit tests hoisted calls up to chrome network delegate so testing of the network transferes is easier hashmap added need to test BUG= ========== to ========== This CL changes a vector to an unordered_map and splits BytesTransferredParam into BytesTransferedKey and BytesTransferedParam. Where the key contains the |origin_pid|, |child_id| and |route_id|. The BytesTransferedParam contains |byte_read_count| and |byte_sent_count| and will use those values to increment the entry in the unordered map. This hoists |content::ResourceRequestInfo::ForRequest(request)| to the chrome network delegate so the bytes transferred pipeline can be simplified from the chrome network delegate to the task manager. This also allows for unit tests to be written for task_manager_io_thread_helper to test that the unordered map is being filled out appropriately. This is done through dependency injection in the IoThreadHelper. ==========
Description was changed from ========== This CL changes a vector to an unordered_map and splits BytesTransferredParam into BytesTransferedKey and BytesTransferedParam. Where the key contains the |origin_pid|, |child_id| and |route_id|. The BytesTransferedParam contains |byte_read_count| and |byte_sent_count| and will use those values to increment the entry in the unordered map. This hoists |content::ResourceRequestInfo::ForRequest(request)| to the chrome network delegate so the bytes transferred pipeline can be simplified from the chrome network delegate to the task manager. This also allows for unit tests to be written for task_manager_io_thread_helper to test that the unordered map is being filled out appropriately. This is done through dependency injection in the IoThreadHelper. ========== to ========== This CL changes a vector to an unordered_map and splits BytesTransferredParam into BytesTransferedKey and BytesTransferedParam. Where the key contains the |origin_pid|, |child_id| and |route_id|. The BytesTransferedParam contains |byte_read_count| and |byte_sent_count| and will use those values to increment the entry in the unordered map. This hoists |content::ResourceRequestInfo::ForRequest(request)| to the chrome network delegate so the bytes transferred pipeline can be simplified from the chrome network delegate to the task manager. This also allows for unit tests to be written for task_manager_io_thread_helper to test that the unordered map is being filled out appropriately. This is done through dependency injection in the IoThreadHelper. ==========
Description was changed from ========== This CL changes a vector to an unordered_map and splits BytesTransferredParam into BytesTransferedKey and BytesTransferedParam. Where the key contains the |origin_pid|, |child_id| and |route_id|. The BytesTransferedParam contains |byte_read_count| and |byte_sent_count| and will use those values to increment the entry in the unordered map. This hoists |content::ResourceRequestInfo::ForRequest(request)| to the chrome network delegate so the bytes transferred pipeline can be simplified from the chrome network delegate to the task manager. This also allows for unit tests to be written for task_manager_io_thread_helper to test that the unordered map is being filled out appropriately. This is done through dependency injection in the IoThreadHelper. ========== to ========== This CL changes a vector to an unordered_map and splits BytesTransferredParam into BytesTransferedKey and BytesTransferedParam. Where the key contains the |origin_pid|, |child_id| and |route_id|. The BytesTransferedParam contains |byte_read_count| and |byte_sent_count| and will use those values to increment the entry in the unordered map. This hoists |content::ResourceRequestInfo::ForRequest(request)| to the chrome network delegate so the bytes transferred pipeline can be simplified from the chrome network delegate to the task manager. This also allows for unit tests to be written for task_manager_io_thread_helper to test that the unordered map is being filled out appropriately. This is done through dependency injection in the IoThreadHelperManager. ==========
cburn@google.com changed reviewers: + nick@chromium.org
ptal.
https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.cc:404: task_manager::TaskManagerInterface::OnRawBytesTransferred(key, bytes_received, Let's adjust this layering slightly. It shouldn't be chrome_network_delegate's job to unpack its struct into a task_manager::BytesTransferredKey. Constructing the BytesTransferredKey is 'business logic' that belongs inside the TaskManager. Since the unit tests are written terms of TaskManagerIoThreadHelper::OnRawBytesTransferred (and not in terms of the task_manager::TaskManagerInterface static methods), I think you can actually revert the changes to this file, and move the unpacking inside of TaskManagerInterface::OnRawBytesRead and TaskManagerInterface::OnRawBytesSent. By leaving the chrome/browser/net files untouched, we won't need anything but a task_manager OWNER to approve this change. https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h (right): https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:43: : origin_pid(origin_pid), child_id(child_id), route_id(route_id) {} 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. https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:63: int64_t byte_read_count; byte_read_count = 0; https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:66: int64_t byte_sent_count; byte_sent_count = 0; https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:67: BytesTransferredParam() : byte_read_count(0), byte_sent_count(0) {} If you use the inline member initialization (suggested in the previous 2 comments), I think you can omit this ctor. https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:68: BytesTransferredParam(int64_t byte_read_count, int64_t byte_sent_count) I don't see the 2-arg ctor being used anywhere. https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:70: }; newline here https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:73: BytesTransferredKey::Hasher>; newline here https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:81: IoThreadHelperManager(BytesTransferredCallback result_callback); add 'explicit' before IoThreadHelperManager, since this is a single-argument ctor. https://google.github.io/styleguide/cppguide.html#Implicit_Conversions https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:106: TaskManagerIoThreadHelper(BytesTransferredCallback result_callback); add 'explicit' here too https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... File chrome/browser/task_manager/task_manager_interface.cc (right): https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/task_manager_interface.cc:42: void TaskManagerInterface::OnRawBytesTransferred(BytesTransferredKey key, As suggested by the comment in chrome_network_delegate, let's go back to the previous signatures for TaskManagerInterface::OnRawBytesRead and TaskManagerInterface::OnRawBytesSent, as before. Do the request-to-BytesTransferredKey conversion in this file. And, rather than copying the the BytesTransferredKey computation code into both the Sent and Read functions, I suggest actually introducing an anonymous-namespace helper function like this: namespace { BytesTransferredKey KeyForRequest(const net::URLRequest& request) { // computation ... return {origin_pid, child_id, route_id}; // C++ is amazing! } } // namespace
The CQ bit was checked by cburn@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== This CL changes a vector to an unordered_map and splits BytesTransferredParam into BytesTransferedKey and BytesTransferedParam. Where the key contains the |origin_pid|, |child_id| and |route_id|. The BytesTransferedParam contains |byte_read_count| and |byte_sent_count| and will use those values to increment the entry in the unordered map. This hoists |content::ResourceRequestInfo::ForRequest(request)| to the chrome network delegate so the bytes transferred pipeline can be simplified from the chrome network delegate to the task manager. This also allows for unit tests to be written for task_manager_io_thread_helper to test that the unordered map is being filled out appropriately. This is done through dependency injection in the IoThreadHelperManager. ========== to ========== This CL changes a vector to an unordered_map and splits BytesTransferredParam into BytesTransferedKey and BytesTransferedParam. Where the key contains the |origin_pid|, |child_id| and |route_id|. The BytesTransferedParam contains |byte_read_count| and |byte_sent_count| and will use those values to increment the entry in the unordered map. This hoists |content::ResourceRequestInfo::ForRequest(request)| to the task manager interface so the bytes transferred pipeline can be simplified from the task manager interface to the task manager. This also allows for unit tests to be written for task_manager_io_thread_helper to test that the unordered map is being filled out appropriately. This is done through dependency injection in the IoThreadHelperManager. ==========
ptal https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate.cc (right): https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate.cc:404: task_manager::TaskManagerInterface::OnRawBytesTransferred(key, bytes_received, On 2017/06/29 23:40:00, ncarter (slow) wrote: > Let's adjust this layering slightly. It shouldn't be chrome_network_delegate's > job to unpack its struct into a task_manager::BytesTransferredKey. Constructing > the BytesTransferredKey is 'business logic' that belongs inside the TaskManager. > > Since the unit tests are written terms of > TaskManagerIoThreadHelper::OnRawBytesTransferred (and not in terms of the > task_manager::TaskManagerInterface static methods), I think you can actually > revert the changes to this file, and move the unpacking inside of > TaskManagerInterface::OnRawBytesRead and TaskManagerInterface::OnRawBytesSent. > > By leaving the chrome/browser/net files untouched, we won't need anything but a > task_manager OWNER to approve this change. Done. That is a much better solution. https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h (right): https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:43: : origin_pid(origin_pid), child_id(child_id), route_id(route_id) {} On 2017/06/29 23:40:00, ncarter (slow) wrote: > 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. Done. https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:63: int64_t byte_read_count; On 2017/06/29 23:40:01, ncarter (slow) wrote: > byte_read_count = 0; Done. https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:66: int64_t byte_sent_count; On 2017/06/29 23:40:01, ncarter (slow) wrote: > byte_sent_count = 0; Done. https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:67: BytesTransferredParam() : byte_read_count(0), byte_sent_count(0) {} On 2017/06/29 23:40:01, ncarter (slow) wrote: > If you use the inline member initialization (suggested in the previous 2 > comments), I think you can omit this ctor. Done. https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:68: BytesTransferredParam(int64_t byte_read_count, int64_t byte_sent_count) On 2017/06/29 23:40:01, ncarter (slow) wrote: > I don't see the 2-arg ctor being used anywhere. 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. https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:70: }; On 2017/06/29 23:40:01, ncarter (slow) wrote: > newline here Done. https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:73: BytesTransferredKey::Hasher>; On 2017/06/29 23:40:01, ncarter (slow) wrote: > newline here Done. https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:81: IoThreadHelperManager(BytesTransferredCallback result_callback); On 2017/06/29 23:40:01, ncarter (slow) wrote: > add 'explicit' before IoThreadHelperManager, since this is a single-argument > ctor. > > https://google.github.io/styleguide/cppguide.html#Implicit_Conversions Done. https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:106: TaskManagerIoThreadHelper(BytesTransferredCallback result_callback); On 2017/06/29 23:40:00, ncarter (slow) wrote: > add 'explicit' here too Done. https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... File chrome/browser/task_manager/task_manager_interface.cc (right): https://codereview.chromium.org/2964543002/diff/50001/chrome/browser/task_man... chrome/browser/task_manager/task_manager_interface.cc:42: void TaskManagerInterface::OnRawBytesTransferred(BytesTransferredKey key, On 2017/06/29 23:40:01, ncarter (slow) wrote: > As suggested by the comment in chrome_network_delegate, let's go back to the > previous signatures for TaskManagerInterface::OnRawBytesRead and > TaskManagerInterface::OnRawBytesSent, as before. > > Do the request-to-BytesTransferredKey conversion in this file. > > And, rather than copying the the BytesTransferredKey computation code into both > the Sent and Read functions, I suggest actually introducing an > anonymous-namespace helper function like this: > > namespace { > > BytesTransferredKey KeyForRequest(const net::URLRequest& request) { > // computation ... > > return {origin_pid, child_id, route_id}; // C++ is amazing! > } > > } // namespace > Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== This CL changes a vector to an unordered_map and splits BytesTransferredParam into BytesTransferedKey and BytesTransferedParam. Where the key contains the |origin_pid|, |child_id| and |route_id|. The BytesTransferedParam contains |byte_read_count| and |byte_sent_count| and will use those values to increment the entry in the unordered map. This hoists |content::ResourceRequestInfo::ForRequest(request)| to the task manager interface so the bytes transferred pipeline can be simplified from the task manager interface to the task manager. This also allows for unit tests to be written for task_manager_io_thread_helper to test that the unordered map is being filled out appropriately. This is done through dependency injection in the IoThreadHelperManager. ========== to ========== TaskManager: use an unordered_map for tracking network usage This CL changes a vector to an unordered_map and splits BytesTransferredParam into BytesTransferedKey and BytesTransferedParam. Where the key contains the |origin_pid|, |child_id| and |route_id|. The BytesTransferedParam contains |byte_read_count| and |byte_sent_count| and will use those values to increment the entry in the unordered map. This hoists |content::ResourceRequestInfo::ForRequest(request)| to the task manager interface so the bytes transferred pipeline can be simplified from the task manager interface to the task manager. This also allows for unit tests to be written for task_manager_io_thread_helper to test that the unordered map is being filled out appropriately. This is done through dependency injection in the IoThreadHelperManager. ==========
nick@chromium.org changed reviewers: + afakhry@chromium.org
Very close, this is looking great! +afakhry for thoughts https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_manager_impl.cc (right): https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_impl.cc:459: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); Inside the for loop below, prefer "const auto&" to "auto". (see "Range based for loops", https://chromium-cpp.appspot.com/) https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_impl.cc:460: for (auto it : params) { The name |it|, by convention, suggests that this is an iterator. But when you use a range-based for loop, you actually get not an iterator, but a reference to an BytesTransferredMap::value_type i.e. "std::pair<BytesTransferredKey, BytesTransferredParam>&". (An iterator is constructed under the hood, but it's completely hidden from you. Conceptually, the loop variable takes on the value of (*it) at each step.) So rename this. Ideas: |param| or |bytes_transferred| or |map_entry| or |entry| or |key_value_pair| or |process_bytes| https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_impl.cc:461: if (!GetInstance()->UpdateTasksWithBytesTransferred(it.first, it.second)) { The loop body might get more readable if you create these aliases at the top of the loop: const BytesTransferredKey& process_info = map_entry.first; const BytesTransferredParam& bytes_transferred = map_entry.second; (names totally up to you, and this suggestion is optional.) https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h (right): https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:28: // compared keys. 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. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:43: size_t operator()(const BytesTransferredKey& key) const { 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... https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:52: bool operator==(const BytesTransferredKey& other) const { Move this to the cc file too https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:97: // enabled. 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] https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:118: Remove this newline; the comment should adhere to the data member. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc (right): https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:5: #include "base/containers/container_test_utils.h" Do you use this? https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:10: #include "testing/gmock/include/gmock/gmock.h" I don't think you need gmock (I see no EXPECT_THAT's) https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:25: void GotData(BytesTransferredMap params) { returned_map_ = params; } 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"; https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:26: 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. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:57: ASSERT_TRUE(mock_main_runner->HasPendingTask()); 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). https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:61: ASSERT_EQ(returned_map_[key].byte_sent_count, correct_sent_bytes); 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!) https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:89: ASSERT_FALSE(mock_main_runner->HasPendingTask()); EXPECT_EQ(returned_map_.size(), 1); https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:133: base::RunLoop().RunUntilIdle(); EXPECT_EQ(returned_map_.size(), 2); https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:173: base::RunLoop().RunUntilIdle(); EXPECT_EQ(returned_map_.size(), 2); https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:221: ASSERT_FALSE(mock_main_runner->HasPendingTask()); EXPECT_EQ(returned_map_.size(), 2); https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:261: base::RunLoop().RunUntilIdle(); EXPECT_EQ(returned_map_.size(), 2); https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:273: BytesTransferredKey key2 = {32, -1, -1}; 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. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:306: base::RunLoop().RunUntilIdle(); EXPECT_EQ(returned_map_.size(), 2); https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:348: base::RunLoop().RunUntilIdle(); EXPECT_EQ(returned_map_.size(), 2); https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:352: ASSERT_EQ(returned_map_[key2].byte_read_count, correct_read_bytes2); Maybe: returned_map_.clear(); https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:385: base::RunLoop().RunUntilIdle(); EXPECT_EQ(returned_map_.size(), 2);
https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc (right): https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:57: ASSERT_TRUE(mock_main_runner->HasPendingTask()); On 2017/06/30 20:08:30, ncarter (slow) wrote: > 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). 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.
The CQ bit was checked by cburn@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
ptal. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_manager_impl.cc (right): https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_impl.cc:459: DCHECK_CURRENTLY_ON(content::BrowserThread::UI); On 2017/06/30 20:08:28, ncarter (slow) wrote: > Inside the for loop below, prefer "const auto&" to "auto". > > (see "Range based for loops", https://chromium-cpp.appspot.com/) Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_impl.cc:460: for (auto it : params) { On 2017/06/30 20:08:28, ncarter (slow) wrote: > The name |it|, by convention, suggests that this is an iterator. But when you > use a range-based for loop, you actually get not an iterator, but a reference to > an BytesTransferredMap::value_type i.e. "std::pair<BytesTransferredKey, > BytesTransferredParam>&". (An iterator is constructed under the hood, but it's > completely hidden from you. Conceptually, the loop variable takes on the value > of (*it) at each step.) > > So rename this. Ideas: |param| or |bytes_transferred| or |map_entry| or |entry| > or |key_value_pair| or |process_bytes| I went with entry. I may try to go through and change all of params to something that is more readable about what it actually is. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_impl.cc:461: if (!GetInstance()->UpdateTasksWithBytesTransferred(it.first, it.second)) { On 2017/06/30 20:08:28, ncarter (slow) wrote: > The loop body might get more readable if you create these aliases at the top of > the loop: > > const BytesTransferredKey& process_info = map_entry.first; > const BytesTransferredParam& bytes_transferred = map_entry.second; > > (names totally up to you, and this suggestion is optional.) Done. I am all for making loops more readable. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h (right): https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:28: // compared keys. On 2017/06/30 20:08:28, ncarter (slow) wrote: > 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. Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:43: size_t operator()(const BytesTransferredKey& key) const { On 2017/06/30 20:08:28, ncarter (slow) wrote: > 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... Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:52: bool operator==(const BytesTransferredKey& other) const { On 2017/06/30 20:08:28, ncarter (slow) wrote: > Move this to the cc file too Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:97: // enabled. On 2017/06/30 20:08:28, ncarter (slow) wrote: > 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] Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:118: On 2017/06/30 20:08:29, ncarter (slow) wrote: > Remove this newline; the comment should adhere to the data member. Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc (right): https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:5: #include "base/containers/container_test_utils.h" On 2017/06/30 20:08:29, ncarter (slow) wrote: > Do you use this? I do not, cruft from when I was experimenting with EXPECT_THAT's https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:10: #include "testing/gmock/include/gmock/gmock.h" On 2017/06/30 20:08:29, ncarter (slow) wrote: > I don't think you need gmock (I see no EXPECT_THAT's) I do not, cruft from when I was experimenting with EXPECT_THAT's https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:25: void GotData(BytesTransferredMap params) { returned_map_ = params; } On 2017/06/30 20:08:29, ncarter (slow) wrote: > 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"; Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:26: On 2017/06/30 20:08:29, ncarter (slow) wrote: > 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. Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:57: ASSERT_TRUE(mock_main_runner->HasPendingTask()); On 2017/06/30 20:10:07, ncarter (slow) wrote: > On 2017/06/30 20:08:30, ncarter (slow) wrote: > > 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). > > 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. Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:57: ASSERT_TRUE(mock_main_runner->HasPendingTask()); On 2017/06/30 20:08:30, ncarter (slow) wrote: > 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). Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:61: ASSERT_EQ(returned_map_[key].byte_sent_count, correct_sent_bytes); On 2017/06/30 20:08:29, ncarter (slow) wrote: > 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!) Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:89: ASSERT_FALSE(mock_main_runner->HasPendingTask()); On 2017/06/30 20:08:30, ncarter (slow) wrote: > EXPECT_EQ(returned_map_.size(), 1); Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:133: base::RunLoop().RunUntilIdle(); On 2017/06/30 20:08:29, ncarter (slow) wrote: > EXPECT_EQ(returned_map_.size(), 2); Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:173: base::RunLoop().RunUntilIdle(); On 2017/06/30 20:08:30, ncarter (slow) wrote: > EXPECT_EQ(returned_map_.size(), 2); Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:221: ASSERT_FALSE(mock_main_runner->HasPendingTask()); On 2017/06/30 20:08:29, ncarter (slow) wrote: > EXPECT_EQ(returned_map_.size(), 2); Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:261: base::RunLoop().RunUntilIdle(); On 2017/06/30 20:08:29, ncarter (slow) wrote: > EXPECT_EQ(returned_map_.size(), 2); Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:273: BytesTransferredKey key2 = {32, -1, -1}; On 2017/06/30 20:08:29, ncarter (slow) wrote: > > 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. Thank you! I was trying to figure out how to cause a collision and could not figure out a decent way to do it. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:306: base::RunLoop().RunUntilIdle(); On 2017/06/30 20:08:29, ncarter (slow) wrote: > EXPECT_EQ(returned_map_.size(), 2); Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:348: base::RunLoop().RunUntilIdle(); On 2017/06/30 20:08:29, ncarter (slow) wrote: > EXPECT_EQ(returned_map_.size(), 2); 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. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:352: ASSERT_EQ(returned_map_[key2].byte_read_count, correct_read_bytes2); On 2017/06/30 20:08:30, ncarter (slow) wrote: > Maybe: > > returned_map_.clear(); Done. https://codereview.chromium.org/2964543002/diff/70001/chrome/browser/task_man... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:385: base::RunLoop().RunUntilIdle(); On 2017/06/30 20:08:29, ncarter (slow) wrote: > EXPECT_EQ(returned_map_.size(), 2); Done.
lgtm https://codereview.chromium.org/2964543002/diff/110001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc (right): https://codereview.chromium.org/2964543002/diff/110001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:9: #include "testing/gmock/include/gmock/gmock.h" Add back #include "testing/gtest/include/gtest/gtest.h" & remove #include "testing/gmock/include/gmock/gmock.h"
lgtm https://codereview.chromium.org/2964543002/diff/110001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper.cc (right): https://codereview.chromium.org/2964543002/diff/110001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.cc:80: if (g_io_thread_helper) { Nit: Remove single-line block braces. https://codereview.chromium.org/2964543002/diff/110001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h (right): https://codereview.chromium.org/2964543002/diff/110001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:66: explicit IoThreadHelperManager(BytesTransferredCallback result_callback); Nit: Please document when this callback will be invoked and why is it declared repeating.
Description was changed from ========== TaskManager: use an unordered_map for tracking network usage This CL changes a vector to an unordered_map and splits BytesTransferredParam into BytesTransferedKey and BytesTransferedParam. Where the key contains the |origin_pid|, |child_id| and |route_id|. The BytesTransferedParam contains |byte_read_count| and |byte_sent_count| and will use those values to increment the entry in the unordered map. This hoists |content::ResourceRequestInfo::ForRequest(request)| to the task manager interface so the bytes transferred pipeline can be simplified from the task manager interface to the task manager. This also allows for unit tests to be written for task_manager_io_thread_helper to test that the unordered map is being filled out appropriately. This is done through dependency injection in the IoThreadHelperManager. ========== to ========== TaskManager: use an unordered_map for tracking network usage This CL changes a vector to an unordered_map and splits BytesTransferredParam into BytesTransferedKey and BytesTransferedParam. Where the key contains the |origin_pid|, |child_id| and |route_id|. The BytesTransferedParam contains |byte_read_count| and |byte_sent_count| and will use those values to increment the entry in the unordered map. This hoists |content::ResourceRequestInfo::ForRequest(request)| to the task manager interface so the bytes transferred pipeline can be simplified from the task manager interface to the task manager. This also allows for unit tests to be written for task_manager_io_thread_helper to test that the unordered map is being filled out appropriately. This is done through dependency injection in the IoThreadHelperManager. BUG=720773 ==========
The CQ bit was checked by cburn@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from afakhry@chromium.org, nick@chromium.org Link to the patchset: https://codereview.chromium.org/2964543002/#ps130001 (title: "fixed nits from lgtm")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by cburn@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by cburn@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2964543002/diff/110001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper.cc (right): https://codereview.chromium.org/2964543002/diff/110001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.cc:80: if (g_io_thread_helper) { On 2017/07/05 19:01:21, afakhry wrote: > Nit: Remove single-line block braces. Done. https://codereview.chromium.org/2964543002/diff/110001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h (right): https://codereview.chromium.org/2964543002/diff/110001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:66: explicit IoThreadHelperManager(BytesTransferredCallback result_callback); On 2017/07/05 19:01:21, afakhry wrote: > Nit: Please document when this callback will be invoked and why is it declared > repeating. Done. https://codereview.chromium.org/2964543002/diff/110001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc (right): https://codereview.chromium.org/2964543002/diff/110001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_io_thread_helper_unittest.cc:9: #include "testing/gmock/include/gmock/gmock.h" On 2017/07/05 18:28:18, ncarter (slow) wrote: > Add back > > #include "testing/gtest/include/gtest/gtest.h" > > & remove > > #include "testing/gmock/include/gmock/gmock.h" Done.
CQ is committing da patch.
Bot data: {"patchset_id": 130001, "attempt_start_ts": 1499362217758730,
"parent_rev": "6cec39438c5c3515de5d14159a971619f51b0751", "commit_rev":
"c0a595329cf5a58ed1beb487a84c3213b0247061"}
Message was sent while issue was closed.
Description was changed from ========== TaskManager: use an unordered_map for tracking network usage This CL changes a vector to an unordered_map and splits BytesTransferredParam into BytesTransferedKey and BytesTransferedParam. Where the key contains the |origin_pid|, |child_id| and |route_id|. The BytesTransferedParam contains |byte_read_count| and |byte_sent_count| and will use those values to increment the entry in the unordered map. This hoists |content::ResourceRequestInfo::ForRequest(request)| to the task manager interface so the bytes transferred pipeline can be simplified from the task manager interface to the task manager. This also allows for unit tests to be written for task_manager_io_thread_helper to test that the unordered map is being filled out appropriately. This is done through dependency injection in the IoThreadHelperManager. BUG=720773 ========== to ========== TaskManager: use an unordered_map for tracking network usage This CL changes a vector to an unordered_map and splits BytesTransferredParam into BytesTransferedKey and BytesTransferedParam. Where the key contains the |origin_pid|, |child_id| and |route_id|. The BytesTransferedParam contains |byte_read_count| and |byte_sent_count| and will use those values to increment the entry in the unordered map. This hoists |content::ResourceRequestInfo::ForRequest(request)| to the task manager interface so the bytes transferred pipeline can be simplified from the task manager interface to the task manager. This also allows for unit tests to be written for task_manager_io_thread_helper to test that the unordered map is being filled out appropriately. This is done through dependency injection in the IoThreadHelperManager. BUG=720773 Review-Url: https://codereview.chromium.org/2964543002 Cr-Commit-Position: refs/heads/master@{#484652} Committed: https://chromium.googlesource.com/chromium/src/+/c0a595329cf5a58ed1beb487a84c... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:130001) as https://chromium.googlesource.com/chromium/src/+/c0a595329cf5a58ed1beb487a84c... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
