[net/dns] Hardcode DnsConfig.timeout to 1 second.

[net/dns] Hardcode DnsConfig.timeout to 1 second.

Currently, it is read from OS (if available) or set to 5 seconds (on Windows). In some cases the OS provides ridiculous values (30s on Mac). 

This change hard-codes the timeout to 1 second, which is the default used by getaddrinfo on Windows and Mac.

BUG=124437
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151626

[cbentzel, 2012-08-09 20:29:07]

http://codereview.chromium.org/10826212/diff/1/net/dns/dns_config_service_pos...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/10826212/diff/1/net/dns/dns_config_service_pos...
net/dns/dns_config_service_posix.cc:62: config->timeout =
base::TimeDelta::FromSeconds(1);
min of timeout and 1 second?

Could 1s move to a constant in a header?

[szym, 2012-08-09 20:59:29]

http://codereview.chromium.org/10826212/diff/1/net/dns/dns_config_service_pos...
File net/dns/dns_config_service_posix.cc (right):

http://codereview.chromium.org/10826212/diff/1/net/dns/dns_config_service_pos...
net/dns/dns_config_service_posix.cc:62: config->timeout =
base::TimeDelta::FromSeconds(1);
On 2012/08/09 20:29:08, cbentzel wrote:
> min of timeout and 1 second?
> 
> Could 1s move to a constant in a header?

Done. Another option is to completely remove the code which sets
DnsConfig::timeout along with the tests and only set it in DnsConfig::DnsConfig.
I plan to do this eventually, but should I just do it now?

[cbentzel, 2012-08-09 21:21:13]

On 2012/08/09 20:59:29, szym wrote:
>
http://codereview.chromium.org/10826212/diff/1/net/dns/dns_config_service_pos...
> File net/dns/dns_config_service_posix.cc (right):
> 
>
http://codereview.chromium.org/10826212/diff/1/net/dns/dns_config_service_pos...
> net/dns/dns_config_service_posix.cc:62: config->timeout =
> base::TimeDelta::FromSeconds(1);
> On 2012/08/09 20:29:08, cbentzel wrote:
> > min of timeout and 1 second?
> > 
> > Could 1s move to a constant in a header?
> 
> Done. Another option is to completely remove the code which sets
> DnsConfig::timeout along with the tests and only set it in
DnsConfig::DnsConfig.
> I plan to do this eventually, but should I just do it now?

I think it would help to histogram what the results are from the system - do we
have one in place already?

[szym, 2012-08-09 21:24:30]

We currently have a histogram for the total time of DnsTransaction
(AsyncDNS.TransactionSuccess is directly comparable to getaddrinfo
DNS.ResolveSuccess). I think a histogram for the timing of individual
DnsUDPAttempt might be useful.

On Thu, Aug 9, 2012 at 5:21 PM,  <cbentzel@chromium.org> wrote:
> On 2012/08/09 20:59:29, szym wrote:
>
>
http://codereview.chromium.org/10826212/diff/1/net/dns/dns_config_service_pos...
>>
>> File net/dns/dns_config_service_posix.cc (right):
>
>
>
>
http://codereview.chromium.org/10826212/diff/1/net/dns/dns_config_service_pos...
>>
>> net/dns/dns_config_service_posix.cc:62: config->timeout =
>> base::TimeDelta::FromSeconds(1);
>> On 2012/08/09 20:29:08, cbentzel wrote:
>> > min of timeout and 1 second?
>> >
>> > Could 1s move to a constant in a header?
>
>
>> Done. Another option is to completely remove the code which sets
>> DnsConfig::timeout along with the tests and only set it in
>
> DnsConfig::DnsConfig.
>>
>> I plan to do this eventually, but should I just do it now?
>
>
> I think it would help to histogram what the results are from the system - do
> we
> have one in place already?
>
> http://codereview.chromium.org/10826212/

[mmenke, 2012-08-09 21:45:39]

On 2012/08/09 21:24:30, szym wrote:
> We currently have a histogram for the total time of DnsTransaction
> (AsyncDNS.TransactionSuccess is directly comparable to getaddrinfo
> DNS.ResolveSuccess). I think a histogram for the timing of individual
> DnsUDPAttempt might be useful.

Our results aren't directly comparable to the system's, unfortunately, due to
caching differences.

Unfortunately, it seems impossible to separate out caching from network
transaction time.  May just want to compare lifetime of DNS requests, though
that has its own issues.

[szym, 2012-08-09 21:48:37]

On 2012/08/09 21:45:39, Matt Menke wrote:
> On 2012/08/09 21:24:30, szym wrote:
> > We currently have a histogram for the total time of DnsTransaction
> > (AsyncDNS.TransactionSuccess is directly comparable to getaddrinfo
> > DNS.ResolveSuccess). I think a histogram for the timing of individual
> > DnsUDPAttempt might be useful.
> 
> Our results aren't directly comparable to the system's, unfortunately, due to
> caching differences.
> 
> Unfortunately, it seems impossible to separate out caching from network
> transaction time.  May just want to compare lifetime of DNS requests, though
> that has its own issues.

Agreed, which is the purpose of the '*DNS.TotalTime' histogram. We should
probably first increase HostCache size until TotalTime stops improving
noticeably.

[cbentzel, 2012-08-10 08:58:16]

I was more curious about a histogram of the specified retry time at the
time we read the system config, just to see if it is sane at all.

On Thu, Aug 9, 2012 at 5:48 PM, <szym@chromium.org> wrote:

> On 2012/08/09 21:45:39, Matt Menke wrote:
>
>> On 2012/08/09 21:24:30, szym wrote:
>> > We currently have a histogram for the total time of DnsTransaction
>> > (AsyncDNS.TransactionSuccess is directly comparable to getaddrinfo
>> > DNS.ResolveSuccess). I think a histogram for the timing of individual
>> > DnsUDPAttempt might be useful.
>>
>
>  Our results aren't directly comparable to the system's, unfortunately,
>> due to
>> caching differences.
>>
>
>  Unfortunately, it seems impossible to separate out caching from network
>> transaction time.  May just want to compare lifetime of DNS requests,
>> though
>> that has its own issues.
>>
>
> Agreed, which is the purpose of the '*DNS.TotalTime' histogram. We should
> probably first increase HostCache size until TotalTime stops improving
> noticeably.
>
>
http://codereview.chromium.**org/10826212/<http://codereview.chromium.org/108...
>

[szym, 2012-08-13 22:45:12]

On 2012/08/10 08:58:16, cbentzel wrote:
> I was more curious about a histogram of the specified retry time at the
> time we read the system config, just to see if it is sane at all.
> 

Would you prefer to have this as part of this CL or a separate one?

[cbentzel, 2012-08-14 22:16:35]

LGTM

You can do it in a separate CL

[commit-bot: I haz the power, 2012-08-14 22:24:21]

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

[commit-bot: I haz the power, 2012-08-15 01:26:46]

Change committed as 151626