predictors: Downgrade net priority of prefetch requests.

predictors: Downgrade net priority of prefetch requests.

Prefetch predictor currently uses net::LOW priority to fetch resources
that is higher than default priority for images, for example. This CL
sets net::IDLE level for such requests.
It allows to prioritize renderer requests above speculative ones.

BUG=631966
Review-Url: https://codereview.chromium.org/2692093006
Cr-Commit-Position: refs/heads/master@{#452847}
Committed: https://chromium.googlesource.com/chromium/src/+/d8c3159729df90d3b12c71c9ab6e5accd7bdf4aa

[alexilin, 2017-02-15 16:07:08]

alexilin@chromium.org changed reviewers:
+ lizeb@chromium.org, pasko@chromium.org

[alexilin, 2017-02-15 16:07:09]

While we don't come up to more complex solution, I propose to downgrade
prefetcher priority to DEFAULT_PRIORITY level as it's set for net::URLFetcher.
WDYT?

[pasko, 2017-02-16 18:14:23]

Following your logic we should make it net::IDLE. Default priority for images is
quite low because we do not render-block when an image is not available. On the
other hand, prefetch will likely pull something important, like a script or CSS,
which, if not available, would render-block. I think the whole system of these
priorities is deeply flawed, but I am hesitant to make this change without
re-running your perf tests that confirmed the good effect of speculative
prefetch even when it is very concurrent to the load. By the way, do you have a
report?

[pasko, 2017-02-16 18:14:40]

https://codereview.chromium.org/2692093006/diff/1/chrome/browser/predictors/r...
File chrome/browser/predictors/resource_prefetcher.cc (right):

https://codereview.chromium.org/2692093006/diff/1/chrome/browser/predictors/r...
chrome/browser/predictors/resource_prefetcher.cc:120: url,
net::DEFAULT_PRIORITY, this);
nit: net::LOWEST is slightly more readable

[pasko, 2017-02-22 14:17:21]

Pat suggested to use net::IDLE. If it does not regress our local test (afair it
does not), then I am all sold on net::IDLE. What about you guys?

[alexilin, 2017-02-23 10:04:06]

Description was changed from

==========
predictors: Downgrade net priority of prefetch requests.

Prefetch predictor currently uses net::LOW priority to fetch resources
that is higher than default priority for images, for example. This CL sets
net::DEFAULT_PRIORITY level for such requests.

BUG=631966
==========

to

==========
predictors: Downgrade net priority of prefetch requests.

Prefetch predictor currently uses net::LOW priority to fetch resources
that is higher than default priority for images, for example. This CL sets
net::IDLE level for such requests.
It allows to prioritize renderer requests above speculative ones.

BUG=631966
==========

[alexilin, 2017-02-23 10:04:32]

Description was changed from

==========
predictors: Downgrade net priority of prefetch requests.

Prefetch predictor currently uses net::LOW priority to fetch resources
that is higher than default priority for images, for example. This CL sets
net::IDLE level for such requests.
It allows to prioritize renderer requests above speculative ones.

BUG=631966
==========

to

==========
predictors: Downgrade net priority of prefetch requests.

Prefetch predictor currently uses net::LOW priority to fetch resources
that is higher than default priority for images, for example. This CL
sets net::IDLE level for such requests.
It allows to prioritize renderer requests above speculative ones.

BUG=631966
==========

[alexilin, 2017-02-23 10:25:38]

I agree. Prefetcher prioritizes prefetch requests by its own so a
RequestPriority value is only about prefetcher requests vs. renderer. 
It should be fair to prioritize renderer requests above speculative ones.

[pasko, 2017-02-24 15:05:04]

lgtm

in cases when you want reviewers to take another look, please ask for it (a
"PTAL" works as a trigger), otherwise the comment is usually perceived as "I
think this and that, will update the code later".

[alexilin, 2017-02-24 15:25:57]

Thanks!
I think this and that, and by the way I updated the code! PTAL!

[Benoit L, 2017-02-24 15:27:06]

On 2017/02/24 15:25:57, alexilin wrote:
> Thanks!
> I think this and that, and by the way I updated the code! PTAL!

lgtm

[alexilin, 2017-02-24 15:35:05]

The CQ bit was checked by alexilin@chromium.org

[commit-bot: I haz the power, 2017-02-24 15:35:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 16:35:07]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487950505882740,
"parent_rev": "69b28c5c195c33922ac2185df859e722e109e156", "commit_rev":
"d8c3159729df90d3b12c71c9ab6e5accd7bdf4aa"}

[commit-bot: I haz the power, 2017-02-24 16:35:48]

Description was changed from

==========
predictors: Downgrade net priority of prefetch requests.

Prefetch predictor currently uses net::LOW priority to fetch resources
that is higher than default priority for images, for example. This CL
sets net::IDLE level for such requests.
It allows to prioritize renderer requests above speculative ones.

BUG=631966
==========

to

==========
predictors: Downgrade net priority of prefetch requests.

Prefetch predictor currently uses net::LOW priority to fetch resources
that is higher than default priority for images, for example. This CL
sets net::IDLE level for such requests.
It allows to prioritize renderer requests above speculative ones.

BUG=631966

Review-Url: https://codereview.chromium.org/2692093006
Cr-Commit-Position: refs/heads/master@{#452847}
Committed:
https://chromium.googlesource.com/chromium/src/+/d8c3159729df90d3b12c71c9ab6e...
==========

[commit-bot: I haz the power, 2017-02-24 16:35:49]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d8c3159729df90d3b12c71c9ab6e...