[net] Add AsyncDns field trial.

[net] Add AsyncDns field trial.

This adds AsyncDns field trial which sets --enable-async-dns for 50% of users
who have not configured any HostResolverImpl-related flags.

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

[szym, 2012-08-17 18:56:08]

I think this is the simplest way to do that without taking any technical debt.
PTAL.

[cbentzel, 2012-08-20 11:45:56]

http://codereview.chromium.org/10828373/diff/11/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/10828373/diff/11/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:531: kOsWin | kOsMac | kOsLinux | kOsCrOS,
Should you remove kOsCros at this point? I thought there were still issues with
DNS config?

http://codereview.chromium.org/10828373/diff/11/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/10828373/diff/11/chrome/browser/io_thread.cc#n...
chrome/browser/io_thread.cc:117: if
(command_line.HasSwitch(switches::kHostResolverParallelism)) {
Did you see if we could just get rid of the kHostResolverParallelism and
kHostResolverRetryAttempts flags?

http://codereview.chromium.org/10828373/diff/11/chrome/browser/io_thread.cc#n...
chrome/browser/io_thread.cc:149: if
(command_line.HasSwitch(switches::kEnableAsyncDns)) {
Why isn't this just --async-dns=[enabled|disabled|auto]?

Seems like the about:flags approach would steer towards that.

http://codereview.chromium.org/10828373/diff/11/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10828373/diff/11/net/base/host_resolver_impl.c...
net/base/host_resolver_impl.cc:471: const base::FieldTrial::Probability
kAsyncDnsDivisor = 100;
Perhaps restrict enabled_probability to non-0 values for canary+dev channels
only to be safe.

[szym, 2012-08-20 17:54:21]

http://codereview.chromium.org/10828373/diff/11/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/10828373/diff/11/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:531: kOsWin | kOsMac | kOsLinux | kOsCrOS,
On 2012/08/20 11:45:56, cbentzel wrote:
> Should you remove kOsCros at this point? I thought there were still issues
with
> DNS config?

There are, but I'm currently working on the merger of DnsConfigService into
NetworkChangeNotifier which will re-enable it for CrOS. I could disable it for
now if you prefer.

http://codereview.chromium.org/10828373/diff/11/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/10828373/diff/11/chrome/browser/io_thread.cc#n...
chrome/browser/io_thread.cc:117: if
(command_line.HasSwitch(switches::kHostResolverParallelism)) {
On 2012/08/20 11:45:56, cbentzel wrote:
> Did you see if we could just get rid of the kHostResolverParallelism and
> kHostResolverRetryAttempts flags?

Judging from the old bugs which established our current level of parallelism at
6-8, I concluded that reducing parallelism is likely still a very useful tool
when diagnosing network issues. Conceivably, we could replace both flags with
--disable-resolver-parallelism. I asked eroman about this.

http://codereview.chromium.org/10828373/diff/11/chrome/browser/io_thread.cc#n...
chrome/browser/io_thread.cc:149: if
(command_line.HasSwitch(switches::kEnableAsyncDns)) {
On 2012/08/20 11:45:56, cbentzel wrote:
> Why isn't this just --async-dns=[enabled|disabled|auto]?
> 
> Seems like the about:flags approach would steer towards that.

I followed the existing flags. There's currently 8 ternary flags in
about_flags.cc. 4/8 use --XXX=enabled|disabled while 4/8 use
--enable-XXX|--disable-XXX. There's also a number of the latter which are not in
about_flags.cc (XXX = spdy, ipv6, etc.).

Would you prefer to see --async-dns?

http://codereview.chromium.org/10828373/diff/11/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10828373/diff/11/net/base/host_resolver_impl.c...
net/base/host_resolver_impl.cc:471: const base::FieldTrial::Probability
kAsyncDnsDivisor = 100;
On 2012/08/20 11:45:56, cbentzel wrote:
> Perhaps restrict enabled_probability to non-0 values for canary+dev channels
> only to be safe.

Good idea although net cannot depend on chrome/ and only chrome/ knows channels.
I could just disable the field trial in io_thread.cc for the other channels, but
to have the flexibility of per-channel probability, I'll have to move this to
chrome/browser/net/async_dns_field_trial.{h,cc}.

[cbentzel, 2012-08-21 10:50:53]

I also wonder if you need to disable this field trial in browser_tests.

http://codereview.chromium.org/10828373/diff/11/chrome/browser/about_flags.cc
File chrome/browser/about_flags.cc (right):

http://codereview.chromium.org/10828373/diff/11/chrome/browser/about_flags.cc...
chrome/browser/about_flags.cc:531: kOsWin | kOsMac | kOsLinux | kOsCrOS,
On 2012/08/20 17:54:21, szym wrote:
> On 2012/08/20 11:45:56, cbentzel wrote:
> > Should you remove kOsCros at this point? I thought there were still issues
> with
> > DNS config?
> 
> There are, but I'm currently working on the merger of DnsConfigService into
> NetworkChangeNotifier which will re-enable it for CrOS. I could disable it for
> now if you prefer.

Yeah, disable for now especially since this change will opt users in.

http://codereview.chromium.org/10828373/diff/11/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/10828373/diff/11/chrome/browser/io_thread.cc#n...
chrome/browser/io_thread.cc:149: if
(command_line.HasSwitch(switches::kEnableAsyncDns)) {
On 2012/08/20 17:54:21, szym wrote:
> On 2012/08/20 11:45:56, cbentzel wrote:
> > Why isn't this just --async-dns=[enabled|disabled|auto]?
> > 
> > Seems like the about:flags approach would steer towards that.
> 
> I followed the existing flags. There's currently 8 ternary flags in
> about_flags.cc. 4/8 use --XXX=enabled|disabled while 4/8 use
> --enable-XXX|--disable-XXX. There's also a number of the latter which are not
in
> about_flags.cc (XXX = spdy, ipv6, etc.).
> 
> Would you prefer to see --async-dns?

No - it sounds different from common practice.

http://codereview.chromium.org/10828373/diff/11/net/base/host_resolver_impl.cc
File net/base/host_resolver_impl.cc (right):

http://codereview.chromium.org/10828373/diff/11/net/base/host_resolver_impl.c...
net/base/host_resolver_impl.cc:471: const base::FieldTrial::Probability
kAsyncDnsDivisor = 100;
On 2012/08/20 17:54:21, szym wrote:
> On 2012/08/20 11:45:56, cbentzel wrote:
> > Perhaps restrict enabled_probability to non-0 values for canary+dev channels
> > only to be safe.
> 
> Good idea although net cannot depend on chrome/ and only chrome/ knows
channels.
> I could just disable the field trial in io_thread.cc for the other channels,
but
> to have the flexibility of per-channel probability, I'll have to move this to
> chrome/browser/net/async_dns_field_trial.{h,cc}.

Yes. I personally think it makes sense there - you don't want to run this field
trial for non-Chrome embedders of the network stack.

http://codereview.chromium.org/10828373/diff/11/net/base/host_resolver_impl.c...
net/base/host_resolver_impl.cc:478: int enabled_group =
trial->AppendGroup("enabled", enabled_probability);
Should this be restricted to supported platforms only? What if 50% are "enabled"
in iOS or Android?

[szym, 2012-08-21 13:39:49]

On 2012/08/21 10:50:53, cbentzel wrote:
> I also wonder if you need to disable this field trial in browser_tests.

Wouldn't it make sense to leave it enabled to catch any errors? Or do you just
not want to see noise in measurements. If that's the case, I suppose I'd be
adding --enable-async-dns to browser tests command line.

[cbentzel, 2012-08-21 14:52:03]

On 2012/08/21 13:39:49, szym wrote:
> On 2012/08/21 10:50:53, cbentzel wrote:
> > I also wonder if you need to disable this field trial in browser_tests.
> 
> Wouldn't it make sense to leave it enabled to catch any errors? Or do you just
> not want to see noise in measurements. If that's the case, I suppose I'd be
> adding --enable-async-dns to browser tests command line.

Basically - I don't want to introduce hard-to-track-down flakiness. I'd be fine
if this was 100% one way or another.

[szym, 2012-08-21 19:29:48]

Ready for another round of review. PTAL.

[szym, 2012-08-21 20:09:40]

http://codereview.chromium.org/10828373/diff/2011/chrome/browser/net/async_dn...
File chrome/browser/net/async_dns_field_trial.cc (right):

http://codereview.chromium.org/10828373/diff/2011/chrome/browser/net/async_dn...
chrome/browser/net/async_dns_field_trial.cc:23: channel ==
chrome::VersionInfo::CHANNEL_CANARY) {
Should this be |channel <= CHANNEL_DEV| to allow ToT to participate in the
trial? Or is it pointless, since we don't have metrics in those builds anyway?

[szym, 2012-08-22 16:17:09]

ping?

[cbentzel, 2012-08-22 17:16:54]

http://codereview.chromium.org/10828373/diff/2011/chrome/browser/net/async_dn...
File chrome/browser/net/async_dns_field_trial.cc (right):

http://codereview.chromium.org/10828373/diff/2011/chrome/browser/net/async_dn...
chrome/browser/net/async_dns_field_trial.cc:14: #if defined(OS_ANDROID) ||
defined(OS_IOS)
CrOS as well?

http://codereview.chromium.org/10828373/diff/2011/chrome/browser/net/async_dn...
chrome/browser/net/async_dns_field_trial.cc:17: #else
I would just do a #endif here rather than an else.

http://codereview.chromium.org/10828373/diff/2011/chrome/browser/net/async_dn...
chrome/browser/net/async_dns_field_trial.cc:23: channel ==
chrome::VersionInfo::CHANNEL_CANARY) {
On 2012/08/21 20:09:40, szym wrote:
> Should this be |channel <= CHANNEL_DEV| to allow ToT to participate in the
> trial? Or is it pointless, since we don't have metrics in those builds anyway?

Fine with this.

http://codereview.chromium.org/10828373/diff/2011/chrome/browser/net/async_dn...
File chrome/browser/net/async_dns_field_trial.h (right):

http://codereview.chromium.org/10828373/diff/2011/chrome/browser/net/async_dn...
chrome/browser/net/async_dns_field_trial.h:13: namespace net {
Things in chrome/browser/net are not in net namespace.

[szym, 2012-08-22 17:39:01]

http://codereview.chromium.org/10828373/diff/2011/chrome/browser/net/async_dn...
File chrome/browser/net/async_dns_field_trial.h (right):

http://codereview.chromium.org/10828373/diff/2011/chrome/browser/net/async_dn...
chrome/browser/net/async_dns_field_trial.h:13: namespace net {
On 2012/08/22 17:16:55, cbentzel wrote:
> Things in chrome/browser/net are not in net namespace.

I noticed both namespace chrome_browser_net and chrome::browser::net, and of
course no namespace. What do you recommend?

[szym, 2012-08-22 17:56:36]

All done.

[cbentzel, 2012-08-22 18:19:01]

LGTM

[szym, 2012-08-22 18:26:07]

ben: please approve changes in chrome/

[szym, 2012-08-22 19:51:42]

sky: mind taking a quick look? Your OWNERS approval needed for chrome/

[sky, 2012-08-22 21:47:14]

LGTM

[szym, 2012-08-22 21:50:10]

Thanks!

On Wed, Aug 22, 2012 at 5:47 PM,  <sky@chromium.org> wrote:
> LGTM
>
> http://codereview.chromium.org/10828373/

[commit-bot: I haz the power, 2012-08-22 22:36:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/10828373/15022

[commit-bot: I haz the power, 2012-08-22 23:30:17]

Failed to apply patch for chrome/chrome_browser.gypi:
While running patch -p1 --forward --force;
patching file chrome/chrome_browser.gypi
Hunk #1 FAILED at 1502.
1 out of 1 hunk FAILED -- saving rejects to file chrome/chrome_browser.gypi.rej