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

net/http/http_basic_stream.cc

Index: net/http/http_basic_stream.cc
===================================================================
--- net/http/http_basic_stream.cc	(revision 88319)
+++ net/http/http_basic_stream.cc	(working copy)
@@ -4,6 +4,7 @@
 
 #include "net/http/http_basic_stream.h"
 
+#include "base/metrics/histogram.h"
 #include "base/stringprintf.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
@@ -13,6 +14,10 @@
 #include "net/http/http_util.h"
 #include "net/socket/client_socket_handle.h"
 
+namespace {
+std::string g_warm_connection_field_trial_group;

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

This is not allowed by the style guide, see
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Static....

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

I was following
http://codesearch.google.com/#OAMlx_jo-ck/src/net/socket/client_socket_pool_m...
as an example.
Changed to static member of class here, and kept the
client_socket_pool_manager.cc as static variable in file for file level
consistency.

If you want, in a separate small cl, i could move all static file variables of
client_socket_pool_manager.cc to static class members.

[willchan no longer on Chromium, 2011/06/10 15:05:55]

You misunderstand. It doesn't matter if it's a file local static or a class
static. They're both globals. They create static initializers and destructors
that lead to global construction/destruction ordering bugs/crashes and also slow
down startup/shutdown. The style guide prohibits global non-POD.

[Gagan, 2011/06/10 17:44:17]

Ah sorry, missed it completely. Changed to global int.

+}
+
 namespace net {
 
 HttpBasicStream::HttpBasicStream(ClientSocketHandle* connection,
@@ -34,6 +39,7 @@
   request_info_ = request_info;
   parser_.reset(new HttpStreamParser(connection_.get(), request_info,
                                      read_buf_, net_log));
+  bytes_read_offset_ = connection_->socket()->NumBytesRead();
   return OK;
 }
 
@@ -50,6 +56,7 @@
   request_line_ = base::StringPrintf("%s %s HTTP/1.1\r\n",
                                      request_info_->method.c_str(),
                                      path.c_str());
+  response_ = response;
   return parser_->SendRequest(request_line_, headers, request_body, response,
                               callback);
 }
@@ -119,4 +126,33 @@
   return false;
 }
 
+void HttpBasicStream::LogNumRttVsBytesMetrics() const {
+  int64 total_bytes_read = connection_->socket()->NumBytesRead();
+  int64 bytes_received = total_bytes_read - bytes_read_offset_;
+  int64 num_kb = bytes_received / 1024;
+  double rtt = connection_->socket()->GetRTTInMs();
+
+  if (num_kb < 1024 && rtt > 0) {  // Ignore responses > 1MB
+    base::TimeDelta duration = base::Time::Now() -
+                               response_->request_time;
+    double num_rtt = static_cast<double>(duration.InMilliseconds()) / rtt;
+    int64 num_rtt_scaled = (4 * num_rtt);
+    int val = 1024 * num_rtt_scaled + num_kb;  // Supports ~500 RTT of 1418 B.

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

Can you add comments about why you're doing this math?  I understood the
computation of num_rtt, (total time / rtt), but I don't understand the reason
for multiplying num_rtt * 4096 (4 * 1024).  Finally, you've got num_kb, which is
KB, and you're ADDING that to num_rtts, so the units don't match?

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

num_rtt is a double, so scaling it by 4 to basically distinguish 3.0, 3.25, 3.5,
3.75.

The metric used in histogram is more like a 2D graph.
The only measure of success of warmest connection first over last accessed first
i could think of was more number of bytes per rtt. this is kind of independent
of users browsing pattern.
So i thought i could uniquely represent each (num_kb, rtt) pair as 1024* num_rtt
+ num_kb

From the histogram of this metric, we can compute the num_kb vs rtt graph, and
the graph of warmest first should be higher.

Sorry for writing it in a confusing way, will be happy to hear other metrics
that can measure.

+
+    std::string histogram = "Net.RTT_vs_KB_";
+    histogram += g_warm_connection_field_trial_group;
+    HISTOGRAM_CUSTOM_COUNTS(histogram, val, 4096, 3048000, 1000);

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

bug:  This histogram code is incorrect, note that the HISTOGRAM_CUSTOM_COUNTS
macro creates a static, so the string for this histogram name cannot be dynamic
like this.

Comments to explain the size of this histogram would be interesting - 1000
buckets is a lot, btw.

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

the counter is static, but its value is the return value of
Histogram::FactoryGet() which takes the name. I don't think this is a problem.
Have verified the histogram name in about:histograms as well.
histogram buckets seem to be exponential in size. I want something with higher
granularity on the higher buckets because thats where the effect is more
visible.

+    LOG(ERROR) << request_line_
+               << "\nrtt = " << rtt << "\tnum_rtt = " << num_rtt
+               << "\tnum_kb = " << num_kb << "\tval = " << val
+               << "\ttotal bytes = " << total_bytes_read;
+  }
+}
+
+// static
+void HttpBasicStream::set_warm_connection_field_trial_group(
+    const std::string& group) {
+  g_warm_connection_field_trial_group = group;
+}
+
 }  // namespace net