Deciding best connection to schedule requests on based on cwnd and idle time

net/socket/client_socket_pool_base.cc

Index: net/socket/client_socket_pool_base.cc
===================================================================
--- net/socket/client_socket_pool_base.cc	(revision 88319)
+++ net/socket/client_socket_pool_base.cc	(working copy)
@@ -4,11 +4,16 @@
 
 #include "net/socket/client_socket_pool_base.h"
 
+#include <math.h>
+#include <utility>
+#include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/format_macros.h"
+#include "base/logging.h"
 #include "base/message_loop.h"
 #include "base/metrics/stats_counters.h"
 #include "base/stl_util-inl.h"
+#include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/time.h"
 #include "base/values.h"
@@ -32,6 +37,9 @@
 // after a certain timeout has passed without receiving an ACK.
 bool g_connect_backup_jobs_enabled = true;
 
+// Decay coef.
+double g_tcp_socket_estimated_cwnd_decay_coef = 1000000;
+
 }  // namespace
 
 namespace net {
@@ -363,7 +371,13 @@
     const Request* request, Group* group) {
   std::list<IdleSocket>* idle_sockets = group->mutable_idle_sockets();
   std::list<IdleSocket>::iterator idle_socket_it = idle_sockets->end();
+  double max_benefit = -100000000;
+  std::pair<int, int64> best_pair;
 
+  std::string debug_log(request->handle()->group_name());
+  std::string candidates;
+  double rtt_ms = 0;
+
   // Iterate through the idle sockets forwards (oldest to newest)
   //   * Delete any disconnected ones.
   //   * If we find a used idle socket, assign to |idle_socket|.  At the end,
@@ -379,11 +393,34 @@
 
     if (it->socket->WasEverUsed()) {
       // We found one we can reuse!
-      idle_socket_it = it;
+      int64 bytes_read = it->socket->NumBytesRead();
+      double B = static_cast<double>(bytes_read) / 1024.0;

[willchan no longer on Chromium, 2011/06/09 15:08:54]

nit: fix your variable naming, they should follow
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Variab....

as well as:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Genera...

It seems like you're trying to follow some function. I am not familiar with the
function, so using abbreviations like this are just confusing. Please use full
names.

You also need way more comments to explain what the calculations you are doing
mean and why you are doing this.

[Gagan, 2011/06/09 19:49:26]

Changed the variable names to follow convention.
 
Nothing complicated, simply num_kbs - (coef ^ idle_time_in_mins)
coef = 1 prefers warmest connection
coef = 1.83 simulates cwnd decay of 20KB in 5 minutes.
This function partly simulates cwnd validation and does not penalize cwnd for
smaller idle times.

+
+      int64 idle_time = (base::TimeTicks::Now() - it->start_time)
+          .InMicroseconds();
+      double idle_time_mins = static_cast<double>(idle_time) / 60000000.0;
+      double T = pow(g_tcp_socket_estimated_cwnd_decay_coef, idle_time_mins);

[Mike Belshe, 2011/06/09 16:08:52]

I believe this math is broken and we have to find a different algorithm.

The units of B are bytes.
The units of T are minutes (squared).

I don't know of any way to do interesting addition/subtraction when units are
different.  This is not a rate (like B / T (bytes/sec)).  It seems like you've
found a heuristic which kinda sorta seems to work, but it's meaningless
mathematically.

I realize you're just trying to implement a damper.  Perhaps you can explain why
you think this works?

[Gagan, 2011/06/09 19:49:26]

Yes, i want a damping function.
Im choosing coef ^ idle_time because it simulates cwnd validation, reduction of
cwnd by a factor of coef every RTO. Plus, this simulates a non linear
probability increase of connection close as idle time increases.
I thought a lot about what kind of function to choose, and this one kind of felt
right. Would love to hear your ideas.

20 KB / 5 mins was chosen conservatively. I think warmest connection first is
the way to go, but a warm with smaller idle time is also good. Hence choosing
one more value. If there are many cases where missing FIN packet causes
problems, then the stat for this should be better than warmest first.

You are right that there is a mismatch in units, thats because i don't have any
estimate of bandwidth or a good rate function.

+
+      debug_log = StringPrintf("%s\n%f\t%f\t%f",

[Mike Belshe, 2011/06/09 16:08:52]

this debug_log appears unused (or overridden below)

[Gagan, 2011/06/09 19:49:26]

its appending to its previous value

+                               debug_log.data(), B, idle_time_mins, T);
+      double benefit = B - T;
+
+      // Equality to prefer recently used connection.
+      if (benefit >= max_benefit) {
+        idle_socket_it = it;
+        max_benefit = benefit;
+        best_pair = std::make_pair(bytes_read, idle_time);
+        rtt_ms = it->socket->GetRTTInMs();
+      }
     }
 
     ++it;
   }
+  debug_log = StringPrintf("%s\nChose socket <%d %ld>\nRtt = %f",
+                           debug_log.data(),
+                           best_pair.first, best_pair.second,
+                           rtt_ms);
+  LOG(ERROR) << "\n\n----------------------------------------" << debug_log;
 
   // If we haven't found an idle socket, that means there are no used idle
   // sockets.  Pick the oldest (first) idle socket (FIFO).
@@ -649,6 +686,15 @@
   return old_value;
 }
 
+// static
+double ClientSocketPoolBaseHelper::set_tcp_socket_estimated_cwnd_decay_coef(
+    double coef) {
+  LOG(ERROR) << "Setting g_tcp_socket_estimated_cwnd_decay_coef = " << coef;

[Mike Belshe, 2011/06/09 16:08:52]

nit:  this isn't a LOG(ERROR), LOG(DEBUG) or remove?

[Gagan, 2011/06/09 19:49:26]

Changed to LOG(DEBUG)

+  double old_value = g_tcp_socket_estimated_cwnd_decay_coef;
+  g_tcp_socket_estimated_cwnd_decay_coef = coef;
+  return old_value;
+}
+
 void ClientSocketPoolBaseHelper::EnableConnectBackupJobs() {
   connect_backup_jobs_enabled_ = g_connect_backup_jobs_enabled;
 }