Basic HTTP pipelining support

net/http/http_stream_factory_impl_job.cc

Index: net/http/http_stream_factory_impl_job.cc
diff --git a/net/http/http_stream_factory_impl_job.cc b/net/http/http_stream_factory_impl_job.cc
index 895266891977b8481f3c0bf0d55bea2426defc39..7af320f4c2ad662d836f969dc9eed1c6e3cadc2b 100644
--- a/net/http/http_stream_factory_impl_job.cc
+++ b/net/http/http_stream_factory_impl_job.cc
@@ -16,6 +16,10 @@
 #include "net/base/ssl_cert_request_info.h"
 #include "net/http/http_basic_stream.h"
 #include "net/http/http_network_session.h"
+#include "net/http/http_pipelined_connection.h"
+#include "net/http/http_pipelined_host.h"
+#include "net/http/http_pipelined_host_pool.h"
+#include "net/http/http_pipelined_stream.h"
 #include "net/http/http_proxy_client_socket.h"
 #include "net/http/http_proxy_client_socket_pool.h"
 #include "net/http/http_request_info.h"
@@ -85,6 +89,7 @@ HttpStreamFactoryImpl::Job::Job(HttpStreamFactoryImpl* stream_factory,
       was_npn_negotiated_(false),
       num_streams_(0),
       spdy_session_direct_(false),
+      existing_available_pipeline_(NULL),
       ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {
   DCHECK(stream_factory);
   DCHECK(session);
@@ -580,6 +585,18 @@ int HttpStreamFactoryImpl::Job::DoInitConnection() {
   } else if (request_ && (using_ssl_ || ShouldForceSpdyWithoutSSL())) {
     // Update the spdy session key for the request that launched this job.
     request_->SetSpdySessionKey(spdy_session_key);
+  } else if (IsRequestEligibleForPipelining()) {
+    HttpPipelinedHost* host = session_->http_pipelined_host_pool()->
+        GetPipelinedHostIfExists(origin_);
+    if (host) {
+      HttpPipelinedConnection* connection = host->FindAvailablePipeline();
+      if (connection) {
+        next_state_ = STATE_CREATE_STREAM;
+        existing_available_pipeline_ = connection;
+        return OK;
+      }
+    }
+    request_->SetHttpPipeliningKey(origin_);
   }
 
   // OK, there's no available SPDY session. Let |dependent_job_| resume if it's
@@ -757,7 +774,8 @@ int HttpStreamFactoryImpl::Job::DoWaitingUserAction(int result) {
 }
 
 int HttpStreamFactoryImpl::Job::DoCreateStream() {
-  if (!connection_->socket() && !existing_spdy_session_)
+  if (!connection_->socket() && !existing_spdy_session_ &&
+      !existing_available_pipeline_)

[willchan no longer on Chromium, 2011/07/19 13:29:38]

Heh, this is used to debug bug 80095. You probably shouldn't use it. You
probably want a DCHECK(connection_->socket() || existing_spdy_session_ ||
existing_available_pipeline_); Put that on the next line, then later on when I
remove the HACK function, your assertion will be the valid one.

[James Simonsen, 2011/07/19 22:54:23]

Done, but I still need to avoid falling into the HACK when I use an existing
pipeline, so I still need to modify the if().

     HACKCrashHereToDebug80095();
 
   next_state_ = STATE_CREATE_STREAM_COMPLETE;
@@ -773,8 +791,22 @@ int HttpStreamFactoryImpl::Job::DoCreateStream() {
   if (!using_spdy_) {
     bool using_proxy = (proxy_info_.is_http() || proxy_info_.is_https()) &&
         request_info_.url.SchemeIs("http");
-    stream_.reset(new HttpBasicStream(connection_.release(), NULL,
-                                      using_proxy));
+    // TODO(simonjam): Support proxies.
+    if (existing_available_pipeline_) {
+      stream_.reset(new HttpPipelinedStream(existing_available_pipeline_));
+    } else if (!using_proxy && IsRequestEligibleForPipelining()) {
+      HttpPipelinedConnection* pipeline = session_->http_pipelined_host_pool()->
+          GetOrCreatePipelinedHost(origin_)->CreateNewPipeline(

[willchan no longer on Chromium, 2011/07/19 13:29:38]

I need to look at this a bit more. This is pretty key code. Currently this code
seems to make the assumption that for all hosts, we can pipeline requests to
them. Obviously, we'll be tying in heuristics here, and also need to check to
see if we already tried to pipeline to this host, and if it failed, we shouldn't
retry. Lots of logic should be going in around this location.

[James Simonsen, 2011/07/19 22:54:23]

Yeah, I'm still not sure exactly what we all need. For instance, will Google
keep a list of good/bad sites or will each client build their own? Anyway, I
don't intend to figure this out until we have some actual data.

+              connection_.release(),
+              ssl_config_,
+              proxy_info_,
+              net_log_,
+              was_npn_negotiated_);
+      stream_.reset(new HttpPipelinedStream(pipeline));
+    } else {
+      stream_.reset(new HttpBasicStream(connection_.release(), NULL,
+                                        using_proxy));
+    }
     return OK;
   }
 
@@ -1073,6 +1105,13 @@ bool HttpStreamFactoryImpl::Job::IsOrphaned() const {
   return !IsPreconnecting() && !request_;
 }
 
+bool HttpStreamFactoryImpl::Job::IsRequestEligibleForPipelining() const {

[willchan no longer on Chromium, 2011/07/19 13:29:38]

So, we probably want to expose pipelining via about:flags. That will let us set
a command line flag for this feature. You should correlate that command line
flag to a global variable here. When the command line flag is set to true, then
a global variable in this code should be set to true too. Note that command line
flags only exist in chrome/. For an example of how to do this, please refer to
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/browse....
That doesn't show how to tie it into about:flags, but you can probably read
http://codesearch.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/about_...
and figure it out pretty easily (I've never done it myself, but it looks easy).

[James Simonsen, 2011/07/19 22:54:23]

Done. Yep, easy.

+  if (IsPreconnecting() || !request_) {
+    return false;
+  }
+  return request_info_.method == "GET";

[willchan no longer on Chromium, 2011/07/19 13:29:38]

Allow other idempotent methods like HEAD?

[James Simonsen, 2011/07/19 22:54:23]

Done. HEAD seems the only other safe and useful one right now.

+}
+
 #if defined(OS_WIN)
 #pragma warning (disable: 4748)
 #pragma optimize( "", off )