Delay resource scheduling decisions until network access.

Delay resource scheduling decisions until network access.

Moved the ResourceScheduler to operate on requests before they use the
network instead of as soon as they are created.  This gets it out of the
way of the cache and service workers (including the new tab page).

BUG=587507

[Pat Meenan, 2016-02-17 20:22:22]

The CQ bit was checked by pmeenan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-17 20:24:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1706903003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1706903003/1

[commit-bot: I haz the power, 2016-02-17 20:53:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-17 20:53:14]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Pat Meenan, 2016-02-18 14:50:11]

The CQ bit was checked by pmeenan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-18 14:51:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1706903003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1706903003/20001

[commit-bot: I haz the power, 2016-02-18 15:21:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-18 15:21:34]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Pat Meenan, 2016-02-18 21:12:01]

The CQ bit was checked by pmeenan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-18 21:16:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1706903003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1706903003/40001

[commit-bot: I haz the power, 2016-02-18 21:47:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-18 21:47:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Pat Meenan, 2016-02-19 17:59:50]

Patchset #3 (id:40001) has been deleted

[Pat Meenan, 2016-02-19 18:06:42]

The CQ bit was checked by pmeenan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-19 18:09:37]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1706903003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1706903003/60001

[Pat Meenan, 2016-02-19 18:14:22]

Description was changed from

==========
Only schedule resources as they require network access.

Moved the ResourceScheduler to operate on requests before they use the
network instead of as soon as they are created.  This gets it out of the
way of the cache, service workers and any extensions that may block or
otherwise modify them.

BUG=587507
==========

to

==========
Only schedule resources as they require network access.

Moved the ResourceScheduler to operate on requests before they use the
network instead of as soon as they are created.  This gets it out of the
way of the cache and service workers (including the new tab page).

BUG=587507
==========

[Pat Meenan, 2016-02-19 18:15:40]

Description was changed from

==========
Only schedule resources as they require network access.

Moved the ResourceScheduler to operate on requests before they use the
network instead of as soon as they are created.  This gets it out of the
way of the cache and service workers (including the new tab page).

BUG=587507
==========

to

==========
Delay resource scheduling decisions until network access.

Moved the ResourceScheduler to operate on requests before they use the
network instead of as soon as they are created.  This gets it out of the
way of the cache and service workers (including the new tab page).

BUG=587507
==========

[commit-bot: I haz the power, 2016-02-19 19:18:11]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-02-19 19:18:11]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Pat Meenan, 2016-02-19 20:19:16]

pmeenan@chromium.org changed reviewers:
+ jkarlin@chromium.org, mmenke@chromium.org

[Pat Meenan, 2016-02-19 20:19:17]

mmenke@ could you PTAL?

I'm not expecting this to be a huge win on anything but it could save a few ms
here and there for Serviceworker-sourced fetched and cached resources.

The dry run failures on Mac and Linux look unrelated.

[mmenke, 2016-02-22 16:52:30]

On 2016/02/19 20:19:17, Pat Meenan wrote:
> mmenke@ could you PTAL?
> 
> I'm not expecting this to be a huge win on anything but it could save a few ms
> here and there for Serviceworker-sourced fetched and cached resources.
> 
> The dry run failures on Mac and Linux look unrelated.

I'm behind on reviews - probably won't get to this until tomorrow.

[mmenke, 2016-02-25 15:21:05]

Sorry I didn't get to this in the past two days - this fell off my radar
(Commenting on it makes the review queue no longer bold it, and given all the
reviews coming my way this week, just overlooked it).

This seems like something we should run field a trial for - unclear if disk
contention contention will be an issue or not.

Going to do a full review now, just wanted to state that up front.

[jkarlin, 2016-02-25 15:43:05]

On 2016/02/25 15:21:05, mmenke wrote:
> Sorry I didn't get to this in the past two days - this fell off my radar
> (Commenting on it makes the review queue no longer bold it, and given all the
> reviews coming my way this week, just overlooked it).
> 
> This seems like something we should run field a trial for - unclear if disk
> contention contention will be an issue or not.
> 
> Going to do a full review now, just wanted to state that up front.

I think the easiest way to do this with a field trial is the following CL which
I made 2 years ago and just dusted off:
https://codereview.chromium.org/1732363002/

Happy to put it up for review.

[mmenke, 2016-02-25 18:09:51]

https://codereview.chromium.org/1706903003/diff/60001/content/browser/loader/...
File content/browser/loader/resource_scheduler.cc (right):

https://codereview.chromium.org/1706903003/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:168: }
Fix indent (Or just run git cl format)

https://codereview.chromium.org/1706903003/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:299: void
ScheduleRequest(net::URLRequest* url_request,
SchedulerRequest -> AddRequest?  This is no longer doing any actual scheduling.

https://codereview.chromium.org/1706903003/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:305: bool
ShouldSendRequest(ScheduledResourceRequest* request) {
Think this needs a description

https://codereview.chromium.org/1706903003/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:305: bool
ShouldSendRequest(ScheduledResourceRequest* request) {
This should be renamed.  "Should" implies no side effects, and doing nothing but
returning a bool.

https://codereview.chromium.org/1706903003/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:310:
pending_requests_.Insert(request);
pending_requests_ -> paused_requests_, maybe?

https://codereview.chromium.org/1706903003/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:317:
pending_requests_.Erase(request);
Why did you remove the DCHECK here?

https://codereview.chromium.org/1706903003/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:319: EraseInFlightRequest(request);
nit:  Network stack team generally uses braces for all else-ifs.  Since we're
now the owns who now own this file, should probably stick with our style.

https://codereview.chromium.org/1706903003/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:321: all_requests_.erase(request);
We should update the list before calling SetRequestAttributes, like we do
elsewhere, since you remove the current_request parameter of
SetRequestAttributes.

This order happens not to cause the DCHECKs at the end of SetRequestAttributes
to trigger, but...

https://codereview.chromium.org/1706903003/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:422: it !=
in_flight_requests_.end(); ++it) {
Switch to range loops while you're here, on both of these loops?

https://codereview.chromium.org/1706903003/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:691: RequestSet all_requests_;
Hrm...  I really don't like keeping around a third set of requests, instead of
the original two.  Can we just get rid of one of the old ones?  Could even get
rid of both, I suppose, though that may have performance implications.

https://codereview.chromium.org/1706903003/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:792: bool
ResourceScheduler::ShouldSendRequest(ScheduledResourceRequest* request) {
Per earlier comment, the number of side effects this has makes "Should*" a
confusing name for this method.

https://codereview.chromium.org/1706903003/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:795: if (client_it ==
client_map_.end()) {
Think this warrants a comment.

https://codereview.chromium.org/1706903003/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:797: }
nit:  Remove braces.

https://codereview.chromium.org/1706903003/diff/60001/content/browser/loader/...
File content/browser/loader/resource_scheduler_unittest.cc (right):

https://codereview.chromium.org/1706903003/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler_unittest.cc:80: void Send() {
I want to spend some time mulling over this change...This way of testing
requests seems pretty hideous to me, and adding more weirdness to it doesn't
seem like a good direction to me.

https://codereview.chromium.org/1706903003/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler_unittest.cc:259: request->Send();
We have four types of requests now:

1) Those that have not yet reached WillStartUsingNetwork
2) Those that have reached it, and were let through immediately.
3) Those that have reached it, and were paused.
4) Those that have reached it, were paused, and were then let through.

With this approach, 2-4 have analogs in the old code / tests.  1) does not.

In particular, we should test at least these cases:
1) A request is in state 1) when it's cancelled.
2) A request is in state 1) when we detach the client and start all requests for
it.  We should make sure the request is allowed through when it does reach the
network, and completes happily.
3) A low priority request in state 1) has no impact on letting another low
priority request through when that other request is about to reach the network.
4) A high priority request in state 1) *does* prevent low priority requests from
going through (whenever the condition is for that logic is true).

[kinuko, 2016-02-26 02:11:07]

(cc-ing serviceworker-reviews@)

[mmenke, 2016-02-26 02:32:50]

On 2016/02/26 02:11:07, kinuko wrote:
> (cc-ing serviceworker-reviews@)

Hrm...ServiceWorker case is interesting.  The jobs that send a request back to
the ServiceWorker are associated with a RenderFrame, so they go through the
scheduler, but they never call OnBeforeNetworkStart, because they don't directly
go over the network, so they'll just bypass the scheduler with this change.

ServiceWorkerWriteToCacheJobs do pass along OnBeforeNetworkStart, but they don't
implement ResumeNetworkStart, so this CL currently would break them...  Except
ServiceWorkerWriteToCacheJobs aren't associated with a client, so they actually
don't quite break.  Regardless, that ServiceWorker bug should be fixed.  Long
term, we really should figure out how ServiceWorker fits into things.

[mmenke, 2016-04-08 15:23:32]

On 2016/02/26 02:32:50, mmenke wrote:
> On 2016/02/26 02:11:07, kinuko wrote:
> > (cc-ing serviceworker-reviews@)
> 
> Hrm...ServiceWorker case is interesting.  The jobs that send a request back to
> the ServiceWorker are associated with a RenderFrame, so they go through the
> scheduler, but they never call OnBeforeNetworkStart, because they don't
directly
> go over the network, so they'll just bypass the scheduler with this change.
> 
> ServiceWorkerWriteToCacheJobs do pass along OnBeforeNetworkStart, but they
don't
> implement ResumeNetworkStart, so this CL currently would break them...  Except
> ServiceWorkerWriteToCacheJobs aren't associated with a client, so they
actually
> don't quite break.  Regardless, that ServiceWorker bug should be fixed.  Long
> term, we really should figure out how ServiceWorker fits into things.

Note that the current plan is to remove ResourceScheduler and move it down to
the net/ layer, so I think we can abandon this CL - we can try to hook it up to
before network stack after the move.  Also, dropping this lets us remove
URLRequest::Delegate::OnBeforeNetworkStart, to cut some bloat from our APIs. 
Removing myself as a reviewer (My pending review list is getting hard to sort
through).

[mmenke, 2016-04-08 15:23:41]

mmenke@chromium.org changed reviewers:
- mmenke@chromium.org

[Pat Meenan, 2016-04-11 13:59:54]

On 2016/04/08 15:23:32, mmenke wrote:
> On 2016/02/26 02:32:50, mmenke wrote:
> > On 2016/02/26 02:11:07, kinuko wrote:
> > > (cc-ing serviceworker-reviews@)
> > 
> > Hrm...ServiceWorker case is interesting.  The jobs that send a request back
to
> > the ServiceWorker are associated with a RenderFrame, so they go through the
> > scheduler, but they never call OnBeforeNetworkStart, because they don't
> directly
> > go over the network, so they'll just bypass the scheduler with this change.
> > 
> > ServiceWorkerWriteToCacheJobs do pass along OnBeforeNetworkStart, but they
> don't
> > implement ResumeNetworkStart, so this CL currently would break them... 
Except
> > ServiceWorkerWriteToCacheJobs aren't associated with a client, so they
> actually
> > don't quite break.  Regardless, that ServiceWorker bug should be fixed. 
Long
> > term, we really should figure out how ServiceWorker fits into things.
> 
> Note that the current plan is to remove ResourceScheduler and move it down to
> the net/ layer, so I think we can abandon this CL - we can try to hook it up
to
> before network stack after the move.  Also, dropping this lets us remove
> URLRequest::Delegate::OnBeforeNetworkStart, to cut some bloat from our APIs. 
> Removing myself as a reviewer (My pending review list is getting hard to sort
> through).

Yep, closing it out - thanks for taking a look.