Notify NQE of TCP RTT values

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>
 
@@ skipping 128 matching lines @@
 void CheckSupportAndMaybeEnableTCPFastOpen(bool user_enabled) {
 #if defined(OS_LINUX) || defined(OS_ANDROID)
   base::PostTaskAndReplyWithResult(
       base::WorkerPool::GetTaskRunner(/*task_is_slow=*/false).get(),
       FROM_HERE,
       base::Bind(SystemSupportsTCPFastOpen),
       base::Bind(RegisterTCPFastOpenIntentAndSupport, user_enabled));
 #endif
 }
 
-TCPSocketPosix::TCPSocketPosix(NetLog* net_log, const NetLog::Source& source)
-    : use_tcp_fastopen_(false),
+TCPSocketPosix::TCPSocketPosix(
+    scoped_ptr<SocketPerformanceWatcher> socket_performance_watcher,
+    NetLog* net_log,
+    const NetLog::Source& source)
+    : socket_performance_watcher_(std::move(socket_performance_watcher)),
+      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_(BoundNetLog::Make(net_log, NetLog::SOURCE_SOCKET)) {
   net_log_.BeginEvent(NetLog::TYPE_SOCKET_ALIVE,
                       source.ToEventParametersCallback());
 }
 
 TCPSocketPosix::~TCPSocketPosix() {
@@ skipping 356 matching lines @@
                                         IPEndPoint* address) {
   DCHECK(accept_socket_);
 
   SockaddrStorage storage;
   if (accept_socket_->GetPeerAddress(&storage) != OK ||
       !address->FromSockAddr(storage.addr, storage.addr_len)) {
     accept_socket_.reset();
     return ERR_ADDRESS_INVALID;
   }
 
-  tcp_socket->reset(new TCPSocketPosix(net_log_.net_log(), net_log_.source()));
+  tcp_socket->reset(
+      new TCPSocketPosix(nullptr, net_log_.net_log(), net_log_.source()));
   (*tcp_socket)->socket_.reset(accept_socket_.release());
   return OK;
 }
 
 void TCPSocketPosix::ConnectCompleted(const CompletionCallback& callback,
-                                      int rv) const {
+                                      int rv) {
   DCHECK_NE(ERR_IO_PENDING, rv);
+  NotifySocketPerformanceWatcher();
   callback.Run(HandleConnectCompleted(rv));
 }
 
 int TCPSocketPosix::HandleConnectCompleted(int rv) const {
   // Log the end of this attempt (and any OS error it threw).
   if (rv != OK) {
     net_log_.EndEvent(NetLog::TYPE_TCP_CONNECT_ATTEMPT,
                       NetLog::IntCallback("os_error", errno));
   } else {
     net_log_.EndEvent(NetLog::TYPE_TCP_CONNECT_ATTEMPT);
@@ skipping 33 matching lines @@
 
   net_log_.EndEvent(NetLog::TYPE_TCP_CONNECT,
                     CreateNetLogSourceAddressCallback(storage.addr,
                                                       storage.addr_len));
 }
 
 void TCPSocketPosix::ReadCompleted(const scoped_refptr<IOBuffer>& buf,
                                    const CompletionCallback& callback,
                                    int rv) {
   DCHECK_NE(ERR_IO_PENDING, rv);
+  NotifySocketPerformanceWatcher();
   callback.Run(HandleReadCompleted(buf.get(), rv));
 }
 
 int TCPSocketPosix::HandleReadCompleted(IOBuffer* buf, int rv) {
   if (tcp_fastopen_write_attempted_ && !tcp_fastopen_connected_) {
     // A TCP FastOpen connect-with-write was attempted. This read was a
     // subsequent read, which either succeeded or failed. If the read
     // succeeded, the socket is considered connected via TCP FastOpen.
     // If the read failed, TCP FastOpen is (conservatively) turned off for all
     // subsequent connections. TCP FastOpen status is recorded in both cases.
@@ skipping 17 matching lines @@
                                 buf->data());
   NetworkActivityMonitor::GetInstance()->IncrementBytesReceived(rv);
 
   return rv;
 }
 
 void TCPSocketPosix::WriteCompleted(const scoped_refptr<IOBuffer>& buf,
                                     const CompletionCallback& callback,
                                     int rv) {
   DCHECK_NE(ERR_IO_PENDING, rv);
+  NotifySocketPerformanceWatcher();
   callback.Run(HandleWriteCompleted(buf.get(), rv));
 }
 
 int TCPSocketPosix::HandleWriteCompleted(IOBuffer* buf, int rv) {
   if (rv < 0) {
     if (tcp_fastopen_write_attempted_ && !tcp_fastopen_connected_) {
       // TCP FastOpen connect-with-write was attempted, and the write failed
       // for unknown reasons. Record status and (conservatively) turn off
       // TCP FastOpen for all subsequent connections.
       // TODO (jri): This currently results in conservative behavior, where TCP
@@ skipping 67 matching lines @@
     // turning off TCP FastOpen on more specific errors.
     tcp_fastopen_status_ = TCP_FASTOPEN_ERROR;
     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)
+  if (!socket_performance_watcher_)
+    return;
+
+  base::TimeTicks now = base::TimeTicks::Now();
+  if (now - last_socket_performance_watcher_rtt_notify_ <
+      socket_performance_watcher_->rtt_notification_interval()) {
+    return;
+  }

[Ryan Sleevi, 2016/02/25 22:48:47]

Why is this a function of the socket - and not of the watcher?

Is this a performance optimization? If so, document it.

Nothing about this code explains apriori about lines 725-729, so you should try
to provide clarifying guidance.

[tbansal1, 2016/02/26 23:43:34]

Changed it to make it cleaner. Also, moved the logic to SPW.
Done.
Done. Added couple of comments.

+
+  last_socket_performance_watcher_rtt_notify_ = now;
+
+  tcp_info info;
+  if (!GetTcpInfo(socket_->socket_fd(), &info))
+    return;
+
+  if (info.tcpi_rtt > 0) {
+    // Zero RTT value indicates that tcp_info struct was not properly populated.

[Ryan Sleevi, 2016/02/25 22:48:47]

COMMENT NIT: This comment reads weird, because it does not logically flow with
how the condition is evaluated. Further, it raises the question of "What about
less than zero? Is that possible? What does it indicate?"

// Only notify the SocketPerformanceWatcher when the tcp_info struct
// was fully populated.
if (info.tcpi_rtt > 0) {
  socket_performance_watcher_->...
}

[tbansal1, 2016/02/26 23:43:34]

Done.

+    socket_performance_watcher_->OnUpdatedRTTAvailable(
+        base::TimeDelta::FromMicroseconds(info.tcpi_rtt));
+  }
+  UMA_HISTOGRAM_TIMES("SPW.TCPRTT.NotifyDelay", base::TimeTicks::Now() - now);

[Ryan Sleevi, 2016/02/25 22:48:47]

Wow, this seems like a really gross histogram name; do you have to abbreviate
it?

[tbansal1, 2016/02/26 23:43:34]

This is now obsolete. Sorry, I did not know abbreviation in histogram names was
a bad idea. There are plenty of histograms with abbreviated names (SB2.*, PLT.*)

+#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_ =
         (tcp_fastopen_status_ == TCP_FASTOPEN_FAST_CONNECT_RETURN ?
             TCP_FASTOPEN_FAST_CONNECT_READ_FAILED :
             TCP_FASTOPEN_SLOW_CONNECT_READ_FAILED);
@@ skipping 41 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