Add a cap of six in-flight requests per host to the ResourceScheduler

Add a cap of six in-flight requests per host to the ResourceScheduler

There's a cap of 10 delayable in-flight requests already present in the ResourceScheduler, but due to the underlying cap on six connections per host we sometimes don't utilize all ten if they're all for the same host. This CL adds an additional per-host cap, meaning if there's already six in-flight requests going to foo.bar.com, another request to foo.bar.com will be skipped in favour of a later-queued request to foo2.bar.com.

Tests from pmeenan@ suggests this results in an overall increase of onload and Speed Index speed by about 2.5%, and DOM Content Loaded speed by 1.15%. 

BUG=319488
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=235491

[James Simonsen, 2013-11-13 01:26:22]

Sorry for taking so long!

Can you update the CL description to include the results? We should have a
record in the git history of why we made this change.

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:367: DCHECK(query_result ==
kDoNotStartRequest_StopSearching);
I'd prefer it be a switch instead of if/else. Let the compiler take care of
making sure all of the cases are covered instead of finding this in a debug
build at runtime. The D in DCHECK means only crash if we're debug.

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:373: void
ResourceScheduler::GetNumDelayableRequestsInFlight(
It feels like we're doing a lot of work here now. Can we punt that to data
structures instead?

We could use a multimap to hold the inflight_delayable requests. Then you could
get the total number from size() and the number for this host from count(). The
downside is searching for the one to remove.

Alternatively, use a map of sets. The downside is handling empty map entries.

Any other ideas? I prefer the first one.

Either way, I think we're better off keeping inflight delayable and inflight
immediate requests separate. I should've done that when I wrote it.

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:375: net::HostPortPair*
active_request_host,
This should be a const reference. It shouldn't ever be null.

As much as possible, you should only ever pass a raw pointer if you expect the
callee to modify the object. Otherwise, it needs to be const and a pointer only
if it's nullable.

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:386:
net::HostPortPair::FromURL((*it)->url_request()->url());
Yeah, the multimap or whatever would be nice here. Then we don't have to
repeatedly parse the URL.

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:396: if
(!http_server_properties.SupportsSpdy(host_port_pair)) {
Maybe we should keep these in a separate set.

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.h:21: class HostPortPair;
Alphabetize.

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.h:117:
kDoNotStartRequest_StopSearching = -2,
Is this proper style now? net/ uses all caps for enums.

You don't seem to be taking advantage of the specific values. I'd leave them
undefined.

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_scheduler_unittest.cc (right):

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler_unittest.cc:331: net::LOWEST));
nit: fix indenting

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler_unittest.cc:354: net::LOWEST));
indent

[oystein (OOO til 10th of July), 2013-11-14 00:19:07]

Note: Not tested yet, not even sure if it compiles; will check when I get my
desktops up and running :).

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:367: DCHECK(query_result ==
kDoNotStartRequest_StopSearching);
On 2013/11/13 01:26:23, James Simonsen wrote:
> I'd prefer it be a switch instead of if/else. Let the compiler take care of
> making sure all of the cases are covered instead of finding this in a debug
> build at runtime. The D in DCHECK means only crash if we're debug.

The reason I didn't go with a switch is to avoid nasty gotos or a flag to be
able to break out of the outer while() loop; is the compile-time safety worth
the additional logic?

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:373: void
ResourceScheduler::GetNumDelayableRequestsInFlight(
On 2013/11/13 01:26:23, James Simonsen wrote:
> It feels like we're doing a lot of work here now. Can we punt that to data
> structures instead?
> 
> We could use a multimap to hold the inflight_delayable requests. Then you
could
> get the total number from size() and the number for this host from count().
The
> downside is searching for the one to remove.
> 
> Alternatively, use a map of sets. The downside is handling empty map entries.
> 
> Any other ideas? I prefer the first one.
> 
> Either way, I think we're better off keeping inflight delayable and inflight
> immediate requests separate. I should've done that when I wrote it.

I looked into this a bit now, I think the main problem is that we then have to
deal with multiple containers not only when removing a request, but also when
deleting a client and when reprioritizing a request. 

We could maintain a separate count of the delayable requests, but we'd still
have to loop through and check how many matches our current host so it doesn't
really gain us much.

Or we could store the requests in an std::map instead of an std::list and store
the net::HostPortPair as the value to avoid recreating it all the time

Thoughts?

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.h:21: class HostPortPair;
On 2013/11/13 01:26:23, James Simonsen wrote:
> Alphabetize.

Done.

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.h:117:
kDoNotStartRequest_StopSearching = -2,
On 2013/11/13 01:26:23, James Simonsen wrote:
> Is this proper style now? net/ uses all caps for enums.
> 
> You don't seem to be taking advantage of the specific values. I'd leave them
> undefined.

Changed; but this seems super inconsistent right now.
chrome/browser/net/referrer.cc uses the kSomeEnum case for example.

The values are to preserve the boolean semantics of ShouldStartRequest(), for
code safety.

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_scheduler_unittest.cc (right):

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler_unittest.cc:331: net::LOWEST));
On 2013/11/13 01:26:23, James Simonsen wrote:
> nit: fix indenting

Done.

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler_unittest.cc:354: net::LOWEST));
On 2013/11/13 01:26:23, James Simonsen wrote:
> indent

Done.

[James Simonsen, 2013-11-14 19:22:43]

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:373: void
ResourceScheduler::GetNumDelayableRequestsInFlight(
On 2013/11/14 00:19:07, Oystein wrote:
> On 2013/11/13 01:26:23, James Simonsen wrote:
> > It feels like we're doing a lot of work here now. Can we punt that to data
> > structures instead?
> > 
> > We could use a multimap to hold the inflight_delayable requests. Then you
> could
> > get the total number from size() and the number for this host from count().
> The
> > downside is searching for the one to remove.
> > 
> > Alternatively, use a map of sets. The downside is handling empty map
entries.
> > 
> > Any other ideas? I prefer the first one.
> > 
> > Either way, I think we're better off keeping inflight delayable and inflight
> > immediate requests separate. I should've done that when I wrote it.
> 
> I looked into this a bit now, I think the main problem is that we then have to
> deal with multiple containers not only when removing a request, but also when
> deleting a client and when reprioritizing a request. 
> 
> We could maintain a separate count of the delayable requests, but we'd still
> have to loop through and check how many matches our current host so it doesn't
> really gain us much.
> 
> Or we could store the requests in an std::map instead of an std::list and
store
> the net::HostPortPair as the value to avoid recreating it all the time
> 
> Thoughts?

There has to be an easier way to keep track of all this. I don't really want to
spend any more time on it though, so I'm okay with what we've got.

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.h:117:
kDoNotStartRequest_StopSearching = -2,
On 2013/11/14 00:19:07, Oystein wrote:
> On 2013/11/13 01:26:23, James Simonsen wrote:
> > Is this proper style now? net/ uses all caps for enums.
> > 
> > You don't seem to be taking advantage of the specific values. I'd leave them
> > undefined.
> 
> Changed; but this seems super inconsistent right now.
> chrome/browser/net/referrer.cc uses the kSomeEnum case for example.

I don't have a preference. We should just follow whatever the style guide says.
I don't think it has double underscores though. Maybe throw in an _AND_?

https://codereview.chromium.org/23620058/diff/105001/content/browser/loader/r...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/23620058/diff/105001/content/browser/loader/r...
content/browser/loader/resource_scheduler.h:112: Client* client,
These need to be indented 4 spaces. They're a line continuation.

[oystein (OOO til 10th of July), 2013-11-14 19:51:49]

Thanks :)

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:373: void
ResourceScheduler::GetNumDelayableRequestsInFlight(
On 2013/11/14 19:22:43, James Simonsen wrote:
> On 2013/11/14 00:19:07, Oystein wrote:
> > On 2013/11/13 01:26:23, James Simonsen wrote:
> > > It feels like we're doing a lot of work here now. Can we punt that to data
> > > structures instead?
> > > 
> > > We could use a multimap to hold the inflight_delayable requests. Then you
> > could
> > > get the total number from size() and the number for this host from
count().
> > The
> > > downside is searching for the one to remove.
> > > 
> > > Alternatively, use a map of sets. The downside is handling empty map
> entries.
> > > 
> > > Any other ideas? I prefer the first one.
> > > 
> > > Either way, I think we're better off keeping inflight delayable and
inflight
> > > immediate requests separate. I should've done that when I wrote it.
> > 
> > I looked into this a bit now, I think the main problem is that we then have
to
> > deal with multiple containers not only when removing a request, but also
when
> > deleting a client and when reprioritizing a request. 
> > 
> > We could maintain a separate count of the delayable requests, but we'd still
> > have to loop through and check how many matches our current host so it
doesn't
> > really gain us much.
> > 
> > Or we could store the requests in an std::map instead of an std::list and
> store
> > the net::HostPortPair as the value to avoid recreating it all the time
> > 
> > Thoughts?
> 
> There has to be an easier way to keep track of all this. I don't really want
to
> spend any more time on it though, so I'm okay with what we've got.

I agree, but I suggest we wait until the next change to this system and
reevaluate at that point. It'll hopefully become more clear then.

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.cc:375: net::HostPortPair*
active_request_host,
On 2013/11/13 01:26:23, James Simonsen wrote:
> This should be a const reference. It shouldn't ever be null.
> 
> As much as possible, you should only ever pass a raw pointer if you expect the
> callee to modify the object. Otherwise, it needs to be const and a pointer
only
> if it's nullable.

Done.

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/23620058/diff/45001/content/browser/loader/re...
content/browser/loader/resource_scheduler.h:117:
kDoNotStartRequest_StopSearching = -2,
On 2013/11/14 19:22:43, James Simonsen wrote:
> On 2013/11/14 00:19:07, Oystein wrote:
> > On 2013/11/13 01:26:23, James Simonsen wrote:
> > > Is this proper style now? net/ uses all caps for enums.
> > > 
> > > You don't seem to be taking advantage of the specific values. I'd leave
them
> > > undefined.
> > 
> > Changed; but this seems super inconsistent right now.
> > chrome/browser/net/referrer.cc uses the kSomeEnum case for example.
> 
> I don't have a preference. We should just follow whatever the style guide
says.
> I don't think it has double underscores though. Maybe throw in an _AND_?

Done.

https://codereview.chromium.org/23620058/diff/105001/content/browser/loader/r...
File content/browser/loader/resource_scheduler.h (right):

https://codereview.chromium.org/23620058/diff/105001/content/browser/loader/r...
content/browser/loader/resource_scheduler.h:112: Client* client,
On 2013/11/14 19:22:44, James Simonsen wrote:
> These need to be indented 4 spaces. They're a line continuation.

Done.

[oystein (OOO til 10th of July), 2013-11-14 21:23:29]

szym: net/base/priority_queue* please :)

[szym, 2013-11-15 01:11:49]

https://codereview.chromium.org/23620058/diff/225001/net/base/priority_queue.h
File net/base/priority_queue.h (right):

https://codereview.chromium.org/23620058/diff/225001/net/base/priority_queue....
net/base/priority_queue.h:269: Pointer NextHighest(const Pointer& previous) {
Is this any different from GetNextTowardsLastMin?

[oystein (OOO til 10th of July), 2013-11-15 03:02:09]

https://codereview.chromium.org/23620058/diff/225001/net/base/priority_queue.h
File net/base/priority_queue.h (right):

https://codereview.chromium.org/23620058/diff/225001/net/base/priority_queue....
net/base/priority_queue.h:269: Pointer NextHighest(const Pointer& previous) {
On 2013/11/15 01:11:49, szym wrote:
> Is this any different from GetNextTowardsLastMin?

This patch was written before GetNextTowardsLastMin() got added, as far as I can
tell they're compatible (except mine also handles .end() as an input). I'll try
modifying my patch to use the existing function, thanks.

[oystein (OOO til 10th of July), 2013-11-15 19:24:55]

On 2013/11/15 03:02:09, Oystein wrote:
> https://codereview.chromium.org/23620058/diff/225001/net/base/priority_queue.h
> File net/base/priority_queue.h (right):
> 
>
https://codereview.chromium.org/23620058/diff/225001/net/base/priority_queue....
> net/base/priority_queue.h:269: Pointer NextHighest(const Pointer& previous) {
> On 2013/11/15 01:11:49, szym wrote:
> > Is this any different from GetNextTowardsLastMin?
> 
> This patch was written before GetNextTowardsLastMin() got added, as far as I
can
> tell they're compatible (except mine also handles .end() as an input). I'll
try
> modifying my patch to use the existing function, thanks.

Done; kept one of the unit tests I added (using the other traversal function),
as it seemed useful still.

[szym, 2013-11-15 19:48:52]

Thanks for the test.

LGTM but please rename the test to indicate it exercises GetNextTowardsLastMin.

https://codereview.chromium.org/23620058/diff/75002/net/base/priority_queue_u...
File net/base/priority_queue_unittest.cc (right):

https://codereview.chromium.org/23620058/diff/75002/net/base/priority_queue_u...
net/base/priority_queue_unittest.cc:92: TEST_F(PriorityQueueTest,
FirstMaxOrderWithDeletion) {
The name suggests this is the same as FirstMaxOrderErase.

Better name would be GetNextTowardsLastMinAndErase.

I also would be ok with this test only exercising GetNextTowardsLastMin.

[James Simonsen, 2013-11-15 19:50:51]

lgtm

[oystein (OOO til 10th of July), 2013-11-15 19:52:20]

Thanks guys!

https://codereview.chromium.org/23620058/diff/75002/net/base/priority_queue_u...
File net/base/priority_queue_unittest.cc (right):

https://codereview.chromium.org/23620058/diff/75002/net/base/priority_queue_u...
net/base/priority_queue_unittest.cc:92: TEST_F(PriorityQueueTest,
FirstMaxOrderWithDeletion) {
On 2013/11/15 19:48:52, szym wrote:
> The name suggests this is the same as FirstMaxOrderErase.
> 
> Better name would be GetNextTowardsLastMinAndErase.
> 
> I also would be ok with this test only exercising GetNextTowardsLastMin.

Done.

[commit-bot: I haz the power, 2013-11-15 20:05:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oysteine@chromium.org/23620058/445001

[commit-bot: I haz the power, 2013-11-15 21:46:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oysteine@chromium.org/23620058/445001

[commit-bot: I haz the power, 2013-11-16 00:12:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/oysteine@chromium.org/23620058/445001

[commit-bot: I haz the power, 2013-11-16 01:51:37]

Change committed as 235491