Save pipelining capabilities for the most used hosts between sessions.

Save pipelining capabilities for the most used hosts between sessions.

BUG=None
TEST=unit_tests


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

[mmenke, 2011-12-02 21:52:10]

Oops.  Thought I sent this earlier today.

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
File chrome/browser/net/http_server_properties_manager.cc (right):

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager.cc:314: int
pipeline_capability = 0;
Use net::HttpPipelinedHost::UNKNOWN to be a little clearer, and so correctness
here doesn't depend on net::HttpPipelinedHost::UNKNOWN being equal to 0.

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager.cc:316: "pipeline_capability",
&pipeline_capability)) && pipeline_capability) {
pipeline_capability != net::HttpPipelinedHost::UNKNOWN

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager.cc:443: // A local or
temporary data structure to hold supports_spdy, SpdySettings and
While you're here, mind replacing supports_spdy with |supports_spdy|?

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager.cc:445: // UpdatePrefsOnUI.
Update comment.

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager.cc:532:
pipeline_capability_map->begin();
Could you indent this 4 more spaces, to make it clearer it's a continuation of
the previous line, rather than the second clause of the for?

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager.cc:597: server.ToString(),
server_pref_dict);
Mind fixing the indentation in this old code while you're modifying this file?

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
File chrome/browser/net/http_server_properties_manager_unittest.cc (right):

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager_unittest.cc:313: //
ScheduleUpdatePrefsOnIO.
Think it's a little clearer if you put this just above the calls to
SetPipelineCapability, but up to you.

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

http://codereview.chromium.org/8770035/diff/1/net/http/http_pipelined_host_po...
net/http/http_pipelined_host_pool_unittest.cc:94:
scoped_ptr<HttpServerPropertiesImpl> http_server_properties_;
You might want to explicitly query this in a test or two, to make sure it's
populated.

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties.h
File net/http/http_server_properties.h (right):

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
net/http/http_server_properties.h:13: #include "net/http/http_pipelined_host.h"
You might want to move the Capability enumeration into its own file, so you
don't have to include this here.  While this doesn't include too much,
url_request_context.h does include this file, and it's used all over the place.

Alternatively, you could just use ints instead of HttpPipelinedHost::Capability.

Or you could keep it as is, I don't feel particularly strongly about this, just
thought I'd mention it.

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
net/http/http_server_properties.h:40: HttpPipelinedHost::Capability>
PipelineCapabilityMap;
Since changing how HttpPipelinedHost::Capability maps meaning to integers now
breaks things, I suggest you hard code values in the enumeration, and add a
comment that the exact values matter.

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
net/http/http_server_properties.h:108: virtual void ClearPipelineCapabilities()
= 0;
Was thinking we wouldn't need this, but may want it for unit testing.

Also, the SPDY equivalent is called when the network changes, since the meaning
of addresses may change.  We should probably do the same, though it seems a
little odd to me given that we keep it across browser sessions, between which
the network may change.

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
File net/http/http_server_properties_impl.cc (right):

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
net/http/http_server_properties_impl.cc:60: pipeline_capability_map_.Clear();
Whenever we call this, we basically put the pipeline capabilities in the MRU
cache in random order.  Looks like the only fix for this would be to either not
store pipeline capabilities in the ServerPref structure with the other
properties, or keep track of the order in the ServerPref structure, which seems
like too much effort.

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
File net/http/http_server_properties_impl.h (right):

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
net/http/http_server_properties_impl.h:46: void InitializePipelineCapabilities(
Might be nice to have a comment to make clear that it initializes the class
variable, and not the argument.

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
net/http/http_server_properties_impl.h:47: PipelineCapabilityMap*
pipeline_capability_map);
Think you can make this argument const.

[willchan no longer on Chromium, 2011-12-02 23:35:28]

Raman, please verify the HttpServerProperties and HttpServerPropertiesManager
code look good to you.

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
File net/http/http_server_properties_impl.cc (right):

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
net/http/http_server_properties_impl.cc:16: // then, this is just a bad guess.
Good idea. But perhaps we should put this in the constructor so that the
embedder can decide. Memory utilization is best decided by the embedder, which
will have a better knowledge of the memory availability on the platform.

[willchan no longer on Chromium, 2011-12-03 00:11:15]

BTW, everything LGTM. But I'd like Raman to give his approval too since he wrote
all the HttpServerProperties stuff.

[ramant (doing other things), 2011-12-03 00:31:43]

changes look good. Would be good to get a try bot run.

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
File chrome/browser/net/http_server_properties_manager_unittest.cc (right):

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager_unittest.cc:66:
net::AlternateProtocolMap* alternate_protocol_map));
shouldn't we add PipelineCapability to UpdateCacheFromPrefsOnIO

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager_unittest.cc:70:
net::AlternateProtocolMap* alternate_protocol_map));
shouldn't we add PipelineCapability to UpdateCacheFromPrefsOnIO

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager_unittest.cc:241: }
nit: please consider adding a test for PipelineCapability

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties.h
File net/http/http_server_properties.h (right):

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
net/http/http_server_properties.h:110: virtual PipelineCapabilityMap
GetPipelineCapabilityMap() const = 0;
nit: GetPipelineCapabilityMap => pipeline_capability_map

[mmenke, 2011-12-03 00:38:34]

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties.h
File net/http/http_server_properties.h (right):

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
net/http/http_server_properties.h:110: virtual PipelineCapabilityMap
GetPipelineCapabilityMap() const = 0;
On 2011/12/03 00:31:43, ramant wrote:
> nit: GetPipelineCapabilityMap => pipeline_capability_map

This is not a simple accessor.  It converts an MRUCache to an std::map.

[James Simonsen, 2011-12-03 03:16:00]

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
File chrome/browser/net/http_server_properties_manager.cc (right):

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager.cc:314: int
pipeline_capability = 0;
On 2011/12/02 21:52:10, Matt Menke wrote:
> Use net::HttpPipelinedHost::UNKNOWN to be a little clearer, and so correctness
> here doesn't depend on net::HttpPipelinedHost::UNKNOWN being equal to 0.

Done.

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager.cc:316: "pipeline_capability",
&pipeline_capability)) && pipeline_capability) {
On 2011/12/02 21:52:10, Matt Menke wrote:
> pipeline_capability != net::HttpPipelinedHost::UNKNOWN

Done.

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager.cc:443: // A local or
temporary data structure to hold supports_spdy, SpdySettings and
On 2011/12/02 21:52:10, Matt Menke wrote:
> While you're here, mind replacing supports_spdy with |supports_spdy|?

Done.

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager.cc:445: // UpdatePrefsOnUI.
On 2011/12/02 21:52:10, Matt Menke wrote:
> Update comment.

Done.

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager.cc:532:
pipeline_capability_map->begin();
On 2011/12/02 21:52:10, Matt Menke wrote:
> Could you indent this 4 more spaces, to make it clearer it's a continuation of
> the previous line, rather than the second clause of the for?

Done.

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager.cc:597: server.ToString(),
server_pref_dict);
On 2011/12/02 21:52:10, Matt Menke wrote:
> Mind fixing the indentation in this old code while you're modifying this file?

Done.

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
File chrome/browser/net/http_server_properties_manager_unittest.cc (right):

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager_unittest.cc:66:
net::AlternateProtocolMap* alternate_protocol_map));
On 2011/12/03 00:31:43, ramant wrote:
> shouldn't we add PipelineCapability to UpdateCacheFromPrefsOnIO

Done.

It's disconcerting that this compiled and all the tests passed.

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager_unittest.cc:70:
net::AlternateProtocolMap* alternate_protocol_map));
On 2011/12/03 00:31:43, ramant wrote:
> shouldn't we add PipelineCapability to UpdateCacheFromPrefsOnIO

Done.

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager_unittest.cc:241: }
On 2011/12/03 00:31:43, ramant wrote:
> nit: please consider adding a test for PipelineCapability

Done.

http://codereview.chromium.org/8770035/diff/1/chrome/browser/net/http_server_...
chrome/browser/net/http_server_properties_manager_unittest.cc:313: //
ScheduleUpdatePrefsOnIO.
On 2011/12/02 21:52:10, Matt Menke wrote:
> Think it's a little clearer if you put this just above the calls to
> SetPipelineCapability, but up to you.

Done.

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

http://codereview.chromium.org/8770035/diff/1/net/http/http_pipelined_host_po...
net/http/http_pipelined_host_pool_unittest.cc:94:
scoped_ptr<HttpServerPropertiesImpl> http_server_properties_;
On 2011/12/02 21:52:10, Matt Menke wrote:
> You might want to explicitly query this in a test or two, to make sure it's
> populated.

Done.

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties.h
File net/http/http_server_properties.h (right):

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
net/http/http_server_properties.h:13: #include "net/http/http_pipelined_host.h"
On 2011/12/02 21:52:10, Matt Menke wrote:
> You might want to move the Capability enumeration into its own file, so you
> don't have to include this here.  While this doesn't include too much,
> url_request_context.h does include this file, and it's used all over the
place.
> 
> Alternatively, you could just use ints instead of
HttpPipelinedHost::Capability.
> 
> Or you could keep it as is, I don't feel particularly strongly about this,
just
> thought I'd mention it.

Done. Yeah, seems like the right thing to do. The interface files aren't too
heavy now, but it'd be easy for them to grow without knowing the cost.

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
net/http/http_server_properties.h:40: HttpPipelinedHost::Capability>
PipelineCapabilityMap;
On 2011/12/02 21:52:10, Matt Menke wrote:
> Since changing how HttpPipelinedHost::Capability maps meaning to integers now
> breaks things, I suggest you hard code values in the enumeration, and add a
> comment that the exact values matter.

Done.

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
net/http/http_server_properties.h:108: virtual void ClearPipelineCapabilities()
= 0;
On 2011/12/02 21:52:10, Matt Menke wrote:
> Was thinking we wouldn't need this, but may want it for unit testing.

It seems it's mainly used for when the user clears their history. Technically,
pipelining knowledge would be part of that too.

> Also, the SPDY equivalent is called when the network changes, since the
meaning
> of addresses may change.  We should probably do the same, though it seems a
> little odd to me given that we keep it across browser sessions, between which
> the network may change.

I was picturing a separate layer for that.

If the user's network changes, it's possible they now have an invisible proxy
that breaks pipelining. In that case, we should just disable pipelining for the
entire network session rather than trying to track it per host.

The plan was to forcefully pipeline a couple of background test requests to
chromium.org any time the network changes. If those pass, then we deem the
session capable of pipelining and enable per-host pipelining (aka what we have
now).

Anyway, it's on my list to do that, assuming we find some broken proxies in the
wild.

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
File net/http/http_server_properties_impl.cc (right):

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
net/http/http_server_properties_impl.cc:16: // then, this is just a bad guess.
On 2011/12/02 23:35:29, willchan wrote:
> Good idea. But perhaps we should put this in the constructor so that the
> embedder can decide. Memory utilization is best decided by the embedder, which
> will have a better knowledge of the memory availability on the platform.

Hmm. I started in on this, but that constructor is all over the place and I
suspect most users have no idea what to set it to.

Instead, how about we go with a conservative default and then provide the option
to resize it? The MRUCache can easily be resized and I've added the code for
that.

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
net/http/http_server_properties_impl.cc:60: pipeline_capability_map_.Clear();
On 2011/12/02 21:52:10, Matt Menke wrote:
> Whenever we call this, we basically put the pipeline capabilities in the MRU
> cache in random order.  Looks like the only fix for this would be to either
not
> store pipeline capabilities in the ServerPref structure with the other
> properties, or keep track of the order in the ServerPref structure, which
seems
> like too much effort.

Yeah, I know. :( I debated it for a bit, but decided this is probably good
enough, at least for now. We can always fix it later on. I want to start getting
some metrics before playing around with the code too much more.

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
File net/http/http_server_properties_impl.h (right):

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
net/http/http_server_properties_impl.h:46: void InitializePipelineCapabilities(
On 2011/12/02 21:52:10, Matt Menke wrote:
> Might be nice to have a comment to make clear that it initializes the class
> variable, and not the argument.

Done.

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
net/http/http_server_properties_impl.h:47: PipelineCapabilityMap*
pipeline_capability_map);
On 2011/12/02 21:52:10, Matt Menke wrote:
> Think you can make this argument const.

Done.

[ramant (doing other things), 2011-12-05 19:05:21]

LGTM (please fix the try-bot errors).

[James Simonsen, 2011-12-05 22:38:29]

On 2011/12/05 19:05:21, ramant wrote:
> LGTM (please fix the try-bot errors).

Forgot to 'git add' the new file. Fixed now. The sync errors look spurious.

[mmenke, 2011-12-06 15:50:38]

LGTM

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
File net/http/http_server_properties_impl.cc (right):

http://codereview.chromium.org/8770035/diff/1/net/http/http_server_properties...
net/http/http_server_properties_impl.cc:60: pipeline_capability_map_.Clear();
On 2011/12/03 03:16:00, James Simonsen wrote:
> On 2011/12/02 21:52:10, Matt Menke wrote:
> > Whenever we call this, we basically put the pipeline capabilities in the MRU
> > cache in random order.  Looks like the only fix for this would be to either
> not
> > store pipeline capabilities in the ServerPref structure with the other
> > properties, or keep track of the order in the ServerPref structure, which
> seems
> > like too much effort.
> 
> Yeah, I know. :( I debated it for a bit, but decided this is probably good
> enough, at least for now. We can always fix it later on. I want to start
getting
> some metrics before playing around with the code too much more.

Sounds good to me.  Just wanted to make sure you were thinking about it.

http://codereview.chromium.org/8770035/diff/9001/base/memory/mru_cache.h
File base/memory/mru_cache.h (right):

http://codereview.chromium.org/8770035/diff/9001/base/memory/mru_cache.h#newc...
base/memory/mru_cache.h:155: void SetMaxSize(size_t new_max_size) {
You're going to need a signoff from an owner of base to land this CL.  Even
without this added function, you still have the const iterator fixes.

http://codereview.chromium.org/8770035/diff/9001/net/http/http_server_propert...
File net/http/http_server_properties_impl.cc (right):

http://codereview.chromium.org/8770035/diff/9001/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:18: static const int kNumHostsToRemember
= 200;
nit:  This should now be kDefaultNumHostsToRemember or something along those
lines.

http://codereview.chromium.org/8770035/diff/9001/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:68: void
HttpServerPropertiesImpl::SetNumPipelinedHostsToRemember(int max_size) {
Just to make things simple, you could just require this to be created on
construction, and recreate the map from scratch, with a DCHECK that it's empty. 
Up to you, but I don't think there's much need for it to be changed after
startup, and think it's best to keep things simple when there's no need for
complexiy.

[James Simonsen, 2011-12-06 19:57:49]

http://codereview.chromium.org/8770035/diff/9001/base/memory/mru_cache.h
File base/memory/mru_cache.h (right):

http://codereview.chromium.org/8770035/diff/9001/base/memory/mru_cache.h#newc...
base/memory/mru_cache.h:155: void SetMaxSize(size_t new_max_size) {
On 2011/12/06 15:50:38, Matt Menke wrote:
> You're going to need a signoff from an owner of base to land this CL.  Even
> without this added function, you still have the const iterator fixes.

Yeah, Will's an owner and he's already given LGTM for the const changes.

http://codereview.chromium.org/8770035/diff/9001/net/http/http_server_propert...
File net/http/http_server_properties_impl.cc (right):

http://codereview.chromium.org/8770035/diff/9001/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:18: static const int kNumHostsToRemember
= 200;
On 2011/12/06 15:50:38, Matt Menke wrote:
> nit:  This should now be kDefaultNumHostsToRemember or something along those
> lines.

Done.

http://codereview.chromium.org/8770035/diff/9001/net/http/http_server_propert...
net/http/http_server_properties_impl.cc:68: void
HttpServerPropertiesImpl::SetNumPipelinedHostsToRemember(int max_size) {
On 2011/12/06 15:50:38, Matt Menke wrote:
> Just to make things simple, you could just require this to be created on
> construction, and recreate the map from scratch, with a DCHECK that it's
empty. 
> Up to you, but I don't think there's much need for it to be changed after
> startup, and think it's best to keep things simple when there's no need for
> complexiy.

Ok. Done.

[mmenke, 2011-12-06 20:02:55]

You're right about the base thing.  (And still LGTM, modulo comment)

http://codereview.chromium.org/8770035/diff/22001/net/http/http_server_proper...
File net/http/http_server_properties_impl.cc (right):

http://codereview.chromium.org/8770035/diff/22001/net/http/http_server_proper...
net/http/http_server_properties_impl.cc:70:
CHECK(pipeline_capability_map_->empty());
Think this one should actually be a DCHECK.  Otherwise, a bug in usage could
completely hose the whole browser.

[commit-bot: I haz the power, 2011-12-07 00:04:33]

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

[commit-bot: I haz the power, 2011-12-07 01:21:28]

Change committed as 113315