Throttle Socket watcher RTT notifications from QUIC

net/socket/tcp_socket_posix.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/socket/tcp_socket.h"
 
 #include <errno.h>
 #include <netinet/tcp.h>
 #include <sys/socket.h>
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/task_scheduler/post_task.h"
-#include "base/time/default_tick_clock.h"
+#include "base/time/time.h"
 #include "net/base/address_list.h"
 #include "net/base/io_buffer.h"
 #include "net/base/ip_endpoint.h"
 #include "net/base/net_errors.h"
 #include "net/base/network_activity_monitor.h"
 #include "net/base/network_change_notifier.h"
 #include "net/base/sockaddr_storage.h"
 #include "net/log/net_log.h"
 #include "net/log/net_log_event_type.h"
 #include "net/log/net_log_source.h"
@@ skipping 108 matching lines @@
       base::Bind(SystemSupportsTCPFastOpen),
       base::Bind(RegisterTCPFastOpenIntentAndSupport, user_enabled));
 #endif
 }
 
 TCPSocketPosix::TCPSocketPosix(
     std::unique_ptr<SocketPerformanceWatcher> socket_performance_watcher,
     NetLog* net_log,
     const NetLogSource& source)
     : socket_performance_watcher_(std::move(socket_performance_watcher)),
-      tick_clock_(new base::DefaultTickClock()),
-      rtt_notifications_minimum_interval_(base::TimeDelta::FromSeconds(1)),
       use_tcp_fastopen_(false),
       tcp_fastopen_write_attempted_(false),
       tcp_fastopen_connected_(false),
       tcp_fastopen_status_(TCP_FASTOPEN_STATUS_UNKNOWN),
       logging_multiple_connect_attempts_(false),
       net_log_(NetLogWithSource::Make(net_log, NetLogSourceType::SOCKET)) {
   net_log_.BeginEvent(NetLogEventType::SOCKET_ALIVE,
                       source.ToEventParametersCallback());
 }
 
@@ skipping 317 matching lines @@
 
 void TCPSocketPosix::EndLoggingMultipleConnectAttempts(int net_error) {
   if (logging_multiple_connect_attempts_) {
     LogConnectEnd(net_error);
     logging_multiple_connect_attempts_ = false;
   } else {
     NOTREACHED();
   }
 }
 
-void TCPSocketPosix::SetTickClockForTesting(
-    std::unique_ptr<base::TickClock> tick_clock) {
-  tick_clock_ = std::move(tick_clock);
-}
-
 void TCPSocketPosix::AcceptCompleted(
     std::unique_ptr<TCPSocketPosix>* tcp_socket,
     IPEndPoint* address,
     const CompletionCallback& callback,
     int rv) {
   DCHECK_NE(ERR_IO_PENDING, rv);
   callback.Run(HandleAcceptCompleted(tcp_socket, address, rv));
 }
 
 int TCPSocketPosix::HandleAcceptCompleted(
@@ skipping 215 matching lines @@
     g_tcp_fastopen_has_failed = true;
     return rv;
   }
 
   tcp_fastopen_status_ = TCP_FASTOPEN_SLOW_CONNECT_RETURN;
   return socket_->WaitForWrite(buf, buf_len, callback);
 }
 
 void TCPSocketPosix::NotifySocketPerformanceWatcher() {
 #if defined(TCP_INFO)
-  const base::TimeTicks now_ticks = tick_clock_->NowTicks();
-  // Do not notify |socket_performance_watcher_| if the last notification was
-  // recent than |rtt_notifications_minimum_interval_| ago. This helps in
-  // reducing the overall overhead of the tcp_info syscalls.
-  if (now_ticks - last_rtt_notification_ < rtt_notifications_minimum_interval_)
-    return;
-

[Ryan Hamilton, 2017/02/15 23:08:17]

This is a nice cleanup.

Should we do something similar for TCPSocketWin?

[tbansal1, 2017/02/15 23:16:12]

TCPSocketWin currently does not push any RTT readings. I guess I need to read up
on Windows APIs to write that code. There is also a TODO from bmcquade here:
https://cs.chromium.org/chromium/src/net/socket/tcp_socket_win.cc?rcl=32e074e...
So, I may be I can copy his code once he implements that :)

[Ryan Hamilton, 2017/02/16 00:01:42]

Since windows is huge part of Chrome's user base, it would be good to make sure
we do this. Can you follow up with bcmquade to ensure that this is done by
someone?

[tbansal1, 2017/02/16 00:10:58]

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=692825.

   // Check if |socket_performance_watcher_| is interested in receiving a RTT
   // update notification.
   if (!socket_performance_watcher_ ||
       !socket_performance_watcher_->ShouldNotifyUpdatedRTT()) {

[Ryan Hamilton, 2017/02/15 23:08:17]

Ah! I think I understand your API choice. Getting this RTT estimate in not
necessairly cheap, so it's handy for callers to not do the Update if the update
is not needed?

[tbansal1, 2017/02/15 23:16:12]

Right. (see the other comment)

     return;
   }
 
   tcp_info info;
   if (!GetTcpInfo(socket_->socket_fd(), &info))
     return;
 
   // Only notify the |socket_performance_watcher_| if the RTT in |tcp_info|
   // struct was populated. A value of 0 may be valid in certain cases
   // (on very fast networks), but it is discarded. This means that
   // some of the RTT values may be missed, but the values that are kept are
   // guaranteed to be correct.
   if (info.tcpi_rtt == 0 && info.tcpi_rttvar == 0)
     return;
 
   socket_performance_watcher_->OnUpdatedRTTAvailable(
       base::TimeDelta::FromMicroseconds(info.tcpi_rtt));
-  last_rtt_notification_ = now_ticks;
 #endif  // defined(TCP_INFO)
 }
 
 void TCPSocketPosix::UpdateTCPFastOpenStatusAfterRead() {
   DCHECK(tcp_fastopen_status_ == TCP_FASTOPEN_FAST_CONNECT_RETURN ||
          tcp_fastopen_status_ == TCP_FASTOPEN_SLOW_CONNECT_RETURN);
 
   if (tcp_fastopen_write_attempted_ && !tcp_fastopen_connected_) {
     // TCP FastOpen connect-with-write was attempted, and failed.
     tcp_fastopen_status_ =
@@ skipping 44 matching lines @@
     if (info.tcpi_rtt > 0) {
       *out_rtt = base::TimeDelta::FromMicroseconds(info.tcpi_rtt);
       return true;
     }
   }
 #endif  // defined(TCP_INFO)
   return false;
 }
 
 }  // namespace net