[ResourceScheduler] Throttle H2/QUIC requests just like we do for 1.1

[ResourceScheduler] Throttle H2/QUIC requests just like we do for 1.1

This is a short-term fix to alleviate long delays on early requests when there
are many requests in flight. See the bug for more detail.

BUG=655585
Committed: https://crrev.com/a9d98bd309fd58129b40ee9fccefc2ef76732f6c
Cr-Commit-Position: refs/heads/master@{#426251}

[jkarlin, 2016-10-19 11:16:33]

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

[commit-bot: I haz the power, 2016-10-19 11:17:07]

Dry run: 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, 2016-10-19 11:51:09]

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

[commit-bot: I haz the power, 2016-10-19 11:51:10]

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

[jkarlin, 2016-10-19 15:23:59]

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

[commit-bot: I haz the power, 2016-10-19 15:24:33]

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

[jkarlin, 2016-10-19 15:24:46]

jkarlin@chromium.org changed reviewers:
+ csharrison@chromium.org

[jkarlin, 2016-10-19 15:24:47]

csharrison@ PTAL at everything, thanks!

[Charlie Harrison, 2016-10-19 15:40:43]

LGTM w/ nit

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

https://codereview.chromium.org/2435743002/diff/40001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:35: const base::Feature
kPrioritySupportedRequestsDelayable{
Can you add a comment about what this feature does? Its name is not very
illuminating.

[jkarlin, 2016-10-19 15:53:26]

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

[commit-bot: I haz the power, 2016-10-19 15:53:33]

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

[jkarlin, 2016-10-19 15:53:48]

Thanks!

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

https://codereview.chromium.org/2435743002/diff/40001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:35: const base::Feature
kPrioritySupportedRequestsDelayable{
On 2016/10/19 15:40:43, Charlie Harrison wrote:
> Can you add a comment about what this feature does? Its name is not very
> illuminating.

Done.

[Randy Smith (Not in Mondays), 2016-10-19 16:41:42]

rdsmith@chromium.org changed reviewers:
+ rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2016-10-19 16:41:43]

LGTM with offline conversation about making sure we're testing what the default
is.  The comments below can be used or ignored as you prefer.

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

https://codereview.chromium.org/2435743002/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:524: if
(base::FeatureList::IsEnabled(kPrioritySupportedRequestsDelayable)) {
I note that this lookup seems O(log n) (map) in the set of feature; given that
this code is run per-request, it might be worth a separate boolean.   This would
also make the change I raise below more readable.   Up to you.

https://codereview.chromium.org/2435743002/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:535: if
(!http_server_properties.SupportsRequestPriority(scheme_host_port))
I'll call out that another option (which probably would have been my preferred)
would have been to just combine the conditionals here rather than have the
larger if statement.  But I can see arguments both ways, and I presume you made
the choice you did consciously, so just a thought.

[jkarlin, 2016-10-19 16:59:51]

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

[commit-bot: I haz the power, 2016-10-19 17:00:26]

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

[jkarlin, 2016-10-19 17:01:41]

rdsmith@ Thanks, PTAL!

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

https://codereview.chromium.org/2435743002/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:524: if
(base::FeatureList::IsEnabled(kPrioritySupportedRequestsDelayable)) {
On 2016/10/19 16:41:43, Randy Smith - Not in Mondays wrote:
> I note that this lookup seems O(log n) (map) in the set of feature; given that
> this code is run per-request, it might be worth a separate boolean.   This
would
> also make the change I raise below more readable.   Up to you.

Done.

https://codereview.chromium.org/2435743002/diff/60001/content/browser/loader/...
content/browser/loader/resource_scheduler.cc:535: if
(!http_server_properties.SupportsRequestPriority(scheme_host_port))
On 2016/10/19 16:41:43, Randy Smith - Not in Mondays wrote:
> I'll call out that another option (which probably would have been my
preferred)
> would have been to just combine the conditionals here rather than have the
> larger if statement.  But I can see arguments both ways, and I presume you
made
> the choice you did consciously, so just a thought.

Acknowledged.

[Randy Smith (Not in Mondays), 2016-10-19 17:04:47]

Still LGTM.

[jkarlin, 2016-10-19 17:07:45]

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

[commit-bot: I haz the power, 2016-10-19 17:08:10]

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

[Randy Smith (Not in Mondays), 2016-10-19 17:09:27]

LGTM; I assume the compiler/try bots would catch any problems.

[commit-bot: I haz the power, 2016-10-19 18:16:58]

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

[commit-bot: I haz the power, 2016-10-19 18:16:59]

Dry run: This issue passed the CQ dry run.

[jkarlin, 2016-10-19 18:34:32]

The CQ bit was checked by jkarlin@chromium.org

[jkarlin, 2016-10-19 18:34:32]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2435743002/#ps120001
(title: "Nit")

[commit-bot: I haz the power, 2016-10-19 18:35:13]

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, 2016-10-19 18:46:33]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-10-21 13:10:25]

Description was changed from

==========
[ResourceScheduler] Throttle H2/QUIC requests just like we do for 1.1

This is a short-term fix to alleviate long delays on early requests when there
are many requests in flight. See the bug for more detail.

BUG=655585
==========

to

==========
[ResourceScheduler] Throttle H2/QUIC requests just like we do for 1.1

This is a short-term fix to alleviate long delays on early requests when there
are many requests in flight. See the bug for more detail.

BUG=655585

Committed: https://crrev.com/a9d98bd309fd58129b40ee9fccefc2ef76732f6c
Cr-Commit-Position: refs/heads/master@{#426251}
==========

[commit-bot: I haz the power, 2016-10-21 13:10:25]

Patchset 7 (id:??) landed as
https://crrev.com/a9d98bd309fd58129b40ee9fccefc2ef76732f6c
Cr-Commit-Position: refs/heads/master@{#426251}