826238 - convert DNS expiration tracking to TimeStamp/TimeDuration

Status: RESOLVED FIXED

(Core :: Networking: DNS, defect)

Description

[Josh Aas]

Attachment: fix v1.0

We should convert DNS expiration tracking to TimeStamp/TimeDuration. I think it makes things easier to understand, and it makes modifications I'm planning to make easier (use real TTLs).

Comment 1

[Josh Aas]

https://tbpl.mozilla.org/?tree=Try&rev=efc9a0101812

Comment 2

[Honza Bambas (:mayhemer)]

Are you aware of potential performance issues here on windows?  TimeStamp shouldn't be used unless a hi-precision ms resolution is needed.

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 697415 [details] [diff] [review]
fix v1.0

Review of attachment 697415 [details] [diff] [review]:
-----------------------------------------------------------------

Patch looks fine to me.

I looked at the implementations of PR_Now and TimeStamp:Now for windows, but I don't know enough to compare their performance :) So, I'd prefer to wait to hear more from Honza on that. But otherwise, r=me with the small changes.

::: netwerk/dns/nsHostResolver.cpp
@@ +853,4 @@
>              rec->negative = false;
>          }
>          else {
> +            rec->expiration += TimeDuration::FromSeconds(60); /* one minute for negative cache */

Can you declare mMaxCacheLifetime as a TimeDuration and do this calculation one time at startup/nsHostResolver creation? Similarly, declare a TimeDuration for 1 minute - kOneMinute or something like that.

@@ +878,5 @@
>  
>                  if (!head->negative) {
>                      // record the age of the entry upon eviction.
> +                    TimeDuration age = TimeStamp::Now() - (head->expiration - TimeDuration::FromSeconds(mMaxCacheLifetime * 60));
> +                    Telemetry::Accumulate(Telemetry::DNS_CLEANUP_AGE, static_cast<uint32_t>(age.ToSeconds() / 60));

This should look a little clearer if mMaxCacheLifetime is a TimeDuration object already (see other comment). And (nit) try to keep lines to 80 cols max :)

Comment 4

[Josh Aas]

Attachment: fix v1.1

Good idea Steve.

Comment 5

[Josh Aas]

(In reply to Honza Bambas (:mayhemer) from comment #2)
> Are you aware of potential performance issues here on windows?  TimeStamp
> shouldn't be used unless a hi-precision ms resolution is needed.

That seems odd to me, and there is no mention of performance implications in the headers (that I read, anyway). Can you expand on the performance implications? Are they really on a magnitude that would impact this code?

Comment 6

[Honza Bambas (:mayhemer)]

(In reply to Josh Aas (Mozilla Corporation) from comment #5)
> (In reply to Honza Bambas (:mayhemer) from comment #2)
> > Are you aware of potential performance issues here on windows?  TimeStamp
> > shouldn't be used unless a hi-precision ms resolution is needed.
> 
> That seems odd to me, and there is no mention of performance implications in
> the headers (that I read, anyway). Can you expand on the performance
> implications? Are they really on a magnitude that would impact this code?

QueryPerfomanceCounter on win XP and maybe on some platforms also running newer versions of windows is magnitudes (100x) slower then timeGetTime() or GetTickCount().

If you don't need hi precision, don't use TimeStamp.  Period.

Comment 7

[Josh Aas]

Attachment: fix v1.2

Use ::NowLoRes timer function.

Comment 8

[Josh Aas]

pushed to mozilla-inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e524829ba4b7

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e524829ba4b7