827287 - make it possible to use TimeStamp without performance concerns (TimeStamp::LowResNow)

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Josh Aas]

The TimeStamp class apparently has performance issues on Windows -- getting accurate timing does not perform well. Due to this I was told not to use TimeStamp unless I need accurate timing.

I don't need accurate timing, but I want to use TimeStamp for convenience. We should add something like TimeStamp::LowResNow to make that possible.

Comment 1

[Jason Duell]

And we should at least document the issue in the meantime, too.

Comment 2

[Josh Aas]

Honza - we should fix this separately from your Windows timer perf work. This has little to do with that, unless you're going to make this bug unnecessary. Keeping these separate means that if your work gets backed out or delayed this can still get in and stay in quickly.

I am happy to do this myself, but also happy to let you do it - which would you prefer?

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Josh Aas (Mozilla Corporation) from comment #2)
> Honza - we should fix this separately from your Windows timer perf work.
> This has little to do with that, unless you're going to make this bug
> unnecessary. Keeping these separate means that if your work gets backed out
> or delayed this can still get in and stay in quickly.
> 
> I am happy to do this myself, but also happy to let you do it - which would
> you prefer?

If you have time to do it now, then please proceed.  True is that we may not need to rewrite the code with one exception.  When you let the LowRes version use non-overlapping GetTickCount you then must take care to not mix LowRes and HiRes results when subtracting.

My preliminary plan for the complete rewrite was to change mValue of TimeStamp to encapsulate both QPC (when not found faulty and called from HiRes) and GTC (always) when on Windows.  At [1] the result of overloaded '-' operator for my new class would then return the final result transparently to the current code.  That way there will always be at least GTC to compare to safely when hires and lowres timestamps would be cross-mixed by mistake.

But when you only introduce two methods where one returns the calibrated QPC and one GTC you must take great care to not mix them. That was the reason I added the dependency.

[1] http://hg.mozilla.org/mozilla-central/annotate/a3effbaa10bb/xpcom/ds/TimeStamp.h#l214

Comment 4

[Josh Aas]

Sounds good. Assigning this to you.

Comment 5

[Honza Bambas (:mayhemer)]

Attachment: v1

- introduced TimeStamp::NowLoRes() with low performance impact priority and potentially lower resolution
- has affect only on the Windows platform
- not used on any place in the current code right now ; should be done in followup bugs by module owners probably

You may not like the new function name, though.

Comment 6

[(no longer active)]

Comment on attachment 706592 [details] [diff] [review]
v1

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

I'm not a super-reviewer.  Forwarding to roc for sr.

Comment 7

[Jason Duell]

> I'm not a super-reviewer

That sounds like a bug too :)

Comment 8

[(no longer active)]

(In reply to comment #7)
> > I'm not a super-reviewer
> 
> That sounds like a bug too :)

Lol!  I think that list is mostly static.

Comment 9

[Jason Duell]

> I think that list is mostly static.

That's the meta bug :)

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 706592 [details] [diff] [review]
v1

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

::: xpcom/ds/TimeStamp.h
@@ +214,5 @@
> +   *
> +   * Since on some platforms high resolution timer functions may have bad
> +   * impact on performance, consumers are encouraged to use NowLoRes where
> +   * larger intervals are measured and tens on milliseconds resolution is
> +   * enough.

Can you be more precise here? Will you guarantee 10ms resolution?

Comment 11

[Honza Bambas (:mayhemer)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #10)
> Comment on attachment 706592 [details] [diff] [review]
> v1
> 
> Review of attachment 706592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: xpcom/ds/TimeStamp.h
> @@ +214,5 @@
> > +   *
> > +   * Since on some platforms high resolution timer functions may have bad
> > +   * impact on performance, consumers are encouraged to use NowLoRes where
> > +   * larger intervals are measured and tens on milliseconds resolution is
> > +   * enough.
> 
> Can you be more precise here? Will you guarantee 10ms resolution?

Then the comment may be more specific, like:

"Now() is trying to ensure the best possible precision on each platform, at least one millisecond.

NowLoRes() has been introduced to workaround performance problems of QueryPerformanceCounter on the Windows platform.  NowLoRes() is giving lower precision, usually 15.6 ms, with very good performance benefit.  Use it for measurements of longer times, like >200ms timeouts."

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 706592 [details] [diff] [review]
v1

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

That sounds good. Thanks, please use that comment.

Comment 13

[Honza Bambas (:mayhemer)]

Attachment: v1.1

- comment updated

Comment 14

[Josh Aas]

Is this ready to land?

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Josh Aas (Mozilla Corporation) from comment #14)
> Is this ready to land?

I need to update heuristic in the new timestamp code.  It's too sensitive.  Retest, rereview and that we can land this all.

Comment 16

[Honza Bambas (:mayhemer)]

Comment on attachment 708722 [details] [diff] [review]
v1.1

https://hg.mozilla.org/integration/mozilla-inbound/rev/55ef31ba2298

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/55ef31ba2298

Comment 18

[Patrick McManus [:mcmanus]]

this is awesome.. maybe a note to dev-platform or a blog post on planet moz?

Comment 19

[Honza Bambas (:mayhemer)]

(In reply to Patrick McManus [:mcmanus] from comment #18)
> this is awesome.. maybe a note to dev-platform or a blog post on planet moz?

Worth both probably.  I'll post to dev-platform definitely.