convert DNS expiration tracking to TimeStamp/TimeDuration

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: jaas, Assigned: jaas)

Tracking

Trunk
mozilla21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Posted patch fix v1.0 (obsolete) — Splinter Review
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).
Attachment #697415 - Flags: review?(sworkman)
Are you aware of potential performance issues here on windows?  TimeStamp shouldn't be used unless a hi-precision ms resolution is needed.
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 :)
Attachment #697415 - Flags: review?(sworkman) → review+
Posted patch fix v1.1 (obsolete) — Splinter Review
Good idea Steve.
Attachment #697415 - Attachment is obsolete: true
(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?
(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.
Depends on: 827287
Posted patch fix v1.2Splinter Review
Use ::NowLoRes timer function.
Attachment #697857 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e524829ba4b7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.