Effectively disable the predictor's DNS queue for an experiment

Effectively disable the predictor's DNS queue for an experiment

This patch lets the predictor send as many requests as it needs to the
host resolver. This is a test to see if the intermediate queues are
really necessary for performance, as they add complexity to the codebase.

BUG=610750
Committed: https://crrev.com/362027367e6bfd0a4203c030746f41bb74201afb
Cr-Commit-Position: refs/heads/master@{#393141}

[Charlie Harrison, 2016-05-10 18:30:14]

csharrison@chromium.org changed reviewers:
+ asvitkine@chromium.org, rdsmith@chromium.org

[Charlie Harrison, 2016-05-10 18:30:14]

PTAL. I expect I'll just run this experiment on Dev/Canary.

[Alexei Svitkine (slow), 2016-05-10 19:23:04]

https://codereview.chromium.org/1967663002/diff/20001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/1967663002/diff/20001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:62: "DisablePredictorDNSQueue",
base::FEATURE_DISABLED_BY_DEFAULT};
I think this is confusing because you have a feature named "Disable" and it's
set as Disabled by default.

How about: kEnablePredictorDNSQueue or kUsePredictorDNSQueue and have it enabled
by default? This way, you avoid the double negative.

[Charlie Harrison, 2016-05-10 20:42:14]

https://codereview.chromium.org/1967663002/diff/20001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/1967663002/diff/20001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:62: "DisablePredictorDNSQueue",
base::FEATURE_DISABLED_BY_DEFAULT};
On 2016/05/10 19:23:04, Alexei Svitkine wrote:
> I think this is confusing because you have a feature named "Disable" and it's
> set as Disabled by default.
> 
> How about: kEnablePredictorDNSQueue or kUsePredictorDNSQueue and have it
enabled
> by default? This way, you avoid the double negative.

Thanks for the suggestion. Done.

[Alexei Svitkine (slow), 2016-05-10 20:48:41]

lgtm

https://codereview.chromium.org/1967663002/diff/40001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/1967663002/diff/40001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:61: const base::Feature kUsePredictorDNSQueue {
Nit: No indent.

[Charlie Harrison, 2016-05-11 13:29:14]

Thanks!

https://codereview.chromium.org/1967663002/diff/40001/chrome/browser/net/pred...
File chrome/browser/net/predictor.cc (right):

https://codereview.chromium.org/1967663002/diff/40001/chrome/browser/net/pred...
chrome/browser/net/predictor.cc:61: const base::Feature kUsePredictorDNSQueue {
On 2016/05/10 20:48:41, Alexei Svitkine wrote:
> Nit: No indent.

Done.

[Randy Smith (Not in Mondays), 2016-05-11 19:27:26]

This looks basically good, but can you give me a pointer to how base::Feature
interacts with field trials?  The comments in feature.h don't appear to address
how one would, e.g., create a field trial to (in this case) disable this
feature.

[Charlie Harrison, 2016-05-11 20:53:56]

On 2016/05/11 19:27:26, Randy Smith - Not in Fridays wrote:
> This looks basically good, but can you give me a pointer to how base::Feature
> interacts with field trials?  The comments in feature.h don't appear to
address
> how one would, e.g., create a field trial to (in this case) disable this
> feature.

This is the common way of doing field trials with only two groups now. In finch
you can selectively disable_features or enable_features.

[Alexei Svitkine (slow), 2016-05-11 21:04:09]

See internal go/feature-api for how base::Features can be driven by
server-side field trials.

On Wed, May 11, 2016 at 4:53 PM, <csharrison@chromium.org> wrote:

> On 2016/05/11 19:27:26, Randy Smith - Not in Fridays wrote:
> > This looks basically good, but can you give me a pointer to how
> base::Feature
> > interacts with field trials? The comments in feature.h don't appear to
> address
> > how one would, e.g., create a field trial to (in this case) disable this
> > feature.
>
> This is the common way of doing field trials with only two groups now. In
> finch
> you can selectively disable_features or enable_features.
>
> https://codereview.chromium.org/1967663002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Randy Smith (Not in Mondays), 2016-05-11 21:06:13]

lgtm.  Thank you both.

[Charlie Harrison, 2016-05-11 23:15:50]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-05-11 23:15:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1967663002/#ps60001
(title: "Fix indent")

[commit-bot: I haz the power, 2016-05-11 23:16:13]

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

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

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

[commit-bot: I haz the power, 2016-05-11 23:19:03]

Try jobs failed on following builders:
  ios-device on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Charlie Harrison, 2016-05-11 23:27:51]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-05-11 23:27:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/1967663002/#ps80001
(title: "Rebase on #393103")

[commit-bot: I haz the power, 2016-05-11 23:28:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1967663002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1967663002/80001

[commit-bot: I haz the power, 2016-05-12 01:39:59]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-05-12 01:42:32]

Description was changed from

==========
Effectively disable the predictor's DNS queue for an experiment

This patch lets the predictor send as many requests as it needs to the
host resolver. This is a test to see if the intermediate queues are
really necessary for performance, as they add complexity to the codebase.

BUG=610750
==========

to

==========
Effectively disable the predictor's DNS queue for an experiment

This patch lets the predictor send as many requests as it needs to the
host resolver. This is a test to see if the intermediate queues are
really necessary for performance, as they add complexity to the codebase.

BUG=610750

Committed: https://crrev.com/362027367e6bfd0a4203c030746f41bb74201afb
Cr-Commit-Position: refs/heads/master@{#393141}
==========

[commit-bot: I haz the power, 2016-05-12 01:42:33]

Patchset 5 (id:??) landed as
https://crrev.com/362027367e6bfd0a4203c030746f41bb74201afb
Cr-Commit-Position: refs/heads/master@{#393141}