Slow start pipelining.

Slow start pipelining.

We need to wait for an HTTP/1.1 keep-alive response before we try to pipeline. Notably, this fixes wordpress.com and techcrunch.com.

Remember which hosts clearly support, or don't support pipelining. If pipelining is supported, skip the slow start. If it's not, fall back to HttpBasicStreams.

A site is judged not to support pipelining if we see an old HTTP version or encounter a socket error. A site does support pipelining if it successfully handles 3 requests. There's obviously room for improvement here, but this is a
start.

Related changes:
- In the spirit of CHECK() failing. Use CHECK(false) instead of NOTREACHED().
- HttpPipelinedHost is now an interface with a corresponding Impl. This is to help unit test HttpPipelinedHostPool.

BUG=None
TEST=net_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112557

[James Simonsen, 2011-11-17 02:42:13]

I started in on this before you discovered the problem with Drain(). Thanks for
finding that.

Anyway, I figured it was easier to finish this CL than to change gears. I'll
work on Drain next.

For the record, the current state is:
- Drain is busted.
- Some sites constantly spin.
- NTLM authentication doesn't work.
- NetLog needs pipelining info.

[mmenke, 2011-11-17 15:32:14]

http://codereview.chromium.org/8586015/diff/1/net/http/http_pipelined_host_po...
File net/http/http_pipelined_host_pool.cc (right):

http://codereview.chromium.org/8586015/diff/1/net/http/http_pipelined_host_po...
net/http/http_pipelined_host_pool.cc:9: #include
"net/http/http_pipelined_host_impl.h"
Looks like the CL is missing a few files.

[James Simonsen, 2011-11-17 19:29:21]

On 2011/11/17 15:32:14, Matt Menke wrote:
>
http://codereview.chromium.org/8586015/diff/1/net/http/http_pipelined_host_po...
> File net/http/http_pipelined_host_pool.cc (right):
> 
>
http://codereview.chromium.org/8586015/diff/1/net/http/http_pipelined_host_po...
> net/http/http_pipelined_host_pool.cc:9: #include
> "net/http/http_pipelined_host_impl.h"
> Looks like the CL is missing a few files.

Bah. Fixed.

[mmenke, 2011-11-17 22:25:37]

Some preliminary comments/nits.  I'll do a more thorough pass tomorrow.

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
File net/http/http_pipelined_host_impl.cc (right):

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.cc:155: // just ignore these unless they're
common. Until then, fall through...
Just think of the case of losing a network connection, or spotty 4G.  Or a
computer going to sleep with sockets active.   That last one will result in
ERR_ABORTED, I believe, so can handle that particular case specifically, but
there are a whole host of other possible failure reasons.

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.cc:159: // which site, so I can debug it.
I'm no expert, but wouldn't surprise me if it did.  Could see completely
different modules handling requests for different files.  One could be
responsible for serving chunked-encoded streaming videos, and another for static
HTML files, for example.  If they generate the entire response themselves, they
could well return different versions in the headers.

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.cc:170: : capacity(1),
nit:  4 space indent.

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
File net/http/http_pipelined_host_impl.h (right):

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.h:46: bool was_npn_negotiated);
This function and the next two should all use virtual/OVERRIDE.

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.h:63: virtual const HostPortPair& origin()
const OVERRIDE;
Now that this is virtual, it's no longer just a lightweight accessor, so might
want to rename it GetOrigin.  There are a number of other cases of this in
HttpPipelinedConnection, but feel free to leave those for another time.

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
File net/http/http_pipelined_host_pool_unittest.cc (right):

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:21: static ClientSocketHandle*
kDummyConnection =
Instead of making these static, can we just stick everything in this in an
anonymous namespace?

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:55: const HostPortPair& origin()
const OVERRIDE { return origin_; }
virtual

[mmenke, 2011-11-17 22:27:08]

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
File net/http/http_pipelined_host_pool_unittest.cc (right):

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:21: static ClientSocketHandle*
kDummyConnection =
On 2011/11/17 22:25:37, Matt Menke wrote:
> Instead of making these static, can we just stick everything in this in an
> anonymous namespace?

*Everything in this file.

[James Simonsen, 2011-11-18 00:03:56]

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
File net/http/http_pipelined_host_impl.cc (right):

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.cc:155: // just ignore these unless they're
common. Until then, fall through...
On 2011/11/17 22:25:37, Matt Menke wrote:
> Just think of the case of losing a network connection, or spotty 4G.  Or a
> computer going to sleep with sockets active.   That last one will result in
> ERR_ABORTED, I believe, so can handle that particular case specifically, but
> there are a whole host of other possible failure reasons.

Okay, I think a whitelist is the safest thing to do. I'll implement that in the
connection. It's hard to know which errors we're likely to see and whether they
can be ignored. I erred on the safe side.

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.cc:159: // which site, so I can debug it.
On 2011/11/17 22:25:37, Matt Menke wrote:
> I'm no expert, but wouldn't surprise me if it did.  Could see completely
> different modules handling requests for different files.  One could be
> responsible for serving chunked-encoded streaming videos, and another for
static
> HTML files, for example.  If they generate the entire response themselves,
they
> could well return different versions in the headers.

That's a good example. I guess I'll just take out the comment then. It'd be a
bummer if a site like this broke pipelining, which is why I wanted to see real
examples.

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.cc:170: : capacity(1),
On 2011/11/17 22:25:37, Matt Menke wrote:
> nit:  4 space indent.

Done. I really need to figure out how to get vim to do that correctly. You catch
these all the time.

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
File net/http/http_pipelined_host_impl.h (right):

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.h:46: bool was_npn_negotiated);
On 2011/11/17 22:25:37, Matt Menke wrote:
> This function and the next two should all use virtual/OVERRIDE.

Done.

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.h:63: virtual const HostPortPair& origin()
const OVERRIDE;
On 2011/11/17 22:25:37, Matt Menke wrote:
> Now that this is virtual, it's no longer just a lightweight accessor, so might
> want to rename it GetOrigin.  There are a number of other cases of this in
> HttpPipelinedConnection, but feel free to leave those for another time.

I added it to my task list. This CL is big enough.

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
File net/http/http_pipelined_host_pool_unittest.cc (right):

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:21: static ClientSocketHandle*
kDummyConnection =
On 2011/11/17 22:27:08, Matt Menke wrote:
> On 2011/11/17 22:25:37, Matt Menke wrote:
> > Instead of making these static, can we just stick everything in this in an
> > anonymous namespace?
> 
> *Everything in this file.

Done.

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:55: const HostPortPair& origin()
const OVERRIDE { return origin_; }
On 2011/11/17 22:25:37, Matt Menke wrote:
> virtual

Done.

[willchan no longer on Chromium, 2011-11-19 23:25:20]

I don't understand the changelist description of falling back to
HttpBasicStreams. If we detect HTTP/1.0, do we close the socket and create a new
one with HttpBasicStreams? It's obviously not desirable to close the socket, we
should simply not pipeline over it.

Reading through the implementation, it looks to me like the semantic is that we
always create a HttpPipelinedConnection and a HttpPipelinedHost on the first
connection to a site. If we find out that the HTTP version is too old, we mark
the HttpPipelinedHostImpl as incapable, so it won't ever indicate that the
pipeline is empty. This may be slightly confusing for someone new to the stack.
I wonder if we should do something like provide a way to upgrade a
HttpBasicStream to a HttpPipelinedHostImpl. What makes the most sense for when
we introduce something like pseudo pipelining? Would pseudo pipelining be merged
into HttpPipelinedHostImpl or would we use a separate class for it?

I'm just kicking around things for you to think about. I don't have any well
formulated thoughts.

And again, I will leave the nitty gritty review to Matt, thanks!

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_pool.h (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool.h:73: HttpPipelinedHost::Capability>
CapabilityMap;
I think you should be using HttpServerProperties here. This information strikes
me as being something that we want to persist. It sucks that we have to relearn
it each time. What do you think? We can keep it as a LRU cache, that makes
sense, but we should probably use the HttpServerProperties interface and
HttpServerPropertiesManager in chrome/browser/net to persist it.

Note that I'm also fine with you deferring this change to another changelist,
since you've already coded things up this way, and getting persistence working
separately is fine by me.

[mmenke, 2011-11-20 12:12:33]

On 2011/11/19 23:25:20, willchan wrote:
> I don't understand the changelist description of falling back to
> HttpBasicStreams. If we detect HTTP/1.0, do we close the socket and create a
new
> one with HttpBasicStreams? It's obviously not desirable to close the socket,
we
> should simply not pipeline over it.

I was thinking the opposite - we start with a "pipelined" stream with a max
capacity of 1, and then increase it if we discover pipelining is (apparently)
supported.  Otherwise, we just close the pipelined stream when it's done and
switch to a basic stream (Or keep the "pipelined" stream with one request
around.  Only issue there is prioritization of new requests).  I'm not quite
sure what HTTP pseudo-pipelining is, so can't comment on how that would work.

Using basic streams when support is unknown, and then switching over to
pipelined streams once we've read the basic stream's response and determined the
server theoretically supports pipelining also works.  I think switching over in
the middle of the basic stream transfer is a little too needlessly complicated
to implement.

[willchan no longer on Chromium, 2011-11-20 18:00:07]

On Sun, Nov 20, 2011 at 4:12 AM, <mmenke@chromium.org> wrote:

> On 2011/11/19 23:25:20, willchan wrote:
>
>> I don't understand the changelist description of falling back to
>> HttpBasicStreams. If we detect HTTP/1.0, do we close the socket and
>> create a
>>
> new
>
>> one with HttpBasicStreams? It's obviously not desirable to close the
>> socket,
>>
> we
>
>> should simply not pipeline over it.
>>
>
> I was thinking the opposite - we start with a "pipelined" stream with a max
> capacity of 1, and then increase it if we discover pipelining is
> (apparently)
> supported.  Otherwise, we just close the pipelined stream when it's done
> and
> switch to a basic stream (Or keep the "pipelined" stream with one request
> around.  Only issue there is prioritization of new requests).  I'm not
> quite
> sure what HTTP pseudo-pipelining is, so can't comment on how that would
> work.
>

As long as we don't close the socket, I'm fine. Transferring control of a
socket from a pipelined stream to a basic stream afterward seems reasonable
to me.


>
> Using basic streams when support is unknown, and then switching over to
> pipelined streams once we've read the basic stream's response and
> determined the
> server theoretically supports pipelining also works.  I think switching
> over in
> the middle of the basic stream transfer is a little too needlessly
> complicated
> to implement.
>

I see, that's a good point.


>
http://codereview.chromium.**org/8586015/<http://codereview.chromium.org/8586...
>

[mmenke, 2011-11-21 14:51:39]

Here are some more comments.  I apologize for not getting a full review in last
week, and given that I'm taking this week off, I'm unlikely to get around to it
this one.

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
File net/http/http_pipelined_host_impl.cc (right):

http://codereview.chromium.org/8586015/diff/3002/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.cc:155: // just ignore these unless they're
common. Until then, fall through...
On 2011/11/18 00:03:56, James Simonsen wrote:
> On 2011/11/17 22:25:37, Matt Menke wrote:
> > Just think of the case of losing a network connection, or spotty 4G.  Or a
> > computer going to sleep with sockets active.   That last one will result in
> > ERR_ABORTED, I believe, so can handle that particular case specifically, but
> > there are a whole host of other possible failure reasons.
> 
> Okay, I think a whitelist is the safest thing to do. I'll implement that in
the
> connection. It's hard to know which errors we're likely to see and whether
they
> can be ignored. I erred on the safe side.

Consider what servers that don't support pipelining are likely to do.  Note that
these are just fairly uneducated guesses based on no actual data.

They can:

1)  Abruptly close the connection immediately (ERR_CONNECTION_CLOSED /
ERR_EMPTY_RESPONSE on the socket, maybe ERR_CONTENT_LENGTH_MISMATCH on the
stream).
2)  Close the connection after sending the current file (ERR_EMPTY_RESPONSE)
3)  Intermingle responses for two requests.
4)  Ignore pipelined requests and just respond to the next request it sees, if
any, after servicing the first request.
5)  Hang?

All of the error codes in 1) and 2) can probably also occur as a result of
connection errors.  At least it's not uncommon for me to see those errors when
my router is mad.

3) and 4) will require special detection.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_conn...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_conn...
net/http/http_pipelined_connection_impl.cc:617: if (result < OK) {
When things go wrong, we won't necessarily get the error, if any, when receiving
headers.  I think we're going to have to play with things in the wild to see if
excluding certain error codes like this works at all.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_conn...
net/http/http_pipelined_connection_impl.cc:639: }
We should also be doing something about NTLM AUTH requests.  Currently, simplest
thing to do is to disallow pipelining and force the connection to close, in that
case.  Feel free to add it as a TODO in this CL.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host.h
File net/http/http_pipelined_host.h (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host.h:11: #include
"net/http/http_pipelined_connection.h"
Don't think we need this one.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host.h:75: virtual bool IsExistingPipelineAvailable() =
0;
Believe this can be const.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_impl.h (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.h:40: // HttpPipelinedHost Interface
nit:  interface is generally not capitalized in lines like this, elsewhere in
the code.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.h:50: virtual bool
IsExistingPipelineAvailable() OVERRIDE;
I believe this can be const.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.h:52: // HttpPipelinedConnection::Delegate
Interface
lowercase "interface"

[mmenke, 2011-11-28 23:05:28]

Still need to take a look at the last unittest file, but other than that, I've
looked over everything now.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_impl.cc (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.cc:115: capability_ = CAPABLE;
Seems a little weird to wait until shutting down a connection to do this. 
Suppose it's not as delayed as it seems at first, since this just happens
whenever we run out of requests, rather than when we close the socket.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_impl.h (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.h:69: int capacity;
Having capacity here strikes me as a little weird.  It seems more natural to me
to either make it a property of the PipelinedHost, and apply it to all its
streams, or to make it a property of the pipeline itself (Maybe along with
num_successes), rather than to have it here.

The one complexity of the first case is that we'd need to call
OnPipelineHasCapacity on all a host's streams once we decide a host supports
pipelining, but that may give us better performance, anyways.

What do you think?

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_impl_unittest.cc (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:20: static const int kMaxCapacity
= 3;
Think it's best to just use HttpPipelinedHostImpl::max_pipeline_depth() (Which
can be made static), so we don't have to duplicate this value all over the
place.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:25:
reinterpret_cast<ClientSocketHandle*>(84);
Can you use an anonymous namespace here, too?  And ditch the static.  Not that
there's anything wrong with static, but doing it to the whole file makes
conflicts less likely.  Same goes for your other tests.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:129:
TEST_F(HttpPipelinedHostImplTest, OnHostIdle) {
nit:  Maybe OnHostIdleUnusablePipeline?

Or maybe OnUnusablePipelineHasCapacity for this, and OnUsablePipelineHasCapacity
for the next one?

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:136:
host_->OnPipelineHasCapacity(pipeline);
Might want to make sure |pipeline| gets deleted.  Could have factory keep track,
or check that |host_| has no pipelines, or use some other method.  This goes for
a bunch of other tests in this file, too.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:143: .Times(2);
I think you should split this into two lines, one for each call.  Make for
clearer code.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:204: EXPECT_EQ(kDummyStream,
host_->CreateStreamOnExistingPipeline());
Is there really a need to create a dummy stream here?  I could see putting it in
another test, just to make sure the infrastructure is working, but seems kinda
odd in this test, since it doesn't have any effect on anything.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:224: EXPECT_EQ(kDummyStream,
host_->CreateStreamOnExistingPipeline());
You don't seem to have a test here that this call would fail before you call
OnPipelineFeedback.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:246: EXPECT_EQ(NULL,
host_->CreateStreamOnExistingPipeline());
This doesn't seem to be testing anything.  ClearTestPipeline sets the depth of
the pipeline to 0 and calls OnPipelineHasCapacity, which deletes pipelines with
a depth of 0, so there's no existing pipeline to use, regardless of whether or
not we correctly prevent pipelining in response to OLD_HTTP_VERSION.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_pool.cc (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool.cc:97: CapabilityMap::iterator known_it =
known_capability_map_.Get(origin);
Minor suggestion:  Create a GetCapability(origin) function, and have it do all
the logic here.  Then use it in IsHostEligibleForPipelining and
OnHostDeterminedCapability, too.  Think it's a little simpler than duplicating
code to search through the map, and handling the case where nothing is found.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_pool_unittest.cc (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:107: pool_->OnHostIdle(host_);
Might want to make sure |host_| gets deleted.  This goes for a bunch of other
tests in this file, too.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:146: pool_->OnHostIdle(host_);
nit:  The results would be the same on remembering nothing (or UNKNOWN).  Might
want to be a bit more picky, though not sure if that can be done easily.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:155: CreateDummyStream();
Maybe do a CAPABLE up here, too?

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:161: 
You might also want a test where we don't remember anything, just have a second
request for a pipeline on a host after destruction (And maybe before the call to
OnHostIdle, too).  We could live without them, but should be simple enough to
do.

[mmenke, 2011-11-29 16:52:20]

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_netw...
File net/http/http_pipelined_network_transaction_unittest.cc (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:100: void
CompleteTwoRequests(int data_index, int num_async) {
|num_async|'s meaning is not at all clear here, since it's not actually the
number of reads/writes tagged as asynchronous, but rather the number of
reads/writes that are performed asynchronously, after we wait for the first
write to have been sent, so the count generally includes the second write, which
is synchronous, but is started asynchronously by
HttpStreamFactoryImpl::Job::OnStreamReadyCallback.

That description is too confusing and obscure.  Instead, I'd just call it
stop_step, and use the index of the async event.  Then call SetStop(stop_step)
and Run().

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:238: MockWrite(false, 0,
"GET /one.html HTTP/1.1\r\n"
I think a comment that we're still pipelining here, despite the fact that each
send only completes after the previous file is received, because we
asynchronously remove deleted streams from the pipeline would make things
clearer.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:469:
TEST_F(HttpPipelinedNetworkTransactionTest, OldVersionDisablesPipelining) {
I suggest making the second and third responses HTTP/1.1, to make sure it
disables pipelining on other transactions, rather than on just the current
HTTP/1.0 transaction.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:469:
TEST_F(HttpPipelinedNetworkTransactionTest, OldVersionDisablesPipelining) {
I also think it would be good to have a "NoLengthDoesNotDisablePipelining" one,
which sends one with no length, and then makes sure that we still pipeline
afterwards.  Also maybe one that errors out.  Once you have the infrastructure,
adding these is pretty simple, and these tests are fast, so no reason not to
over test.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:521:
TEST_F(HttpPipelinedNetworkTransactionTest, PipelinesImmediatelyIfKnownGood) {
The first 4 transactions are just to label the pipeline as good, and then the
other two are to make sure pipelining is instantly set up on a newly created
pipelined connection, after deleting the first, which is why both sends are
before all the reads?  I think this is sufficiently non-obvious that a comment
about this would be useful.

I also think "five" and "six" should be renamed to "new-pipeline-one" and
"new-pipeline-two" or some such, to make it a little clearer where they fit into
things.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_stream_factory...
File net/http/http_stream_factory_impl.h (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_stream_factory...
net/http/http_stream_factory_impl.h:47: // HttpPipelinedHostPool::Delegate
Interface
nit:  lowercase "interface"

[James Simonsen, 2011-12-01 01:17:09]

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_conn...
File net/http/http_pipelined_connection_impl.cc (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_conn...
net/http/http_pipelined_connection_impl.cc:617: if (result < OK) {
On 2011/11/21 14:51:39, Matt Menke wrote:
> When things go wrong, we won't necessarily get the error, if any, when
receiving
> headers.  I think we're going to have to play with things in the wild to see
if
> excluding certain error codes like this works at all.

Okay, I've added a TODO. Eventually, I want to circle back and start
instrumenting all of this code so we get some metrics and can start tuning.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_conn...
net/http/http_pipelined_connection_impl.cc:639: }
On 2011/11/21 14:51:39, Matt Menke wrote:
> We should also be doing something about NTLM AUTH requests.  Currently,
simplest
> thing to do is to disallow pipelining and force the connection to close, in
that
> case.  Feel free to add it as a TODO in this CL.

Yeah, that was next on my list. I'm going to do it in a separate CL to directly
address the bug report.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host.h
File net/http/http_pipelined_host.h (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host.h:11: #include
"net/http/http_pipelined_connection.h"
On 2011/11/21 14:51:39, Matt Menke wrote:
> Don't think we need this one.

We need it for the HttpPipelinedConnection::Factory.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host.h:75: virtual bool IsExistingPipelineAvailable() =
0;
On 2011/11/21 14:51:39, Matt Menke wrote:
> Believe this can be const.

Yep. Done.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_impl.cc (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.cc:115: capability_ = CAPABLE;
On 2011/11/28 23:05:28, Matt Menke wrote:
> Seems a little weird to wait until shutting down a connection to do this. 
> Suppose it's not as delayed as it seems at first, since this just happens
> whenever we run out of requests, rather than when we close the socket.

Done.

Yeah, I don't think it matters in practice, so I'll do it where I think you
expected it, to make it more readable.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_impl.h (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.h:40: // HttpPipelinedHost Interface
On 2011/11/21 14:51:39, Matt Menke wrote:
> nit:  interface is generally not capitalized in lines like this, elsewhere in
> the code.

Done.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.h:50: virtual bool
IsExistingPipelineAvailable() OVERRIDE;
On 2011/11/21 14:51:39, Matt Menke wrote:
> I believe this can be const.

Done.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.h:52: // HttpPipelinedConnection::Delegate
Interface
On 2011/11/21 14:51:39, Matt Menke wrote:
> lowercase "interface"

Done.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl.h:69: int capacity;
On 2011/11/28 23:05:28, Matt Menke wrote:
> Having capacity here strikes me as a little weird.  It seems more natural to
me
> to either make it a property of the PipelinedHost, and apply it to all its
> streams, or to make it a property of the pipeline itself (Maybe along with
> num_successes), rather than to have it here.
> 
> The one complexity of the first case is that we'd need to call
> OnPipelineHasCapacity on all a host's streams once we decide a host supports
> pipelining, but that may give us better performance, anyways.
> 
> What do you think?

Good idea. I like the performance win. Done.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_impl_unittest.cc (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:20: static const int kMaxCapacity
= 3;
On 2011/11/28 23:05:28, Matt Menke wrote:
> Think it's best to just use HttpPipelinedHostImpl::max_pipeline_depth() (Which
> can be made static), so we don't have to duplicate this value all over the
> place.

Done.

FYI, I expect max_pipeline_capacity() to become non-const eventually though.
It'll probably be a ways down the road, but the general idea is to tune it based
on the user's network connection to the server. For example, lots of tiny
requests to a slow server should prefer fewer, deeper pipelines. Otherwise, we
should generally prefer to avoid pipelining, because of front-of-queue blocking
and evictions.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:25:
reinterpret_cast<ClientSocketHandle*>(84);
On 2011/11/28 23:05:28, Matt Menke wrote:
> Can you use an anonymous namespace here, too?  And ditch the static.  Not that
> there's anything wrong with static, but doing it to the whole file makes
> conflicts less likely.  Same goes for your other tests.

Done.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:129:
TEST_F(HttpPipelinedHostImplTest, OnHostIdle) {
On 2011/11/28 23:05:28, Matt Menke wrote:
> nit:  Maybe OnHostIdleUnusablePipeline?
> 
> Or maybe OnUnusablePipelineHasCapacity for this, and
OnUsablePipelineHasCapacity
> for the next one?

Done. Went with the latter.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:136:
host_->OnPipelineHasCapacity(pipeline);
On 2011/11/28 23:05:28, Matt Menke wrote:
> Might want to make sure |pipeline| gets deleted.  Could have factory keep
track,
> or check that |host_| has no pipelines, or use some other method.  This goes
for
> a bunch of other tests in this file, too.

I think we're already covered here. Valgrind will report any leaks. And
~HttpPipelinedHostImpl() will CHECK() fail if its pipelines_ map isn't empty.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:143: .Times(2);
On 2011/11/28 23:05:28, Matt Menke wrote:
> I think you should split this into two lines, one for each call.  Make for
> clearer code.

Done.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:204: EXPECT_EQ(kDummyStream,
host_->CreateStreamOnExistingPipeline());
On 2011/11/28 23:05:28, Matt Menke wrote:
> Is there really a need to create a dummy stream here?  I could see putting it
in
> another test, just to make sure the infrastructure is working, but seems kinda
> odd in this test, since it doesn't have any effect on anything.

Hmm. I guess not. The whole test doesn't seem all that useful. It probably lost
its meaning weeks ago.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:224: EXPECT_EQ(kDummyStream,
host_->CreateStreamOnExistingPipeline());
On 2011/11/28 23:05:28, Matt Menke wrote:
> You don't seem to have a test here that this call would fail before you call
> OnPipelineFeedback.

I'm probably misunderstanding you. The 4th line of this function is supposed to
be the failure before OnPipelineFeedback.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:246: EXPECT_EQ(NULL,
host_->CreateStreamOnExistingPipeline());
On 2011/11/28 23:05:28, Matt Menke wrote:
> This doesn't seem to be testing anything.  ClearTestPipeline sets the depth of
> the pipeline to 0 and calls OnPipelineHasCapacity, which deletes pipelines
with
> a depth of 0, so there's no existing pipeline to use, regardless of whether or
> not we correctly prevent pipelining in response to OLD_HTTP_VERSION.

Done.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_pool.cc (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool.cc:97: CapabilityMap::iterator known_it =
known_capability_map_.Get(origin);
On 2011/11/28 23:05:28, Matt Menke wrote:
> Minor suggestion:  Create a GetCapability(origin) function, and have it do all
> the logic here.  Then use it in IsHostEligibleForPipelining and
> OnHostDeterminedCapability, too.  Think it's a little simpler than duplicating
> code to search through the map, and handling the case where nothing is found.

Done.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_pool.h (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool.h:73: HttpPipelinedHost::Capability>
CapabilityMap;
On 2011/11/19 23:25:21, willchan wrote:
> I think you should be using HttpServerProperties here. This information
strikes
> me as being something that we want to persist. It sucks that we have to
relearn
> it each time. What do you think? We can keep it as a LRU cache, that makes
> sense, but we should probably use the HttpServerProperties interface and
> HttpServerPropertiesManager in chrome/browser/net to persist it.
> 
> Note that I'm also fine with you deferring this change to another changelist,
> since you've already coded things up this way, and getting persistence working
> separately is fine by me.

Yeah, that's definitely the way it should be done. I'll start that now. I think
it'll probably be better to land separately, just to keep the changes in
reasonably sized chunks.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_pool_unittest.cc (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:107: pool_->OnHostIdle(host_);
On 2011/11/28 23:05:28, Matt Menke wrote:
> Might want to make sure |host_| gets deleted.  This goes for a bunch of other
> tests in this file, too.

As with the other test file, I think this will be covered by Valgrind.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:146: pool_->OnHostIdle(host_);
On 2011/11/28 23:05:28, Matt Menke wrote:
> nit:  The results would be the same on remembering nothing (or UNKNOWN). 
Might
> want to be a bit more picky, though not sure if that can be done easily.

The difference is that CAPABLE is passed in to CreateNewHost(). If it hadn't
remembered anything, UNKNOWN would've been passed in.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:155: CreateDummyStream();
On 2011/11/28 23:05:28, Matt Menke wrote:
> Maybe do a CAPABLE up here, too?

Good idea. Done.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_netw...
File net/http/http_pipelined_network_transaction_unittest.cc (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:100: void
CompleteTwoRequests(int data_index, int num_async) {
On 2011/11/29 16:52:20, Matt Menke wrote:
> |num_async|'s meaning is not at all clear here, since it's not actually the
> number of reads/writes tagged as asynchronous, but rather the number of
> reads/writes that are performed asynchronously, after we wait for the first
> write to have been sent, so the count generally includes the second write,
which
> is synchronous, but is started asynchronously by
> HttpStreamFactoryImpl::Job::OnStreamReadyCallback.
> 
> That description is too confusing and obscure.  Instead, I'd just call it
> stop_step, and use the index of the async event.  Then call SetStop(stop_step)
> and Run().

Done. Good idea. I like that.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:469:
TEST_F(HttpPipelinedNetworkTransactionTest, OldVersionDisablesPipelining) {
On 2011/11/29 16:52:20, Matt Menke wrote:
> I suggest making the second and third responses HTTP/1.1, to make sure it
> disables pipelining on other transactions, rather than on just the current
> HTTP/1.0 transaction.

Done.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:469:
TEST_F(HttpPipelinedNetworkTransactionTest, OldVersionDisablesPipelining) {
On 2011/11/29 16:52:20, Matt Menke wrote:
> I also think it would be good to have a "NoLengthDoesNotDisablePipelining"
one,
> which sends one with no length, and then makes sure that we still pipeline
> afterwards.  Also maybe one that errors out.  Once you have the
infrastructure,
> adding these is pretty simple, and these tests are fast, so no reason not to
> over test.

I'm actually not crazy about adding a ton of tests here. I prefer to test at the
lower levels if at all possible. I'd like to leave this file for integration
tests, where several components all have to function correctly together. I think
the tests that are already here ensure that the proper signals are connected to
each other (i.e. CAPABLE and INCAPABLE are learned, remembered, and honored).

For the cases you mention, I'd rather just test at the lower levels and make
sure that unknown size and errors send the correct signals back to the
HttpPipelinedHost. I think we already have these.

Feel free to push back. Like you said, it's not hard to add them, but I'd like
to keep the test code manageable and minimize overlap.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:521:
TEST_F(HttpPipelinedNetworkTransactionTest, PipelinesImmediatelyIfKnownGood) {
On 2011/11/29 16:52:20, Matt Menke wrote:
> The first 4 transactions are just to label the pipeline as good, and then the
> other two are to make sure pipelining is instantly set up on a newly created
> pipelined connection, after deleting the first, which is why both sends are
> before all the reads?  I think this is sufficiently non-obvious that a comment
> about this would be useful.
> 
> I also think "five" and "six" should be renamed to "new-pipeline-one" and
> "new-pipeline-two" or some such, to make it a little clearer where they fit
into
> things.

Done.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_stream_factory...
File net/http/http_stream_factory_impl.h (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_stream_factory...
net/http/http_stream_factory_impl.h:47: // HttpPipelinedHostPool::Delegate
Interface
On 2011/11/29 16:52:20, Matt Menke wrote:
> nit:  lowercase "interface"

Done.

[mmenke, 2011-12-01 02:17:31]

Just a couple quick responses to your responses.  I'll do another (Hopefully
final) round of review tomorrow.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_impl_unittest.cc (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:20: static const int kMaxCapacity
= 3;
On 2011/12/01 01:17:10, James Simonsen wrote:
> On 2011/11/28 23:05:28, Matt Menke wrote:
> > Think it's best to just use HttpPipelinedHostImpl::max_pipeline_depth()
(Which
> > can be made static), so we don't have to duplicate this value all over the
> > place.
> 
> Done.
> 
> FYI, I expect max_pipeline_capacity() to become non-const eventually though.
> It'll probably be a ways down the road, but the general idea is to tune it
based
> on the user's network connection to the server. For example, lots of tiny
> requests to a slow server should prefer fewer, deeper pipelines. Otherwise, we
> should generally prefer to avoid pipelining, because of front-of-queue
blocking
> and evictions.

Yea, figured that was the reason, but then you'll need to adjust the code here
to compensate, anyways.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_impl_unittest.cc:224: EXPECT_EQ(kDummyStream,
host_->CreateStreamOnExistingPipeline());
On 2011/12/01 01:17:10, James Simonsen wrote:
> On 2011/11/28 23:05:28, Matt Menke wrote:
> > You don't seem to have a test here that this call would fail before you call
> > OnPipelineFeedback.
> 
> I'm probably misunderstanding you. The 4th line of this function is supposed
to
> be the failure before OnPipelineFeedback.

You aren't misunderstanding me.  I just managed to miss that call repeatedly
during review, somehow.  Maybe it's the hypnotic green background.  Carry on.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_pool_unittest.cc (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:107: pool_->OnHostIdle(host_);
On 2011/12/01 01:17:10, James Simonsen wrote:
> On 2011/11/28 23:05:28, Matt Menke wrote:
> > Might want to make sure |host_| gets deleted.  This goes for a bunch of
other
> > tests in this file, too.
> 
> As with the other test file, I think this will be covered by Valgrind.

There were one or two tests in particular that I was concerned about, but can't
remember which ones, and they look fine to me now.

That having been said, in general, I think if a significant part of a test
involves when/if something is deleted, depending on Valgrind is bad for a number
of reasons:

1) Long cycle time.
2) Can be hard to pin down the cause, particularly for a memory sheriff with
just a partial stack track containing mangled symbols.
3) In some cases, can be difficult even to figure out which test had the
failure.
4) Valgrind is a pain, requires developers to have it set up, isn't available on
windows, etc.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:146: pool_->OnHostIdle(host_);
On 2011/12/01 01:17:10, James Simonsen wrote:
> On 2011/11/28 23:05:28, Matt Menke wrote:
> > nit:  The results would be the same on remembering nothing (or UNKNOWN). 
> Might
> > want to be a bit more picky, though not sure if that can be done easily.
> 
> The difference is that CAPABLE is passed in to CreateNewHost(). If it hadn't
> remembered anything, UNKNOWN would've been passed in.

Ahh, right, good point.

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_netw...
File net/http/http_pipelined_network_transaction_unittest.cc (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_netw...
net/http/http_pipelined_network_transaction_unittest.cc:469:
TEST_F(HttpPipelinedNetworkTransactionTest, OldVersionDisablesPipelining) {
On 2011/12/01 01:17:10, James Simonsen wrote:
> On 2011/11/29 16:52:20, Matt Menke wrote:
> > I also think it would be good to have a "NoLengthDoesNotDisablePipelining"
> one,
> > which sends one with no length, and then makes sure that we still pipeline
> > afterwards.  Also maybe one that errors out.  Once you have the
> infrastructure,
> > adding these is pretty simple, and these tests are fast, so no reason not to
> > over test.
> 
> I'm actually not crazy about adding a ton of tests here. I prefer to test at
the
> lower levels if at all possible. I'd like to leave this file for integration
> tests, where several components all have to function correctly together. I
think
> the tests that are already here ensure that the proper signals are connected
to
> each other (i.e. CAPABLE and INCAPABLE are learned, remembered, and honored).
> 
> For the cases you mention, I'd rather just test at the lower levels and make
> sure that unknown size and errors send the correct signals back to the
> HttpPipelinedHost. I think we already have these.
> 
> Feel free to push back. Like you said, it's not hard to add them, but I'd like
> to keep the test code manageable and minimize overlap.

I think that's reasonable.  I just get a bit paranoid as the complexity of the
underlying code increases.

[mmenke, 2011-12-01 17:22:48]

LGTM

http://codereview.chromium.org/8586015/diff/21001/net/http/http_pipelined_host.h
File net/http/http_pipelined_host.h (right):

http://codereview.chromium.org/8586015/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host.h:30: PROBABLY_CAPABLE,
Maybe add a comment that this is a state, where we pipeline requests, but it's
not recorded by the HttpPipelinedHostPool?

http://codereview.chromium.org/8586015/diff/21001/net/http/http_pipelined_hos...
File net/http/http_pipelined_host_impl.cc (right):

http://codereview.chromium.org/8586015/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_impl.cc:137: OnPipelineHasCapacity(it->first);
We may want a unit test for this behavior at some point, but think we can live
without one for now.

http://codereview.chromium.org/8586015/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_impl.cc:141: kNumKnownSuccessesThreshold) {
Could you indent this 4 more spaces, to make it a little clearer that it's a
continuation of the comparison from the previous line?

http://codereview.chromium.org/8586015/diff/21001/net/http/http_pipelined_hos...
File net/http/http_pipelined_host_impl.h (right):

http://codereview.chromium.org/8586015/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_impl.h:67: static int max_pipeline_depth() { return
3; }
Think you'll need a NET_EXPORT_PRIVATE here.

http://codereview.chromium.org/8586015/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_impl.h:84: // Returns the current pipeline capacity
based on |capability_|.
Maybe add a note that it can't be called when capability is INCAPABLE?  Don't
think that's obvious from the description.

[James Simonsen, 2011-12-01 20:36:42]

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
File net/http/http_pipelined_host_pool_unittest.cc (right):

http://codereview.chromium.org/8586015/diff/6001/net/http/http_pipelined_host...
net/http/http_pipelined_host_pool_unittest.cc:107: pool_->OnHostIdle(host_);
On 2011/12/01 02:17:31, Matt Menke wrote:
> On 2011/12/01 01:17:10, James Simonsen wrote:
> > On 2011/11/28 23:05:28, Matt Menke wrote:
> > > Might want to make sure |host_| gets deleted.  This goes for a bunch of
> other
> > > tests in this file, too.
> > 
> > As with the other test file, I think this will be covered by Valgrind.
> 
> There were one or two tests in particular that I was concerned about, but
can't
> remember which ones, and they look fine to me now.
> 
> That having been said, in general, I think if a significant part of a test
> involves when/if something is deleted, depending on Valgrind is bad for a
number
> of reasons:
> 
> 1) Long cycle time.
> 2) Can be hard to pin down the cause, particularly for a memory sheriff with
> just a partial stack track containing mangled symbols.
> 3) In some cases, can be difficult even to figure out which test had the
> failure.
> 4) Valgrind is a pain, requires developers to have it set up, isn't available
on
> windows, etc.

Agreed.

We also have the CHECK() in the destructor to make sure the host map is empty
and we delete and remove from the map at the same time. Hopefully that keeps us
pretty well covered.

I've also seen GMock warn me if mock objects aren't deleted, but I'm not sure
how that works.

http://codereview.chromium.org/8586015/diff/21001/net/http/http_pipelined_host.h
File net/http/http_pipelined_host.h (right):

http://codereview.chromium.org/8586015/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host.h:30: PROBABLY_CAPABLE,
On 2011/12/01 17:22:48, Matt Menke wrote:
> Maybe add a comment that this is a state, where we pipeline requests, but it's
> not recorded by the HttpPipelinedHostPool?

Done.

http://codereview.chromium.org/8586015/diff/21001/net/http/http_pipelined_hos...
File net/http/http_pipelined_host_impl.cc (right):

http://codereview.chromium.org/8586015/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_impl.cc:137: OnPipelineHasCapacity(it->first);
On 2011/12/01 17:22:48, Matt Menke wrote:
> We may want a unit test for this behavior at some point, but think we can live
> without one for now.

Done.

http://codereview.chromium.org/8586015/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_impl.cc:141: kNumKnownSuccessesThreshold) {
On 2011/12/01 17:22:48, Matt Menke wrote:
> Could you indent this 4 more spaces, to make it a little clearer that it's a
> continuation of the comparison from the previous line?

Done.

http://codereview.chromium.org/8586015/diff/21001/net/http/http_pipelined_hos...
File net/http/http_pipelined_host_impl.h (right):

http://codereview.chromium.org/8586015/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_impl.h:67: static int max_pipeline_depth() { return
3; }
On 2011/12/01 17:22:48, Matt Menke wrote:
> Think you'll need a NET_EXPORT_PRIVATE here.

Done.

http://codereview.chromium.org/8586015/diff/21001/net/http/http_pipelined_hos...
net/http/http_pipelined_host_impl.h:84: // Returns the current pipeline capacity
based on |capability_|.
On 2011/12/01 17:22:48, Matt Menke wrote:
> Maybe add a note that it can't be called when capability is INCAPABLE?  Don't
> think that's obvious from the description.

Done.

[commit-bot: I haz the power, 2011-12-01 20:36:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/8586015/26004

[commit-bot: I haz the power, 2011-12-01 22:20:08]

Change committed as 112557