Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2905403002:
plumb network upload into the task manager (Closed)
DescriptionThis adds the plumbing needed to take data about completed outbound
network traffic to the task manager for tracking network usage.
There are a number of naming changes to help distinguish between what
is and isn't a "rate" and what is sent vs read vs transferred. Read being
incoming bytes, sent being outgoing bytes and transfered being either
read or sent bytes.
The "rates" are not calculated based on what is live on the network.
It is instead based on when requests are completed so large uploads
will all be registered in one refresh.
This update changes the struct BytesReadParam to BytesTrasferedParam
and stores both bytes read and bytes sent. It updates how the struct is
processed to handle the additional tracking of sent bytes.
Tasks have been changed to store the cumulative_bytes_read_ and
cumulative_bytes_sent_ which are stored on Refresh() to the
last_refresh_cumulative_bytes_sent_ and
last_refresh_cumulative_bytes_read_ . These are used to calculate
network_sent_rate_ and network_read_rate_ which are summed for the
network_usage_rate_.
Tasks no longer store a network usage of -1 to signify that they have
not had any network traffic, the utility of that flag no longer appears
to be used so it was removed. Because of this ReportsNetworkUsage() has been removed.
Tasks and Task Groups can now report their cumulative network usage.
BUG=720773
Review-Url: https://codereview.chromium.org/2905403002
Cr-Commit-Position: refs/heads/master@{#480988}
Committed: https://chromium.googlesource.com/chromium/src/+/e9d2f361a156c914b8e4ac51f98397c33d92573d
Patch Set 1 #Patch Set 2 : adding browser test to task manager network uploads not being tracked #Patch Set 3 : Fixed failing IdleWakeups test; missing break statement #Patch Set 4 : Added aggregate network use for both sent and recieved bytes #Patch Set 5 : fixed some typos noticed in code review #Patch Set 6 : fixed negative byte totals #
Total comments: 28
Patch Set 7 : merged byte_read_buffer_ and byte_sent_buffer_ into byte_transfered_buffer_ to make it one pipeline #Patch Set 8 : updated names and comments for readability #Patch Set 9 : fixed comments and names as far as possible in only task manager #Patch Set 10 : missed save caused file to retain old naming #Patch Set 11 : fixed spelling/parellelism issues #
Total comments: 32
Patch Set 12 : moved test and added task group tests #Patch Set 13 : added catch for base::TimeDelta() being 0s and causing a div by 0 crash #Patch Set 14 : added refresh timer tests #
Total comments: 34
Patch Set 15 : changed browser test to be hopefully less flakey #Patch Set 16 : js formatting, fixing spelling, modifying browser tests #
Total comments: 8
Patch Set 17 : fixed comments #Patch Set 18 : fixed comments #Dependent Patchsets: Messages
Total messages: 90 (77 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: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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 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: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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...
nick@chromium.org changed reviewers: + nick@chromium.org
This is looking really good. I have some structural feedback aimed at simplifying the code, but overall this is an impressive first Chromium CL, and I love the tests. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/browser_process_task_unittest.cc (right): https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:63: // Test the provided browser process task for recieved bytes rate This comment should be restored; you didn't edit this test. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:83: REFRESH_TYPE_NETWORK_USAGE); Since we have new tests specifically focused on network usage, maybe remove the OnNetworkBytesRead and below from this one? https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:89: TEST_F(BrowserProcessTaskProviderTest, TestSentTask) { A good name for this test would be: BrowserProcessTaskProviderTest.NetworkBytesSent ('Test' and 'Task' already appears in the fixture name [BrowserProcessTaskProviderTest] so you don't need to put it in the text name too). Name the test after what it does differently from the other tests. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:95: EXPECT_EQ(base::GetCurrentProcessHandle(), provided_task_->process_handle()); It's not necessary to duplicate the GetCurrentProcHandle asserts into the new tests -- they're redundant to the coverage that TestProvidedTask does. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:113: // Test the provided browser process task for recieved then sent bytes rate recieved -> received (here and elsewhere) https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:114: TEST_F(BrowserProcessTaskProviderTest, TestRecievedSentTask) { BrowserProcessTaskProviderTest.NetworkBytesReceived https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/task.h (right): https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:173: // The task's total network usage for its lifetime measured in bytes Extra space before 'total' https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:209: int64_t total_sent_byte_count_; Let's try to reduce the number of fields here to as few as possible -- I think four should suffice, and we can interpolate the rest: last_refresh_cumulative_bytes_sent_ cumulative_bytes_sent_ last_refresh_cumulative_bytes_read_ cumulative_bytes_read_ (maybe we need to store a TimeDelta too, but that's less clear to me) the rate can be determined then by (last_refresh_cumulative - cumulative) / (time interval), and the combined totals can just be computed by adding (sent + read). It's fine to expose a getter_function() that does simple arithmetic (e.g. to get the totals). https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_impl.cc (right): https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.cc:485: void TaskManagerImpl::OnMultipleBytesSentUI( Once BytesTransferedParam holds both send and read data, merge this with OnMultipleBytesReadUI. Possible name suggestion: AscribeNetworkUsageOnUI? https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.cc:569: bool TaskManagerImpl::UpdateTasksWithBytesRead( Merge this function with TaskManagerImpl::UpdateTasksWithBytesSent. (UpdateTasksWithNetworkUsage ? ) https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.cc:576: task->OnNetworkBytesRead(param.byte_count); This might be the natural spot to split off into separate Read/Sent methods. We've already looked up the Task, so we can just have two calls here: if (task) { task->OnNetworkBytesRead(param.bytes_read); task->OnNetworkBytesSent(param.bytes_sent); return true; } https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_impl.h (right): https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.h:105: static void OnMultipleBytesSentUI(std::vector<BytesTransferedParam>* params); Because the PostTask operation itself is a few heap operations, it's probably better not to have two functions here, and instead to make BytesTransferedParam have both |bytes_read| and |bytes_sent| fields. It should be less memory this way, and hopefully less code too. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper.cc (right): https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.cc:131: There's a potential optimization here we can consider -- if bytes_read_buffer_ is non-empty, and bytes_read_buffer_.back() has a match for (origin_pid, child_id, route_id), then we can update (operator+=) the existing entry rather than pushing a new one. We could search the entire vector too, but I'm thinking that the common case here is a burst of requests from one client. Especially because every request has a little bit of send, this may prevent this buffer from getting problematically bigger as a result of what we're adding. Most efficient would be to make this a std::unordered_set<> (defining a custom hasher for BytesTransferedParam) or std::unordered_map<> (using std::tuple as the keys, and maybe eliminating BytesTransferedParam altogether). But if we go that route, it probably makes sense to tackle it as a follow-on change. So leave this as a vector for now. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.cc:143: bytes_sent_buffer_.swap(*bytes_sent_buffer); Because this whole dance with the vectors, swapping, and the PostDelayedTask is actually a very subtle optimization, it's worthwhile to have only one implementation of it.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
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...
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
ptal https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/browser_process_task_unittest.cc (right): https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:63: // Test the provided browser process task for recieved bytes rate On 2017/06/08 23:37:18, ncarter (slow) wrote: > This comment should be restored; you didn't edit this test. Done. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:83: REFRESH_TYPE_NETWORK_USAGE); On 2017/06/08 23:37:18, ncarter (slow) wrote: > Since we have new tests specifically focused on network usage, maybe remove the > OnNetworkBytesRead and below from this one? Done. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:89: TEST_F(BrowserProcessTaskProviderTest, TestSentTask) { On 2017/06/08 23:37:18, ncarter (slow) wrote: > A good name for this test would be: > > BrowserProcessTaskProviderTest.NetworkBytesSent > > ('Test' and 'Task' already appears in the fixture name > [BrowserProcessTaskProviderTest] so you don't need to put it in the text name > too). Name the test after what it does differently from the other tests. Done. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:95: EXPECT_EQ(base::GetCurrentProcessHandle(), provided_task_->process_handle()); On 2017/06/08 23:37:18, ncarter (slow) wrote: > It's not necessary to duplicate the GetCurrentProcHandle asserts into the new > tests -- they're redundant to the coverage that TestProvidedTask does. Done. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:113: // Test the provided browser process task for recieved then sent bytes rate On 2017/06/08 23:37:18, ncarter (slow) wrote: > recieved -> received (here and elsewhere) Done. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:114: TEST_F(BrowserProcessTaskProviderTest, TestRecievedSentTask) { On 2017/06/08 23:37:18, ncarter (slow) wrote: > BrowserProcessTaskProviderTest.NetworkBytesReceived Done. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/task.h (right): https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:173: // The task's total network usage for its lifetime measured in bytes On 2017/06/08 23:37:18, ncarter (slow) wrote: > Extra space before 'total' Done. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:209: int64_t total_sent_byte_count_; On 2017/06/08 23:37:18, ncarter (slow) wrote: > Let's try to reduce the number of fields here to as few as possible -- I think > four should suffice, and we can interpolate the rest: > > last_refresh_cumulative_bytes_sent_ > cumulative_bytes_sent_ > last_refresh_cumulative_bytes_read_ > cumulative_bytes_read_ > (maybe we need to store a TimeDelta too, but that's less clear to me) > > the rate can be determined then by (last_refresh_cumulative - cumulative) / > (time interval), and the combined totals can just be computed by adding (sent + > read). > > It's fine to expose a getter_function() that does simple arithmetic (e.g. to get > the totals). Done. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_impl.cc (right): https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.cc:485: void TaskManagerImpl::OnMultipleBytesSentUI( On 2017/06/08 23:37:18, ncarter (slow) wrote: > Once BytesTransferedParam holds both send and read data, merge this with > OnMultipleBytesReadUI. > > Possible name suggestion: AscribeNetworkUsageOnUI? Done. Named it OnMultipleBytesTransfered to try to keep parallel naming and if anyone is searching or grepping around for OnMultipleBytes looking for the old name something will still come up. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.cc:569: bool TaskManagerImpl::UpdateTasksWithBytesRead( On 2017/06/08 23:37:18, ncarter (slow) wrote: > Merge this function with TaskManagerImpl::UpdateTasksWithBytesSent. > (UpdateTasksWithNetworkUsage ? ) Done. I went with UpdateTasksWithBytesTransfered to try to keep naming predictable for searching. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.cc:576: task->OnNetworkBytesRead(param.byte_count); On 2017/06/08 23:37:18, ncarter (slow) wrote: > This might be the natural spot to split off into separate Read/Sent methods. > We've already looked up the Task, so we can just have two calls here: > > if (task) { > task->OnNetworkBytesRead(param.bytes_read); > task->OnNetworkBytesSent(param.bytes_sent); > return true; > } Done. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_impl.h (right): https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_impl.h:105: static void OnMultipleBytesSentUI(std::vector<BytesTransferedParam>* params); On 2017/06/08 23:37:18, ncarter (slow) wrote: > Because the PostTask operation itself is a few heap operations, it's probably > better not to have two functions here, and instead to make BytesTransferedParam > have both |bytes_read| and |bytes_sent| fields. > > It should be less memory this way, and hopefully less code too. Done. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper.cc (right): https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.cc:131: On 2017/06/08 23:37:19, ncarter (slow) wrote: > There's a potential optimization here we can consider -- if bytes_read_buffer_ > is non-empty, and bytes_read_buffer_.back() has a match for (origin_pid, > child_id, route_id), then we can update (operator+=) the existing entry rather > than pushing a new one. > > We could search the entire vector too, but I'm thinking that the common case > here is a burst of requests from one client. Especially because every request > has a little bit of send, this may prevent this buffer from getting > problematically bigger as a result of what we're adding. > > Most efficient would be to make this a std::unordered_set<> (defining a custom > hasher for BytesTransferedParam) or std::unordered_map<> (using std::tuple as > the keys, and maybe eliminating BytesTransferedParam altogether). But if we go > that route, it probably makes sense to tackle it as a follow-on change. So leave > this as a vector for now. Done. https://codereview.chromium.org/2905403002/diff/100001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.cc:143: bytes_sent_buffer_.swap(*bytes_sent_buffer); On 2017/06/08 23:37:19, ncarter (slow) wrote: > Because this whole dance with the vectors, swapping, and the PostDelayedTask is > actually a very subtle optimization, it's worthwhile to have only one > implementation of it. Done.
One bug, one suggestion for expanding the test, and one idea for reducing the code that deals with the "-1" sentinel value. The rest of the feedback is just comment nits. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/browser_process_task_unittest.cc (right): https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:160: EXPECT_EQ(sent_bytes + received_bytes, provided_task_->network_usage_rate()); To really validate that cumulative is "since the beginning of time" and rate is "just the last Refresh cycle", we ought to do a second refresh cycle in at least one of these tests. Try this (though maybe you want to spread it out into multiple tests): const int batch_one_bytes = sent_bytes + received_bytes; EXPECT_EQ(batch_one_bytes, provided_task_->cumulative_network_usage()); EXPECT_EQ(batch_one_bytes, provided_task_->network_usage_rate()); // Some more events are reported by the network stack. The rate should be // determined only by these events, but cumulative should include the first // cycle. int batch_two_bytes = 0; for (int sent : {1241, 26424}) { batch_two_bytes += sent; provided_task_->OnNetworkBytesSent(sent); } for (int read : {55, 56, 889}) { batch_two_bytes += read; provided_task_->OnNetworkBytesSent(read); } // Until Refresh() occurs, the task should still report stats for the original // batch. EXPECT_EQ(batch_one_bytes, provided_task_->cumulative_network_usage()); EXPECT_EQ(batch_one_bytes, provided_task_->network_usage_rate()); // Do a refresh with a 2-second update time. provided_task_->Refresh(base::TimeDelta::FromSeconds(1), REFRESH_TYPE_NETWORK_USAGE); // After Refresh(), the cumulative network usage should reflect a combination // of both batches. EXPECT_EQ(batch_one_bytes + batch_two_bytes, provided_task_->cumulative_network_usage()); // But the instantaneous rate should reflect only what happened in the // previous refresh window. EXPECT_EQ(batch_two_bytes, provided_task_->network_usage_rate()); https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:174: provided_task_->Refresh(base::TimeDelta::FromSeconds(2), 2 -> refresh_secs https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:177: EXPECT_EQ(received_bytes, provided_task_->cumulative_network_usage()); EXPECT_EQ(received_bytes / refresh_secs, provided_task_->network_usage_rate()); https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:191: provided_task_->Refresh(base::TimeDelta::FromSeconds(2), 2 -> refresh_secs https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:194: EXPECT_EQ(sent_bytes, provided_task_->cumulative_network_usage()); EXPECT_EQ(sent_bytes / refresh_secs, provided_task_->network_usage_rate()); https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/task.cc (right): https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.cc:35: cumulative_bytes_sent_(-1), I have a hunch that this code would come out easier if we started cumulative_bytes_sent_ and cumulative_bytes_read_ off at zero. Then we're in a state where we never actually have to store the -1 sentinel value, which means we don't have to special case that value. The original meaning of a return value of "-1" vs "0" for the rate getter is so that the caller can distinguish between a task that's never had any network usage reported, and a task that just happens to not have any data in the last refresh cycle. If that's important (and it's not clear to me that it is!), we can just do something like this in the rate getter: return ReportsNetworkUsage() ? network_sent_rate_ + network_read_rate_ : -1; And I think that would be the only place in this class where we have to deal with the -1 special value. ReportsNetworkUsage() can detect the "ever got any bytes" case like this: return last_refresh_cumulative_bytes_sent_ || last_refresh_cumulative_bytes_read_; https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.cc:178: // the network has been used I think this comment is probably better moved to inside the {}'s, since it's really explaining the meaning of the expression. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/task.h (right): https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:88: // |current_read_byte_count_| in this refresh cycle. |current_read_byte_count_| no longer exists. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:93: // |current_sent_byte_count_| in this refresh cycle. |current_sent_byte_count_| no longer exists. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:150: // Returns the instantaneous rate, in bytes per second, of network usage (sent and recieved), as measured over the last refresh cycle. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:155: } These methods aren't quite trivial, so they should be declared in the .h file and defined in the .cc file, per the rule here: https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i... However, if you take my later suggestion of not starting cumulative_bytes_read_ off at -1, this method probably becomes trivial enough (cumulative_bytes_sent_ + cumulative_bytes_read_) to keep in the .h file. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:156: // Returns the cumulative number of bytes of network usage (sent and received) since this task was created. This value is monotonically increasing over the lifetime of the task. ... and depending on how you resolve my next comment, maybe clarify to add something like: This value is updated each time Refresh() is called. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:162: return cumulative_bytes_sent + cumulative_bytes_read; Should these be based on cumulative_bytes_read_ or last_refresh_cumulative_bytes_read_ ? It's not actually totally clear to me. We could prefer the freshest values, or we could provide a stable value that only updates at Refresh() time. I could go either way. What do you think? https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:188: // Keeps track of current number of bytes sent on a refresh After you finalize the code comment for cumulative_network_usage(), use the same language down here. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:197: // Keeps track of the current network sending rate In all of these comments, the "Keeps track of" can be dropped. Match the style of the existing data member comments, including ending with a period. For this one, I'd write something like: // The download rate (in bytes per second) for this task during the most recent refresh window. Often it's useful to phrase comments in terms alternate to those used in the variable name (e.g. "download" instead of "read"), because otherwise you wind up with comments like the following, that add no signal: // The network read rate. network_read_rate_; https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:169: per_process_network_usage_rate_ = network_usage_refresh_enabled ? 0 : -1; Don't you need to zero out cumulative_per_process_network_usage_ here? Otherwise it'll grow quadratically? Would be nice to see a task_group_unittest case that catches this (you could consider moving the test scenarios you already have, from browser_process_task_unittest.cc to task_group_unittest)
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: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
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...
ptal https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/browser_process_task_unittest.cc (right): https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:160: EXPECT_EQ(sent_bytes + received_bytes, provided_task_->network_usage_rate()); On 2017/06/14 20:20:58, ncarter (slow) wrote: > To really validate that cumulative is "since the beginning of time" and rate is > "just the last Refresh cycle", we ought to do a second refresh cycle in at least > one of these tests. Try this (though maybe you want to spread it out into > multiple tests): > > const int batch_one_bytes = sent_bytes + received_bytes; > EXPECT_EQ(batch_one_bytes, provided_task_->cumulative_network_usage()); > EXPECT_EQ(batch_one_bytes, provided_task_->network_usage_rate()); > > // Some more events are reported by the network stack. The rate should be > // determined only by these events, but cumulative should include the first > // cycle. > int batch_two_bytes = 0; > for (int sent : {1241, 26424}) { > batch_two_bytes += sent; > provided_task_->OnNetworkBytesSent(sent); > } > > for (int read : {55, 56, 889}) { > batch_two_bytes += read; > provided_task_->OnNetworkBytesSent(read); > } > > // Until Refresh() occurs, the task should still report stats for the original > // batch. > EXPECT_EQ(batch_one_bytes, provided_task_->cumulative_network_usage()); > EXPECT_EQ(batch_one_bytes, provided_task_->network_usage_rate()); > > // Do a refresh with a 2-second update time. > provided_task_->Refresh(base::TimeDelta::FromSeconds(1), > REFRESH_TYPE_NETWORK_USAGE); > > // After Refresh(), the cumulative network usage should reflect a combination > // of both batches. > EXPECT_EQ(batch_one_bytes + batch_two_bytes, > provided_task_->cumulative_network_usage()); > // But the instantaneous rate should reflect only what happened in the > // previous refresh window. > EXPECT_EQ(batch_two_bytes, provided_task_->network_usage_rate()); I've added some tests that cover this case. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:174: provided_task_->Refresh(base::TimeDelta::FromSeconds(2), On 2017/06/14 20:20:58, ncarter (slow) wrote: > 2 -> refresh_secs Done. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:177: EXPECT_EQ(received_bytes, provided_task_->cumulative_network_usage()); On 2017/06/14 20:20:58, ncarter (slow) wrote: > EXPECT_EQ(received_bytes / refresh_secs, provided_task_->network_usage_rate()); Done. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:191: provided_task_->Refresh(base::TimeDelta::FromSeconds(2), On 2017/06/14 20:20:58, ncarter (slow) wrote: > 2 -> refresh_secs Done. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:194: EXPECT_EQ(sent_bytes, provided_task_->cumulative_network_usage()); On 2017/06/14 20:20:58, ncarter (slow) wrote: > EXPECT_EQ(sent_bytes / refresh_secs, provided_task_->network_usage_rate()); Done. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/task.cc (right): https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.cc:35: cumulative_bytes_sent_(-1), On 2017/06/14 20:20:58, ncarter (slow) wrote: > I have a hunch that this code would come out easier if we started > cumulative_bytes_sent_ and cumulative_bytes_read_ off at zero. Then we're in a > state where we never actually have to store the -1 sentinel value, which means > we don't have to special case that value. > > The original meaning of a return value of "-1" vs "0" for the rate getter is so > that the caller can distinguish between a task that's never had any network > usage reported, and a task that just happens to not have any data in the last > refresh cycle. If that's important (and it's not clear to me that it is!), we > can just do something like this in the rate getter: > > return ReportsNetworkUsage() ? network_sent_rate_ + network_read_rate_ : -1; > > And I think that would be the only place in this class where we have to deal > with the -1 special value. > > ReportsNetworkUsage() can detect the "ever got any bytes" case like this: > > return last_refresh_cumulative_bytes_sent_ || > last_refresh_cumulative_bytes_read_; I agree that this makes more sense. I am unsure if ReportsNetworkUsage() should be tied to last_refresh_cumulative_bytes_read_ or cumulative_bytes_read_ I am leaning towards the last_refresh_cumulative_bytes_read_ because in the old version it is tied into network_usage_ which is only ever updated after a Refresh. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.cc:178: // the network has been used On 2017/06/14 20:20:58, ncarter (slow) wrote: > I think this comment is probably better moved to inside the {}'s, since it's > really explaining the meaning of the expression. Deleted since we removed the -1 flag. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/task.h (right): https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:88: // |current_read_byte_count_| in this refresh cycle. On 2017/06/14 20:20:59, ncarter (slow) wrote: > |current_read_byte_count_| no longer exists. Done. Changed to |cumulative_read_bytes_| as is done in the function https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:93: // |current_sent_byte_count_| in this refresh cycle. On 2017/06/14 20:20:59, ncarter (slow) wrote: > |current_sent_byte_count_| no longer exists. Done. Changed to |cumulative_sent_bytes_| as is done in the function https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:150: On 2017/06/14 20:20:59, ncarter (slow) wrote: > // Returns the instantaneous rate, in bytes per second, of network usage (sent > and recieved), as measured over the last refresh cycle. Agreed and added. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:155: } On 2017/06/14 20:20:59, ncarter (slow) wrote: > These methods aren't quite trivial, so they should be declared in the .h file > and defined in the .cc file, per the rule here: > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i... > > > However, if you take my later suggestion of not starting cumulative_bytes_read_ > off at -1, this method probably becomes trivial enough (cumulative_bytes_sent_ + > cumulative_bytes_read_) to keep in the .h file. I agree with the later suggestion and simplified these based on the impact of the changes. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:156: On 2017/06/14 20:20:59, ncarter (slow) wrote: > // Returns the cumulative number of bytes of network usage (sent and received) > since this task was created. This value is monotonically increasing over the > lifetime of the task. > > ... and depending on how you resolve my next comment, maybe clarify to add > something like: > > This value is updated each time Refresh() is called. I have kept it to be as up to date as possible. I looked at it with the last refresh as what is returned and the naming conventions just felt off to me. I am not sold that the most up to date way is the best way to do this, but the names "feel" more correct as this way. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:162: return cumulative_bytes_sent + cumulative_bytes_read; On 2017/06/14 20:20:59, ncarter (slow) wrote: > Should these be based on cumulative_bytes_read_ or > last_refresh_cumulative_bytes_read_ ? It's not actually totally clear to me. We > could prefer the freshest values, or we could provide a stable value that only > updates at Refresh() time. I could go either way. > > What do you think? I think it should be the most up to date information we can provide, though I think use is what matters too. If this were to be used to display information in the task manager in the future I think switching to last_refresh_cumulative_bytes_* make more sense because it will be in sync with the refreshes on the task manager. Also from the names of the variables I think it makes more sense to keep it aligned with what might be expected from just reading the names. I could see adding in a getter for the last_refresh_cumulative_ for testing as well. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:188: // Keeps track of current number of bytes sent on a refresh On 2017/06/14 20:20:58, ncarter (slow) wrote: > After you finalize the code comment for cumulative_network_usage(), use the same > language down here. Updated the language. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:197: // Keeps track of the current network sending rate On 2017/06/14 20:20:59, ncarter (slow) wrote: > In all of these comments, the "Keeps track of" can be dropped. Match the style > of the existing data member comments, including ending with a period. > > For this one, I'd write something like: > > // The download rate (in bytes per second) for this task during the most recent > refresh window. > > Often it's useful to phrase comments in terms alternate to those used in the > variable name (e.g. "download" instead of "read"), because otherwise you wind up > with comments like the following, that add no signal: > > // The network read rate. > network_read_rate_; Updated the comments to have more signals to the purpose of the variables. https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2905403002/diff/200001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:169: per_process_network_usage_rate_ = network_usage_refresh_enabled ? 0 : -1; On 2017/06/14 20:20:59, ncarter (slow) wrote: > Don't you need to zero out cumulative_per_process_network_usage_ here? Otherwise > it'll grow quadratically? > > Would be nice to see a task_group_unittest case that catches this (you could > consider moving the test scenarios you already have, from > browser_process_task_unittest.cc to task_group_unittest) We absolutely do, and I will move those tests over and add one to get coverage on this case.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/browser_process_task_unittest.cc (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:82: REFRESH_TYPE_NETWORK_USAGE); Now that the main tests of network-usage functionality have moved out of this class, it makes sense to restore the original assertion here: EXPECT_EQ(received_bytes, provided_task_->network_usage_rate()); It works as a "sanity check that the browser process can show network usage". https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/task.h (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:149: // (sent and recieved), as measured over the last refresh cycle. recieved -> received (I always get this wrong) https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:153: // are 0 and ReportsNetworkUsage is also false. Delete these three comment lines. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:158: // over the tasks lifetime. It is calculated independantly of refreshes and tasks -> task's independantly -> independently https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:200: // The upload rate (in bytes per second) for htis task during the latest htis -> this https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group.h (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.h:170: // The total(sum) network usage in bytes as the sum of all network usages of "A continuously updating sum of all bytes that have been downloaded and uploaded by all tasks in this process." (to be consistent with the language you used in task.h) https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group_unittest.cc (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group_unittest.cc:346: TEST_F(TaskGroupTest, NetworkBytesTransferedRefreshOutOfOrder) { "transfered" -> "transferred" throughout (I'd thought one-r was an acceptable alternative spelling, but it looks like double-r is the correct spelling for both british and american english). According to stackoverflow, this might work (but 'git commit' your work first, in case it destroys your client): git diff --name-only @{u} | xargs sed -i 's/ransfered/ransferred/g'
https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper.cc (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.cc:65: bytes_sent); You can also consider using this style: OnNetworkBytesTransfered(request, bytes_read, 0 /* bytes_sent */) https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:22: // reception of bytes read notifications. "bytes read" is still read-specific https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/task_manager_browsertest.cc (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_browsertest.cc:701: WaitForTaskManagerStatToExceed(MatchTab(test_gurl.GetContent().c_str()), Just use MatchTab("title1.html") -- it's consistent with what the existing tests do. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_browsertest.cc:702: ColumnSpecifier::NETWORK_USE, 16000000)); I worry that this might be flaky, for two reasons: - The upload is done by the time ExecuteScriptAndExtractString returns, so it's possible that the TaskManager has already shown the network use and refreshed again. - It seems like it would be incorrect if the network service actually gave us regular reports, rather than all-at-once. I think you can likely fix the first case (not the second) by means of the following recipe: 1. Don't bother with the domAutomationController.send(). You just wait for the TaskManager stat, so you don't a result at all. 2. Instead of content::ExecuteScript, use: browser()->tab_strip_model()->GetActiveWebContents()->GetMainFrame()->ExecuteJavaScriptForTests(UTF8ToUTF16(script)); The difference is that this is a nonblocking call -- it doesn't run the message loop waiting for the script to complete, it just sends a message to the renderer and returns. (Look at the definition of content::ExecuteScript, and you'll see it's implemented in terms of this function). So it's not possible for the TaskManager's state to have changed in the interim, which means that you couldn't have missed anything inside of WaitForStatToExeceed. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_browsertest.cc:711: mem = new Uint8Array(16 << 20); "var mem =" https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_browsertest.cc:732: mem[i] = i; mem already exists from the previous script execution, so you don't need to set it up a second time. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_browsertest.cc:738: setTimeout( function(){fetch(request) Use lambda syntax here, since it's used elsewhere in the statement. "function(){" becomes "() => {" https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_browsertest.cc:742: 2000); What's the importance of this two-second wait? (Is it just to exceed the nyquist ratio?) If so, it's maybe worth documenting with a comment. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/task_manager_interface.h (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_interface.h:202: // means network usage calculation refresh is currently not available. Remove the last sentence of this comment -- task_manager_impl.cc doesn't seem to ever return -1 anymore. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/test_task_manager.cc (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/test_task_manager.cc:151: return -1; return 0; (afaik the real implementation of this doesn't ever return -1 anymore, so this shouldn't either)
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...
ptal https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/browser_process_task_unittest.cc (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/providers/browser_process_task_unittest.cc:82: REFRESH_TYPE_NETWORK_USAGE); On 2017/06/16 22:28:30, ncarter (slow) wrote: > Now that the main tests of network-usage functionality have moved out of this > class, it makes sense to restore the original assertion here: > > EXPECT_EQ(received_bytes, provided_task_->network_usage_rate()); > > It works as a "sanity check that the browser process can show network usage". Done. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/task.h (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:149: // (sent and recieved), as measured over the last refresh cycle. On 2017/06/16 22:28:30, ncarter (slow) wrote: > recieved -> received > > (I always get this wrong) Done. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:153: // are 0 and ReportsNetworkUsage is also false. On 2017/06/16 22:28:30, ncarter (slow) wrote: > Delete these three comment lines. Done. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:158: // over the tasks lifetime. It is calculated independantly of refreshes and On 2017/06/16 22:28:30, ncarter (slow) wrote: > tasks -> task's > independantly -> independently Done. I also have sublime doing spell checking on comments now (my apologies, I should have done that after the last round). https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:200: // The upload rate (in bytes per second) for htis task during the latest On 2017/06/16 22:28:30, ncarter (slow) wrote: > htis -> this Done. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group.h (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.h:170: // The total(sum) network usage in bytes as the sum of all network usages of On 2017/06/16 22:28:30, ncarter (slow) wrote: > "A continuously updating sum of all bytes that have been downloaded and uploaded > by all tasks in this process." > > (to be consistent with the language you used in task.h) Done. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group_unittest.cc (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group_unittest.cc:346: TEST_F(TaskGroupTest, NetworkBytesTransferedRefreshOutOfOrder) { On 2017/06/16 22:28:30, ncarter (slow) wrote: > "transfered" -> "transferred" throughout > > (I'd thought one-r was an acceptable alternative spelling, but it looks like > double-r is the correct spelling for both british and american english). > > According to stackoverflow, this might work (but 'git commit' your work first, > in case it destroys your client): > > git diff --name-only @{u} | xargs sed -i 's/ransfered/ransferred/g' Done, it looks like it didn't destroy everything. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper.cc (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.cc:65: bytes_sent); On 2017/06/17 00:21:01, ncarter (slow) wrote: > You can also consider using this style: > > OnNetworkBytesTransfered(request, bytes_read, 0 /* bytes_sent */) Acknowledged. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_manager_io_thread_helper.h:22: // reception of bytes read notifications. On 2017/06/17 00:21:01, ncarter (slow) wrote: > "bytes read" is still read-specific Done. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/task_manager_browsertest.cc (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_browsertest.cc:701: WaitForTaskManagerStatToExceed(MatchTab(test_gurl.GetContent().c_str()), On 2017/06/17 00:21:02, ncarter (slow) wrote: > Just use MatchTab("title1.html") -- it's consistent with what the existing tests > do. Done. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_browsertest.cc:702: ColumnSpecifier::NETWORK_USE, 16000000)); On 2017/06/17 00:21:02, ncarter (slow) wrote: > I worry that this might be flaky, for two reasons: > > - The upload is done by the time ExecuteScriptAndExtractString returns, so it's > possible that the TaskManager has already shown the network use and refreshed > again. > - It seems like it would be incorrect if the network service actually gave us > regular reports, rather than all-at-once. > > I think you can likely fix the first case (not the second) by means of the > following recipe: > > 1. Don't bother with the domAutomationController.send(). You just wait for the > TaskManager stat, so you don't a result at all. > 2. Instead of content::ExecuteScript, use: > > > browser()->tab_strip_model()->GetActiveWebContents()->GetMainFrame()->ExecuteJavaScriptForTests(UTF8ToUTF16(script)); > > The difference is that this is a nonblocking call -- it doesn't run the message > loop waiting for the script to complete, it just sends a message to the renderer > and returns. (Look at the definition of content::ExecuteScript, and you'll see > it's implemented in terms of this function). So it's not possible for the > TaskManager's state to have changed in the interim, which means that you > couldn't have missed anything inside of WaitForStatToExeceed. Done. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_browsertest.cc:711: mem = new Uint8Array(16 << 20); On 2017/06/17 00:21:02, ncarter (slow) wrote: > "var mem =" Done. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_browsertest.cc:732: mem[i] = i; On 2017/06/17 00:21:01, ncarter (slow) wrote: > mem already exists from the previous script execution, so you don't need to set > it up a second time. Done. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_browsertest.cc:738: setTimeout( function(){fetch(request) On 2017/06/17 00:21:02, ncarter (slow) wrote: > Use lambda syntax here, since it's used elsewhere in the statement. > > "function(){" becomes "() => {" Done. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_browsertest.cc:742: 2000); On 2017/06/17 00:21:01, ncarter (slow) wrote: > What's the importance of this two-second wait? (Is it just to exceed the nyquist > ratio?) > > If so, it's maybe worth documenting with a comment. Done. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/task_manager_interface.h (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_interface.h:202: // means network usage calculation refresh is currently not available. On 2017/06/17 00:21:02, ncarter (slow) wrote: > Remove the last sentence of this comment -- task_manager_impl.cc doesn't seem to > ever return -1 anymore. Done. https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... File chrome/browser/task_manager/test_task_manager.cc (right): https://codereview.chromium.org/2905403002/diff/260001/chrome/browser/task_ma... chrome/browser/task_manager/test_task_manager.cc:151: return -1; On 2017/06/17 00:21:02, ncarter (slow) wrote: > return 0; > > (afaik the real implementation of this doesn't ever return -1 anymore, so this > shouldn't either) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
task_manager lgtm w/ a couple nits https://codereview.chromium.org/2905403002/diff/300001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/task.h (right): https://codereview.chromium.org/2905403002/diff/300001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:149: // (sent and recieved), as measured over the last refresh cycle. received is still typo https://codereview.chromium.org/2905403002/diff/300001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2905403002/diff/300001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:170: cumulative_per_process_network_usage_ = 0; Hmm. If a Task goes away (say a tab closes) but the TaskGroup remains, it actually looks like we would encounter a case here where cumulative_per_process_network_usage_ would decrease, since we'd basically forget its contribution. This may be a benign bug, since we only collect this value for testing currently. Maybe the thing to do here is not reset this to zero each loop, and inside the loop, add in the per-task delta? Since it's benign, it would be okay to fix this in a follow-on patch set if you like. https://codereview.chromium.org/2905403002/diff/300001/chrome/browser/task_ma... File chrome/browser/task_manager/task_manager_browsertest.cc (right): https://codereview.chromium.org/2905403002/diff/300001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_browsertest.cc:697: ->ExecuteJavaScriptForTests(base::UTF8ToUTF16(test_js)); Add in: // TODO(cburn): The assertion below currently assumes that the rate contribution of the entire 16MB upload arrives in a single refresh cycle. That's true now because it's only reported when the transaction completes, but if that changes in the future, this assertion may need to change. (btw -- this is totally academic -- if we change it to something, one idea is to assert that the peak reported network use exceeded (16 << 20) / elapsed_time, where elapsed_time for the period that we were waiting. by, like, [waves hands vigorously] the mean value theorem?) https://codereview.chromium.org/2905403002/diff/300001/chrome/browser/task_ma... File chrome/browser/task_manager/task_manager_interface.h (right): https://codereview.chromium.org/2905403002/diff/300001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_interface.h:198: // means network usage calculation refresh is currently not available. GetCumulativeProcessTotalNetworkUsage doesn't seem to ever return -1. (GetProcessTotalNetworkUsage does return -1, however)
Also: please update your change description now that the scope of your change is complete. Write with an intended audience of: - A sheriff, who might read this in the hour after your change lands, and is trying to figure out which change might have caused a bot failure that's closed the tree. - Yourself, months from now, when trying to summarize what impact you've had. - An unknown engineer, a week or several years from now, looking at revision history and trying to understand how & why this code evolved.
Description was changed from ========== This plumbs network upload bytes to the task manager also changed BytesReadParam to BytesTransferParam for readability BUG=720773 ========== to ========== This adds the plumbing needed to take data about completed outbound network traffic to the task manager for tracking network usage. There are a number of naming changes to help distinguish between what is and isn't a rate. This update changes the struct BytesReadParam to BytesTrasferedParam and stores both bytes read and bytes sent. It updates how the struct is processed to handle the additional tracking of sent bytes. Tasks have been changed to store the cumulative_bytes_read_ and cumulative_bytes_sent_ which are stored on Refresh() to the last_refresh_cumulative_bytes_sent_ and last_refresh_cumulative_bytes_read_ . These are used to calculate network_sent_rate_ and network_read_rate_ which are summed for the network_usage_rate_. Tasks no longer store a network usage of -1 to signify that they have not had any network traffic, the utility of that flag no longer appears to be used so it was removed. Tasks and Task Groups can now report their cumulative network usage. BUG=720773 ==========
Description was changed from ========== This adds the plumbing needed to take data about completed outbound network traffic to the task manager for tracking network usage. There are a number of naming changes to help distinguish between what is and isn't a rate. This update changes the struct BytesReadParam to BytesTrasferedParam and stores both bytes read and bytes sent. It updates how the struct is processed to handle the additional tracking of sent bytes. Tasks have been changed to store the cumulative_bytes_read_ and cumulative_bytes_sent_ which are stored on Refresh() to the last_refresh_cumulative_bytes_sent_ and last_refresh_cumulative_bytes_read_ . These are used to calculate network_sent_rate_ and network_read_rate_ which are summed for the network_usage_rate_. Tasks no longer store a network usage of -1 to signify that they have not had any network traffic, the utility of that flag no longer appears to be used so it was removed. Tasks and Task Groups can now report their cumulative network usage. BUG=720773 ========== to ========== This adds the plumbing needed to take data about completed outbound network traffic to the task manager for tracking network usage. There are a number of naming changes to help distinguish between what is and isn't a rate. This update changes the struct BytesReadParam to BytesTrasferedParam and stores both bytes read and bytes sent. It updates how the struct is processed to handle the additional tracking of sent bytes. Tasks have been changed to store the cumulative_bytes_read_ and cumulative_bytes_sent_ which are stored on Refresh() to the last_refresh_cumulative_bytes_sent_ and last_refresh_cumulative_bytes_read_ . These are used to calculate network_sent_rate_ and network_read_rate_ which are summed for the network_usage_rate_. Tasks no longer store a network usage of -1 to signify that they have not had any network traffic, the utility of that flag no longer appears to be used so it was removed. Tasks and Task Groups can now report their cumulative network usage. BUG=720773 ==========
Description was changed from ========== This adds the plumbing needed to take data about completed outbound network traffic to the task manager for tracking network usage. There are a number of naming changes to help distinguish between what is and isn't a rate. This update changes the struct BytesReadParam to BytesTrasferedParam and stores both bytes read and bytes sent. It updates how the struct is processed to handle the additional tracking of sent bytes. Tasks have been changed to store the cumulative_bytes_read_ and cumulative_bytes_sent_ which are stored on Refresh() to the last_refresh_cumulative_bytes_sent_ and last_refresh_cumulative_bytes_read_ . These are used to calculate network_sent_rate_ and network_read_rate_ which are summed for the network_usage_rate_. Tasks no longer store a network usage of -1 to signify that they have not had any network traffic, the utility of that flag no longer appears to be used so it was removed. Tasks and Task Groups can now report their cumulative network usage. BUG=720773 ========== to ========== This adds the plumbing needed to take data about completed outbound network traffic to the task manager for tracking network usage. There are a number of naming changes to help distinguish between what is and isn't a rate. This update changes the struct BytesReadParam to BytesTrasferedParam and stores both bytes read and bytes sent. It updates how the struct is processed to handle the additional tracking of sent bytes. Tasks have been changed to store the cumulative_bytes_read_ and cumulative_bytes_sent_ which are stored on Refresh() to the last_refresh_cumulative_bytes_sent_ and last_refresh_cumulative_bytes_read_ . These are used to calculate network_sent_rate_ and network_read_rate_ which are summed for the network_usage_rate_. Tasks no longer store a network usage of -1 to signify that they have not had any network traffic, the utility of that flag no longer appears to be used so it was removed. Because of this ReportsNetworkUsage() has been removed. Tasks and Task Groups can now report their cumulative network usage. BUG=720773 ==========
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 adds the plumbing needed to take data about completed outbound network traffic to the task manager for tracking network usage. There are a number of naming changes to help distinguish between what is and isn't a rate. This update changes the struct BytesReadParam to BytesTrasferedParam and stores both bytes read and bytes sent. It updates how the struct is processed to handle the additional tracking of sent bytes. Tasks have been changed to store the cumulative_bytes_read_ and cumulative_bytes_sent_ which are stored on Refresh() to the last_refresh_cumulative_bytes_sent_ and last_refresh_cumulative_bytes_read_ . These are used to calculate network_sent_rate_ and network_read_rate_ which are summed for the network_usage_rate_. Tasks no longer store a network usage of -1 to signify that they have not had any network traffic, the utility of that flag no longer appears to be used so it was removed. Because of this ReportsNetworkUsage() has been removed. Tasks and Task Groups can now report their cumulative network usage. BUG=720773 ========== to ========== This adds the plumbing needed to take data about completed outbound network traffic to the task manager for tracking network usage. There are a number of naming changes to help distinguish between what is and isn't a rate and what is sent vs read vs transferred. Read being incoming bytes, sent being outgoing bytes and transfered being either read or sent bytes. This update changes the struct BytesReadParam to BytesTrasferedParam and stores both bytes read and bytes sent. It updates how the struct is processed to handle the additional tracking of sent bytes. Tasks have been changed to store the cumulative_bytes_read_ and cumulative_bytes_sent_ which are stored on Refresh() to the last_refresh_cumulative_bytes_sent_ and last_refresh_cumulative_bytes_read_ . These are used to calculate network_sent_rate_ and network_read_rate_ which are summed for the network_usage_rate_. Tasks no longer store a network usage of -1 to signify that they have not had any network traffic, the utility of that flag no longer appears to be used so it was removed. Because of this ReportsNetworkUsage() has been removed. Tasks and Task Groups can now report their cumulative network usage. BUG=720773 ==========
Description was changed from ========== This adds the plumbing needed to take data about completed outbound network traffic to the task manager for tracking network usage. There are a number of naming changes to help distinguish between what is and isn't a rate and what is sent vs read vs transferred. Read being incoming bytes, sent being outgoing bytes and transfered being either read or sent bytes. This update changes the struct BytesReadParam to BytesTrasferedParam and stores both bytes read and bytes sent. It updates how the struct is processed to handle the additional tracking of sent bytes. Tasks have been changed to store the cumulative_bytes_read_ and cumulative_bytes_sent_ which are stored on Refresh() to the last_refresh_cumulative_bytes_sent_ and last_refresh_cumulative_bytes_read_ . These are used to calculate network_sent_rate_ and network_read_rate_ which are summed for the network_usage_rate_. Tasks no longer store a network usage of -1 to signify that they have not had any network traffic, the utility of that flag no longer appears to be used so it was removed. Because of this ReportsNetworkUsage() has been removed. Tasks and Task Groups can now report their cumulative network usage. BUG=720773 ========== to ========== This adds the plumbing needed to take data about completed outbound network traffic to the task manager for tracking network usage. There are a number of naming changes to help distinguish between what is and isn't a rate and what is sent vs read vs transferred. Read being incoming bytes, sent being outgoing bytes and transfered being either read or sent bytes. This update changes the struct BytesReadParam to BytesTrasferedParam and stores both bytes read and bytes sent. It updates how the struct is processed to handle the additional tracking of sent bytes. Tasks have been changed to store the cumulative_bytes_read_ and cumulative_bytes_sent_ which are stored on Refresh() to the last_refresh_cumulative_bytes_sent_ and last_refresh_cumulative_bytes_read_ . These are used to calculate network_sent_rate_ and network_read_rate_ which are summed for the network_usage_rate_. Tasks no longer store a network usage of -1 to signify that they have not had any network traffic, the utility of that flag no longer appears to be used so it was removed. Because of this ReportsNetworkUsage() has been removed. Tasks and Task Groups can now report their cumulative network usage. BUG=720773 ==========
Description was changed from ========== This adds the plumbing needed to take data about completed outbound network traffic to the task manager for tracking network usage. There are a number of naming changes to help distinguish between what is and isn't a rate and what is sent vs read vs transferred. Read being incoming bytes, sent being outgoing bytes and transfered being either read or sent bytes. This update changes the struct BytesReadParam to BytesTrasferedParam and stores both bytes read and bytes sent. It updates how the struct is processed to handle the additional tracking of sent bytes. Tasks have been changed to store the cumulative_bytes_read_ and cumulative_bytes_sent_ which are stored on Refresh() to the last_refresh_cumulative_bytes_sent_ and last_refresh_cumulative_bytes_read_ . These are used to calculate network_sent_rate_ and network_read_rate_ which are summed for the network_usage_rate_. Tasks no longer store a network usage of -1 to signify that they have not had any network traffic, the utility of that flag no longer appears to be used so it was removed. Because of this ReportsNetworkUsage() has been removed. Tasks and Task Groups can now report their cumulative network usage. BUG=720773 ========== to ========== This adds the plumbing needed to take data about completed outbound network traffic to the task manager for tracking network usage. There are a number of naming changes to help distinguish between what is and isn't a "rate" and what is sent vs read vs transferred. Read being incoming bytes, sent being outgoing bytes and transfered being either read or sent bytes. The "rates" are not calculated based on what is live on the network. It is instead based on when requests are completed so large uploads will all be registered in one refresh. This update changes the struct BytesReadParam to BytesTrasferedParam and stores both bytes read and bytes sent. It updates how the struct is processed to handle the additional tracking of sent bytes. Tasks have been changed to store the cumulative_bytes_read_ and cumulative_bytes_sent_ which are stored on Refresh() to the last_refresh_cumulative_bytes_sent_ and last_refresh_cumulative_bytes_read_ . These are used to calculate network_sent_rate_ and network_read_rate_ which are summed for the network_usage_rate_. Tasks no longer store a network usage of -1 to signify that they have not had any network traffic, the utility of that flag no longer appears to be used so it was removed. Because of this ReportsNetworkUsage() has been removed. Tasks and Task Groups can now report their cumulative network usage. BUG=720773 ==========
cburn@google.com changed reviewers: + mmenke@chromium.org
mmenke, please review chrome/browser/net https://codereview.chromium.org/2905403002/diff/300001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/task.h (right): https://codereview.chromium.org/2905403002/diff/300001/chrome/browser/task_ma... chrome/browser/task_manager/providers/task.h:149: // (sent and recieved), as measured over the last refresh cycle. On 2017/06/20 19:52:27, ncarter (slow) wrote: > received is still typo Done. https://codereview.chromium.org/2905403002/diff/300001/chrome/browser/task_ma... File chrome/browser/task_manager/sampling/task_group.cc (right): https://codereview.chromium.org/2905403002/diff/300001/chrome/browser/task_ma... chrome/browser/task_manager/sampling/task_group.cc:170: cumulative_per_process_network_usage_ = 0; On 2017/06/20 19:52:27, ncarter (slow) wrote: > Hmm. If a Task goes away (say a tab closes) but the TaskGroup remains, it > actually looks like we would encounter a case here where > cumulative_per_process_network_usage_ would decrease, since we'd basically > forget its contribution. > > This may be a benign bug, since we only collect this value for testing > currently. > > Maybe the thing to do here is not reset this to zero each loop, and inside the > loop, add in the per-task delta? > > Since it's benign, it would be okay to fix this in a follow-on patch set if you > like. Ill fix it in a follow-on patch set. https://codereview.chromium.org/2905403002/diff/300001/chrome/browser/task_ma... File chrome/browser/task_manager/task_manager_browsertest.cc (right): https://codereview.chromium.org/2905403002/diff/300001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_browsertest.cc:697: ->ExecuteJavaScriptForTests(base::UTF8ToUTF16(test_js)); On 2017/06/20 19:52:27, ncarter (slow) wrote: > Add in: > > // TODO(cburn): The assertion below currently assumes that the rate contribution > of the entire 16MB upload arrives in a single refresh cycle. That's true now > because it's only reported when the transaction completes, but if that changes > in the future, this assertion may need to change. > > (btw -- this is totally academic -- if we change it to something, one idea is to > assert that the peak reported network use exceeded (16 << 20) / elapsed_time, > where elapsed_time for the period that we were waiting. by, like, [waves hands > vigorously] the mean value theorem?) Done. https://codereview.chromium.org/2905403002/diff/300001/chrome/browser/task_ma... File chrome/browser/task_manager/task_manager_interface.h (right): https://codereview.chromium.org/2905403002/diff/300001/chrome/browser/task_ma... chrome/browser/task_manager/task_manager_interface.h:198: // means network usage calculation refresh is currently not available. On 2017/06/20 19:52:27, ncarter (slow) wrote: > GetCumulativeProcessTotalNetworkUsage doesn't seem to ever return -1. > > (GetProcessTotalNetworkUsage does return -1, however) Done.
chrome/browser/net LGTM
The CQ bit was unchecked by cburn@google.com
The CQ bit was checked by cburn@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org Link to the patchset: https://codereview.chromium.org/2905403002/#ps340001 (title: "fixed comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1497996300414220,
"parent_rev": "de503a07456be6c8c6b85deaa9559c4296a60d41", "commit_rev":
"e9d2f361a156c914b8e4ac51f98397c33d92573d"}
Message was sent while issue was closed.
Description was changed from ========== This adds the plumbing needed to take data about completed outbound network traffic to the task manager for tracking network usage. There are a number of naming changes to help distinguish between what is and isn't a "rate" and what is sent vs read vs transferred. Read being incoming bytes, sent being outgoing bytes and transfered being either read or sent bytes. The "rates" are not calculated based on what is live on the network. It is instead based on when requests are completed so large uploads will all be registered in one refresh. This update changes the struct BytesReadParam to BytesTrasferedParam and stores both bytes read and bytes sent. It updates how the struct is processed to handle the additional tracking of sent bytes. Tasks have been changed to store the cumulative_bytes_read_ and cumulative_bytes_sent_ which are stored on Refresh() to the last_refresh_cumulative_bytes_sent_ and last_refresh_cumulative_bytes_read_ . These are used to calculate network_sent_rate_ and network_read_rate_ which are summed for the network_usage_rate_. Tasks no longer store a network usage of -1 to signify that they have not had any network traffic, the utility of that flag no longer appears to be used so it was removed. Because of this ReportsNetworkUsage() has been removed. Tasks and Task Groups can now report their cumulative network usage. BUG=720773 ========== to ========== This adds the plumbing needed to take data about completed outbound network traffic to the task manager for tracking network usage. There are a number of naming changes to help distinguish between what is and isn't a "rate" and what is sent vs read vs transferred. Read being incoming bytes, sent being outgoing bytes and transfered being either read or sent bytes. The "rates" are not calculated based on what is live on the network. It is instead based on when requests are completed so large uploads will all be registered in one refresh. This update changes the struct BytesReadParam to BytesTrasferedParam and stores both bytes read and bytes sent. It updates how the struct is processed to handle the additional tracking of sent bytes. Tasks have been changed to store the cumulative_bytes_read_ and cumulative_bytes_sent_ which are stored on Refresh() to the last_refresh_cumulative_bytes_sent_ and last_refresh_cumulative_bytes_read_ . These are used to calculate network_sent_rate_ and network_read_rate_ which are summed for the network_usage_rate_. Tasks no longer store a network usage of -1 to signify that they have not had any network traffic, the utility of that flag no longer appears to be used so it was removed. Because of this ReportsNetworkUsage() has been removed. Tasks and Task Groups can now report their cumulative network usage. BUG=720773 Review-Url: https://codereview.chromium.org/2905403002 Cr-Commit-Position: refs/heads/master@{#480988} Committed: https://chromium.googlesource.com/chromium/src/+/e9d2f361a156c914b8e4ac51f983... ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as https://chromium.googlesource.com/chromium/src/+/e9d2f361a156c914b8e4ac51f983... |
