Notify NQE of TCP RTT values

net/socket/tcp_socket_posix.cc

Index: net/socket/tcp_socket_posix.cc
diff --git a/net/socket/tcp_socket_posix.cc b/net/socket/tcp_socket_posix.cc
index 0546e6075a57e7663640ac93993b8fa71f1cd3e4..5dba2b92382ed95c08c6efd2351c4f78bf0e328c 100644
--- a/net/socket/tcp_socket_posix.cc
+++ b/net/socket/tcp_socket_posix.cc
@@ -146,8 +146,12 @@ void CheckSupportAndMaybeEnableTCPFastOpen(bool 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),
@@ -524,14 +528,16 @@ int TCPSocketPosix::BuildTcpSocketPosix(scoped_ptr<TCPSocketPosix>* tcp_socket,
     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));
 }
 
@@ -585,6 +591,7 @@ 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));
 }
 
@@ -622,6 +629,7 @@ 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));
 }
 
@@ -709,6 +717,32 @@ int TCPSocketPosix::TcpFastOpenWrite(IOBuffer* buf,
   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);