Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
726673002:
Add a new NetworkActivityMonitor to track network activity (Closed)
|
Created:
6 years, 1 month ago by Ryan Hamilton Modified:
6 years, 1 month ago Reviewers:
eroman CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdd a new NetworkActivityMonitor to track network activity across all
sockets and provides cumulative statistics about bytes written to and
read from the network. It uses locks to ensure thread-safety.
Committed: https://crrev.com/29ae89d5019ec937b66613083deb9a87900b39fc
Cr-Commit-Position: refs/heads/master@{#304302}
Patch Set 1 #Patch Set 2 : add tests #Patch Set 3 : NET_EXPORT_PRIVATE #
Total comments: 26
Patch Set 4 : fix comments #Patch Set 5 : fix comments #
Total comments: 36
Patch Set 6 : fix more comments #Patch Set 7 : Add Reset method #
Total comments: 20
Patch Set 8 : fix final comments #
Total comments: 2
Patch Set 9 : fix *really* final comments #
Messages
Total messages: 19 (4 generated)
rch@chromium.org changed reviewers: + eroman@chromium.org
Hi Eric, I think this is what we talked about. Please let me know what you think! (I'll wire this up to QUIC in a subsequent CL) Cheers, Ryan
https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... File net/base/network_activity_monitor.cc (right): https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.cc:30: g_network_activity_monitor = new NetworkActivityMonitor(); This not a good pattern as it leads to a race when initializing (could allocate g_network_activity_monitor). Try something like this instead: base::LazyInstance<NetworkActivityMonitor>::Leaky g_network_activity_monitor = LAZY_INSTANCE_INITIALIZER; https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.cc:35: void NetworkActivityMonitor::AddBytesRead(uint64 bytes_read) { Not sure if it will be useful, but you may consider making these return a uint64 (either the previous total, or the new total). https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... File net/base/network_activity_monitor.h (right): https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.h:22: // the network. It uses locks to ensure thread-safety. I would also emphasize the caveats of what this is measuring: * This does not include cache activity * Bytes "written" is attempted bytes written (no guarantee they were actually sent over the network) * Whereas bytes read is the actual bytes received from the network. * Network activity not initiated directly using client sockets won't be reflected here (for instance DNS queries issued by getaddrinfo()). https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.h:28: void AddBytesRead(uint64 bytes_read); [optional] Consider "Increment" instead of Add https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.h:35: friend class test::NetworkActivityMonitorPeer; What about using a friend_test macro? Or at a minimum giving it a name that makes it obvious it is a test class. https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.h:37: NetworkActivityMonitor(); Another interesting API for future consideration would be: GetTicksSinceLastRead(); GetTicksSinceLastWrite() (Where adding bytes read/written would also update a timestamp). https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.h:41: mutable base::Lock bytes_read_lock_; Why is this mutable? Presumably you meant to make GetBytesRead() / GetBytesWritten() const? https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.h:45: mutable base::Lock bytes_written_lock_; A single lock to protect all members should suffice, remove this one. https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... File net/base/network_activity_monitor_unittest.cc (right): https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor_unittest.cc:35: NetworkActivityMonitor* monitor = NetworkActivityMonitorPeer::CreateMonitor(); I suggest also adding a test that posts tasks to a threadpool, to exercise concurrent add/get. Could maybe add on 3 different threads, then once it is all done retrieve and make sure total was correct. (Will give TSAN an opportunity to verify stuff). https://codereview.chromium.org/726673002/diff/40001/net/socket/tcp_socket_li... File net/socket/tcp_socket_libevent.cc (right): https://codereview.chromium.org/726673002/diff/40001/net/socket/tcp_socket_li... net/socket/tcp_socket_libevent.cc:605: NetworkActivityMonitor::GetInstance()->AddBytesRead(rv); Perhaps calling this AddBytesReceived() would be more accurate. https://codereview.chromium.org/726673002/diff/40001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/726673002/diff/40001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:599: NetworkActivityMonitor::GetInstance()->AddBytesRead(result); Probably not worth micro-optimizing, but I wonder if result == 0 is something that happens
I think I've addressed all of your comments. One question about naming Written -> Send which I'd like your input on. Hopefully I got a workable threading test... https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... File net/base/network_activity_monitor.cc (right): https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.cc:30: g_network_activity_monitor = new NetworkActivityMonitor(); On 2014/11/13 22:18:39, eroman wrote: > This not a good pattern as it leads to a race when initializing (could allocate > g_network_activity_monitor). > > Try something like this instead: > > base::LazyInstance<NetworkActivityMonitor>::Leaky g_network_activity_monitor = > LAZY_INSTANCE_INITIALIZER; Done! I should have discovered this on my own. https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.cc:35: void NetworkActivityMonitor::AddBytesRead(uint64 bytes_read) { On 2014/11/13 22:18:39, eroman wrote: > Not sure if it will be useful, but you may consider making these return a uint64 > (either the previous total, or the new total). I thought about this, and am happy to do so if we ever have a need. However, I think that the only callers of the Add() methods will be sockets, and the callers of the Get() methods will never be sockets. So if it's cool with you, I think I'll leave it as is? https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... File net/base/network_activity_monitor.h (right): https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.h:22: // the network. It uses locks to ensure thread-safety. On 2014/11/13 22:18:39, eroman wrote: > I would also emphasize the caveats of what this is measuring: > > * This does not include cache activity > * Bytes "written" is attempted bytes written (no guarantee they were actually > sent over the network) > * Whereas bytes read is the actual bytes received from the network. > * Network activity not initiated directly using client sockets won't be > reflected here (for instance DNS queries issued by getaddrinfo()). Done. https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.h:28: void AddBytesRead(uint64 bytes_read); On 2014/11/13 22:18:39, eroman wrote: > [optional] Consider "Increment" instead of Add Done. https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.h:35: friend class test::NetworkActivityMonitorPeer; On 2014/11/13 22:18:39, eroman wrote: > What about using a friend_test macro? Or at a minimum giving it a name that > makes it obvious it is a test class. Doesn't the test:: prefix do that? Personally, I'm not a fan of the FRIEND_TEST macros because each time you add a new test you end up having to change the "real" .h file. What do you think? https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.h:37: NetworkActivityMonitor(); On 2014/11/13 22:18:39, eroman wrote: > Another interesting API for future consideration would be: > GetTicksSinceLastRead(); > GetTicksSinceLastWrite() > > (Where adding bytes read/written would also update a timestamp). Oo! Interesting. That's a good idea. Done. (Though s/Ticks/Time/ since this should probably return a TimeDelta). https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.h:41: mutable base::Lock bytes_read_lock_; On 2014/11/13 22:18:39, eroman wrote: > Why is this mutable? Presumably you meant to make GetBytesRead() / > GetBytesWritten() const? Whoops, yes. Done. https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.h:45: mutable base::Lock bytes_written_lock_; On 2014/11/13 22:18:39, eroman wrote: > A single lock to protect all members should suffice, remove this one. Done. https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... File net/base/network_activity_monitor_unittest.cc (right): https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor_unittest.cc:35: NetworkActivityMonitor* monitor = NetworkActivityMonitorPeer::CreateMonitor(); On 2014/11/13 22:18:39, eroman wrote: > I suggest also adding a test that posts tasks to a threadpool, to exercise > concurrent add/get. > > Could maybe add on 3 different threads, then once it is all done retrieve and > make sure total was correct. > > (Will give TSAN an opportunity to verify stuff). Done. https://codereview.chromium.org/726673002/diff/40001/net/socket/tcp_socket_li... File net/socket/tcp_socket_libevent.cc (right): https://codereview.chromium.org/726673002/diff/40001/net/socket/tcp_socket_li... net/socket/tcp_socket_libevent.cc:605: NetworkActivityMonitor::GetInstance()->AddBytesRead(rv); On 2014/11/13 22:18:39, eroman wrote: > Perhaps calling this AddBytesReceived() would be more accurate. Done. Would you also recommend s/Written/Sent/? (Seems like that would be more symmetrical) https://codereview.chromium.org/726673002/diff/40001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/726673002/diff/40001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:599: NetworkActivityMonitor::GetInstance()->AddBytesRead(result); On 2014/11/13 22:18:39, eroman wrote: > Probably not worth micro-optimizing, but I wonder if result == 0 is something > that happens Heh, I thought about that too. I suspect that's *possible* but probably not worth worrying about. I could add a guard in the implementation of AddBytesRead, I guess. I'll do that.
https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... File net/base/network_activity_monitor.cc (right): https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.cc:35: void NetworkActivityMonitor::AddBytesRead(uint64 bytes_read) { On 2014/11/13 23:38:17, Ryan Hamilton wrote: > On 2014/11/13 22:18:39, eroman wrote: > > Not sure if it will be useful, but you may consider making these return a > uint64 > > (either the previous total, or the new total). > > I thought about this, and am happy to do so if we ever have a need. However, I > think that the only callers of the Add() methods will be sockets, and the > callers of the Get() methods will never be sockets. So if it's cool with you, I > think I'll leave it as is? sgtm https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... File net/base/network_activity_monitor.h (right): https://codereview.chromium.org/726673002/diff/40001/net/base/network_activit... net/base/network_activity_monitor.h:35: friend class test::NetworkActivityMonitorPeer; On 2014/11/13 23:38:18, Ryan Hamilton wrote: > On 2014/11/13 22:18:39, eroman wrote: > > What about using a friend_test macro? Or at a minimum giving it a name that > > makes it obvious it is a test class. > > Doesn't the test:: prefix do that? Personally, I'm not a fan of the FRIEND_TEST > macros because each time you add a new test you end up having to change the > "real" .h file. What do you think? Ah, I hadn't noticed the test:: prefix! https://codereview.chromium.org/726673002/diff/40001/net/socket/tcp_socket_li... File net/socket/tcp_socket_libevent.cc (right): https://codereview.chromium.org/726673002/diff/40001/net/socket/tcp_socket_li... net/socket/tcp_socket_libevent.cc:605: NetworkActivityMonitor::GetInstance()->AddBytesRead(rv); On 2014/11/13 23:38:18, Ryan Hamilton wrote: > On 2014/11/13 22:18:39, eroman wrote: > > Perhaps calling this AddBytesReceived() would be more accurate. > > Done. > > Would you also recommend s/Written/Sent/? (Seems like that would be more > symmetrical) sgtm https://codereview.chromium.org/726673002/diff/40001/net/udp/udp_socket_win.cc File net/udp/udp_socket_win.cc (right): https://codereview.chromium.org/726673002/diff/40001/net/udp/udp_socket_win.c... net/udp/udp_socket_win.cc:599: NetworkActivityMonitor::GetInstance()->AddBytesRead(result); On 2014/11/13 23:38:18, Ryan Hamilton wrote: > On 2014/11/13 22:18:39, eroman wrote: > > Probably not worth micro-optimizing, but I wonder if result == 0 is something > > that happens > > Heh, I thought about that too. I suspect that's *possible* but probably not > worth worrying about. I could add a guard in the implementation of AddBytesRead, > I guess. I'll do that. Meh, i was mostly just bikeshedding. Probably better without the guard since it is likely to happen infrequently. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... File net/base/network_activity_monitor.cc (right): https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.cc:11: // The actual singleton notifier. The class contract forbids usage of the API I am not sure what this comment means. I suggest deleting it. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.cc:35: if (bytes_received != 0) { meh, suggest removing. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.cc:38: last_received_ticks_ = base::TimeTicks::Now(); Can you move the call to TimeTicks::Now() outside of the lock? (The implementation of that function is surprisingly complicated) https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.cc:43: if (bytes_written != 0) { meh, suggest removing (just one more branch and line of code). https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.cc:46: last_write_ticks_ = base::TimeTicks::Now(); Same thing here https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.cc:63: return base::TimeTicks::Now() - last_received_ticks_; Suggest moving Now() outside of lock https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.cc:68: return base::TimeTicks::Now() - last_write_ticks_; Same here. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... File net/base/network_activity_monitor.h (right): https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.h:24: // the network. It uses locks to ensure threceived-safety. typeo in the last word. unless this is german and you are just making up words. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.h:32: // be reflected here (for instance DNS queries issued by getaddrinfo()). I see you took my examples verbatim. while I am flattered, not sure my language was very good :) https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.h:38: void IncrementBytesReceived(uint64 bytes_received); Prefer uint64_t for new code. See: https://code.google.com/p/chromium/codesearch#chromium/src/base/basictypes.h&... This applies throughout. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.h:44: base::TimeDelta GetTimeSinceLastReceived() const; as a side rant: Both base::Time::Now() and base::TimeTicks::Now() are stupid, in their own way... Time can go backwards, because of system clock. And TimeTicks is ill-defined for system suspend/resume. On some platforms it stops ticking while asleep, on others it continues ticking. The end result is when waking up from sleep on (Windows) you may find yourself having a very long time since last received, whereas on Linux it will be small. Sigh. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... File net/base/network_activity_monitor_unittest.cc (right): https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor_unittest.cc:31: NetworkActivityMonitor* monitor = NetworkActivityMonitorPeer::CreateMonitor(); Looks like |monitor| is leaked in these tests. Perhaps a scoped_ptr<> https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor_unittest.cc:45: uint64 bytes = 12345; for funzies you could use a value larger than (2^32 - 1), to make sure it really is 64-bits. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor_unittest.cc:56: uint64 bytes) { indentation. suggest running "git cl format" for auto-awesome. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor_unittest.cc:61: uint64 bytes) { ditto https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor_unittest.cc:68: base::Thread* threceiveds[3]; more german. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor_unittest.cc:105: threceiveds[i]->Stop(); have you tried not leaking thereceiveds ? ;)
Thanks, Eric! https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... File net/base/network_activity_monitor.cc (right): https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.cc:11: // The actual singleton notifier. The class contract forbids usage of the API On 2014/11/14 00:06:16, eroman wrote: > I am not sure what this comment means. I suggest deleting it. Done. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.cc:35: if (bytes_received != 0) { On 2014/11/14 00:06:16, eroman wrote: > meh, suggest removing. Done. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.cc:38: last_received_ticks_ = base::TimeTicks::Now(); On 2014/11/14 00:06:15, eroman wrote: > Can you move the call to TimeTicks::Now() outside of the lock? > > (The implementation of that function is surprisingly complicated) Done, I think. But I'm not sure if I need to use {}s to explicitly scope the AutoLock? https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.cc:43: if (bytes_written != 0) { On 2014/11/14 00:06:16, eroman wrote: > meh, suggest removing (just one more branch and line of code). Done. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.cc:46: last_write_ticks_ = base::TimeTicks::Now(); On 2014/11/14 00:06:15, eroman wrote: > Same thing here Done. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.cc:63: return base::TimeTicks::Now() - last_received_ticks_; On 2014/11/14 00:06:16, eroman wrote: > Suggest moving Now() outside of lock Done. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.cc:68: return base::TimeTicks::Now() - last_write_ticks_; On 2014/11/14 00:06:15, eroman wrote: > Same here. Done. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... File net/base/network_activity_monitor.h (right): https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.h:24: // the network. It uses locks to ensure threceived-safety. On 2014/11/14 00:06:16, eroman wrote: > typeo in the last word. > unless this is german and you are just making up words. *facepalm* s/read/received/ fail! https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.h:32: // be reflected here (for instance DNS queries issued by getaddrinfo()). On 2014/11/14 00:06:16, eroman wrote: > I see you took my examples verbatim. while I am flattered, not sure my language > was very good :) Sounded great to me :> How's this look now? https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.h:38: void IncrementBytesReceived(uint64 bytes_received); On 2014/11/14 00:06:16, eroman wrote: > Prefer uint64_t for new code. See: > https://code.google.com/p/chromium/codesearch#chromium/src/base/basictypes.h&... > > This applies throughout. Done. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor.h:44: base::TimeDelta GetTimeSinceLastReceived() const; On 2014/11/14 00:06:16, eroman wrote: > as a side rant: > > Both base::Time::Now() and base::TimeTicks::Now() are stupid, in their own > way... > > Time can go backwards, because of system clock. > > And TimeTicks is ill-defined for system suspend/resume. On some platforms it > stops ticking while asleep, on others it continues ticking. > > The end result is when waking up from sleep on (Windows) you may find yourself > having a very long time since last received, whereas on Linux it will be small. > > Sigh. Awesome! :| https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... File net/base/network_activity_monitor_unittest.cc (right): https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor_unittest.cc:31: NetworkActivityMonitor* monitor = NetworkActivityMonitorPeer::CreateMonitor(); On 2014/11/14 00:06:16, eroman wrote: > Looks like |monitor| is leaked in these tests. > > Perhaps a scoped_ptr<> I fixed the leak, but since the monitor has a private destructor I ended up not being able to use a scoped_ptr and instead added a method to the peer. Work for you? Alternatively, I could make the Peer a subclass of Monitor and use that in the tests in a scoped_ptr. That might be cleaner? https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor_unittest.cc:45: uint64 bytes = 12345; On 2014/11/14 00:06:16, eroman wrote: > for funzies you could use a value larger than (2^32 - 1), to make sure it really > is 64-bits. Done. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor_unittest.cc:56: uint64 bytes) { On 2014/11/14 00:06:16, eroman wrote: > indentation. > > suggest running "git cl format" for auto-awesome. Done. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor_unittest.cc:61: uint64 bytes) { On 2014/11/14 00:06:16, eroman wrote: > ditto Done. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor_unittest.cc:68: base::Thread* threceiveds[3]; On 2014/11/14 00:06:16, eroman wrote: > more german. Done. https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor_unittest.cc:105: threceiveds[i]->Stop(); On 2014/11/14 00:06:17, eroman wrote: > have you tried not leaking thereceiveds ? > ;) leaking threads! leaking monitors. Leak all the things! Done. Thanks!
https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... File net/base/network_activity_monitor_unittest.cc (right): https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor_unittest.cc:31: NetworkActivityMonitor* monitor = NetworkActivityMonitorPeer::CreateMonitor(); On 2014/11/14 00:32:29, Ryan Hamilton wrote: > On 2014/11/14 00:06:16, eroman wrote: > > Looks like |monitor| is leaked in these tests. > > > > Perhaps a scoped_ptr<> > > I fixed the leak, but since the monitor has a private destructor I ended up not > being able to use a scoped_ptr and instead added a method to the peer. Work for > you? > > Alternatively, I could make the Peer a subclass of Monitor and use that in the > tests in a scoped_ptr. That might be cleaner? Or instead you could add some : ResetStateForTest() private method which rests counters to zero. You could use a test fixture, which resets as part of the setup, so it now predictably starts at zero. This has the advantage of being able to test through the singleton interface. the only read disadvantage is it means the tests can't be run in parallel, but that is true of many of our tests anyway.
https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... File net/base/network_activity_monitor_unittest.cc (right): https://codereview.chromium.org/726673002/diff/80001/net/base/network_activit... net/base/network_activity_monitor_unittest.cc:31: NetworkActivityMonitor* monitor = NetworkActivityMonitorPeer::CreateMonitor(); On 2014/11/14 00:46:13, eroman wrote: > On 2014/11/14 00:32:29, Ryan Hamilton wrote: > > On 2014/11/14 00:06:16, eroman wrote: > > > Looks like |monitor| is leaked in these tests. > > > > > > Perhaps a scoped_ptr<> > > > > I fixed the leak, but since the monitor has a private destructor I ended up > not > > being able to use a scoped_ptr and instead added a method to the peer. Work > for > > you? > > > > Alternatively, I could make the Peer a subclass of Monitor and use that in the > > tests in a scoped_ptr. That might be cleaner? > > Or instead you could add some : > ResetStateForTest() > > private method which rests counters to zero. > > You could use a test fixture, which resets as part of the setup, so it now > predictably starts at zero. > > This has the advantage of being able to test through the singleton interface. > the only read disadvantage is it means the tests can't be run in parallel, but > that is true of many of our tests anyway. I considered that, but my general fear of using statics/singletons in tests scared me away. It also means that I would not be able to test that the bytes_sent and bytes_received default to 0. That being said, I've uploaded a patch with this change. If it seems better to you, I'm happy.
lgtm https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... File net/base/network_activity_monitor.h (right): https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor.h:24: // the network. It uses locks to ensure thread-safety. nit: locks --> "a lock" or alternately "locking" https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor.h:27: // * Bytes "sent" includes all bytes all send attempts and may include "all bytes all send attempts" --> reword https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... File net/base/network_activity_monitor_unittest.cc (right): https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:20: monitor->bytes_sent_ = 0; It might be possible for TSAN to bark about this, since bytes_sent_ etc are accessed later from another thread. I suggest acquiring the lock to avoid any testing noise. https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:81: threads[i]->Start(); nit: I suggest wrapping this inside an ASSERT_TRUE() (I have seen these failures in the wild on Windows, but usually because of third party malware exceeding max number of handles/threads). https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:86: size_t num_increments = 157; Fancy... i like it! https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:87: uint64_t bytes_received = 7294954321u; [no action necessary] Strictly speaking you probably want ull, but "u" should be sufficient to avoid the compiler truncation warning :) https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:98: base::Bind(&Verifier::VerifyBytesSentIsMultipleOf, The Verifier class feels it could just be static methods (no instance variables are used). And then this becomes: base::Bind(&VerifyBytesSentIsMultipleOf, monitor_, bytes_sent) Without the need for Unretained. Also, I would encourage not passing monitor_ and retrieving the singleton inside the verifier function. This has the benefit of making sure (by way of TSAN) that retrieving the singleton across threads is safe too. https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:107: threads[i]->Stop(); Calling Stop() is not necessary, the dtor handles it. (In fact there is weirdness with thread subclasses specifically so they can handle this case!) https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:108: delete threads[i]; [optional] If you wanted to be fancy, could use STLDeleteContainerPointers() instead of looping. Meh, probably lame. I guess if you went that route should just make the array a std::vector<> and then call STLDeleteElements(&threads). Whatever https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:112: EXPECT_EQ(num_increments * bytes_sent, monitor_->GetBytesSent()); Overall I like this test, thanks!
Thanks! https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... File net/base/network_activity_monitor.h (right): https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor.h:24: // the network. It uses locks to ensure thread-safety. On 2014/11/14 20:57:04, eroman wrote: > nit: locks --> "a lock" or alternately "locking" Done. https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor.h:27: // * Bytes "sent" includes all bytes all send attempts and may include On 2014/11/14 20:57:04, eroman wrote: > "all bytes all send attempts" --> reword Done. https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... File net/base/network_activity_monitor_unittest.cc (right): https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:20: monitor->bytes_sent_ = 0; On 2014/11/14 20:57:04, eroman wrote: > It might be possible for TSAN to bark about this, since bytes_sent_ etc are > accessed later from another thread. I suggest acquiring the lock to avoid any > testing noise. Done. https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:81: threads[i]->Start(); On 2014/11/14 20:57:04, eroman wrote: > nit: I suggest wrapping this inside an ASSERT_TRUE() > > (I have seen these failures in the wild on Windows, but usually because of third > party malware exceeding max number of handles/threads). Done. https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:86: size_t num_increments = 157; On 2014/11/14 20:57:04, eroman wrote: > Fancy... i like it! Thanks! https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:87: uint64_t bytes_received = 7294954321u; On 2014/11/14 20:57:04, eroman wrote: > [no action necessary] Strictly speaking you probably want ull, but "u" should be > sufficient to avoid the compiler truncation warning :) Oh! Thanks for the reminder. I actually need to use GG_UINT64_C() because ... compilers. (We have this problem all the time in QUIC tests since we use lots of uint64s) https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:98: base::Bind(&Verifier::VerifyBytesSentIsMultipleOf, On 2014/11/14 20:57:04, eroman wrote: > The Verifier class feels it could just be static methods (no instance variables > are used). > > > And then this becomes: > base::Bind(&VerifyBytesSentIsMultipleOf, monitor_, bytes_sent) > > Without the need for Unretained. AH! For some reason I forgot that this as an option. Done! > Also, I would encourage not passing monitor_ and retrieving the singleton inside > the verifier function. This has the benefit of making sure (by way of TSAN) that > retrieving the singleton across threads is safe too. Good idea. https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:107: threads[i]->Stop(); On 2014/11/14 20:57:04, eroman wrote: > Calling Stop() is not necessary, the dtor handles it. (In fact there is > weirdness with thread subclasses specifically so they can handle this case!) Done. https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:108: delete threads[i]; On 2014/11/14 20:57:04, eroman wrote: > [optional] If you wanted to be fancy, could use STLDeleteContainerPointers() > instead of looping. > > Meh, probably lame. I guess if you went that route should just make the array a > std::vector<> and then call STLDeleteElements(&threads). Whatever I like it! Done. https://codereview.chromium.org/726673002/diff/120001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:112: EXPECT_EQ(num_increments * bytes_sent, monitor_->GetBytesSent()); On 2014/11/14 20:57:04, eroman wrote: > Overall I like this test, thanks! Thank you very much for encouraging me to write it. It's definitely much better than what I had previously.
The CQ bit was checked by rch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/726673002/140001
https://codereview.chromium.org/726673002/diff/140001/net/base/network_activi... File net/base/network_activity_monitor_unittest.cc (right): https://codereview.chromium.org/726673002/diff/140001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:39: NetworkActivityMonitor* monitor_; Consider removing this, since it looks weird that the Verifer functions don't use it but the test body itself does
The CQ bit was unchecked by rch@chromium.org
https://codereview.chromium.org/726673002/diff/140001/net/base/network_activi... File net/base/network_activity_monitor_unittest.cc (right): https://codereview.chromium.org/726673002/diff/140001/net/base/network_activi... net/base/network_activity_monitor_unittest.cc:39: NetworkActivityMonitor* monitor_; On 2014/11/14 22:36:22, eroman wrote: > Consider removing this, since it looks weird that the Verifer functions don't > use it but the test body itself does Good idea. I thought about doing that but was too lazy. Done!
The CQ bit was checked by rch@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/726673002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/29ae89d5019ec937b66613083deb9a87900b39fc Cr-Commit-Position: refs/heads/master@{#304302} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
