539093 - Implement high-resolution platform timers for OS X

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

With bug 522126, we now have infrastructure for platform-specific  TimeStamp/TimeDuration.  Currently on POSIX CLOCK_MONOTONIC is implemented, but it would be nice to have high-resolution timers for all Tier Is.

Comment 1

[Honza Bambas (:mayhemer)]

Attachment: TimeStamp updated to QueryPerformanceCounter, windows only

This changes TimeStamp to use QueryPerformanceCounter.  Not deeply tested, but it gives me 1ms resolution of Now() on Windows 7/XP.

Comment 2

[Honza Bambas (:mayhemer)]

Comment on attachment 529518 [details] [diff] [review]
TimeStamp updated to QueryPerformanceCounter, windows only

Christian, can you take a look if this is the right way to go?

For me the patch works well, and is compilable on other platforms.  I can push it to try as well.

Comment 3

[Rob Arnold [:robarnold]]

(In reply to comment #2)
> Comment on attachment 529518 [details] [diff] [review] [review]
> TimeStamp updated to QueryPerformanceCounter, windows only
> 
> Christian, can you take a look if this is the right way to go?
> 
> For me the patch works well, and is compilable on other platforms.  I can
> push it to try as well.

Some platforms have buggy QPC implementations. Correctness may not be guaranteed across suspend/hibernate as well, even on Vista. Have you looked at the implementation of PRMJ_Now or Chromium's TimeStamp implementation?

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to comment #3)
> Some platforms have buggy QPC implementations. Correctness may not be
> guaranteed across suspend/hibernate as well, even on Vista. Have you looked
> at the implementation of PRMJ_Now or Chromium's TimeStamp implementation?

For me it seems simplest to take our JS implementation and put it to TimeStamp.

Best would be to do this for all platforms where TimeStamp has low resolution.

Anybody against this plan?

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #4)
> (In reply to comment #3)
> For me it seems simplest to take our JS implementation and put it to
> TimeStamp.
>
> Anybody against this plan?

Possibly ... PRMJ_Now is a real-time clock and TimeStamp is a monotonic clock, so it's not at all straightforward to implement one with the other.  Also, our POSIX CLOCK_MONOTONIC TimeStamp implementation can report nanosecond-resolution times but PRMJ_Now is limited to microsecond.

We should definitely share the timer code across gecko/SM but great care needs to be taken :).

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 529518 [details] [diff] [review]
TimeStamp updated to QueryPerformanceCounter, windows only

Shouldn't this live in a TimeStamp_windows.cpp file, similar to TimeStamp_posix.cpp?

That said, I'm not all that familiar with the timestamp APIs on Windows, so changing the feedback request to rob

Comment 7

[Rob Arnold [:robarnold]]

Comment on attachment 529518 [details] [diff] [review]
TimeStamp updated to QueryPerformanceCounter, windows only

This approach doesn't address the monotonicity requirement. PRMJ_Now doesn't either. I think what you want is something that uses the same core as PRMJ_Now (the self-correcting/calibration) but w/o the truncation to microseconds and conversion to an actual date. For the clock-skew detection, you could use the multimedia timer since that is monotonic (I think). PRMJ_Now also should work with using the multimedia timer as its guide so you might want to rewrite it to use the MM timer (perhaps even sharing the same calibration data as TimeStamp::Now so that they're more consistent).

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Attachment: TimeStamp implementation for OS X using mach_absolute_time

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #8)
> Created attachment 546876 [details] [diff] [review] [review]
> TimeStamp implementation for OS X using mach_absolute_time

Currently this does the build keyed off of COCOA widgets. There's probably something better but I couldn't think of it.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 546876 [details] [diff] [review]
TimeStamp implementation for OS X using mach_absolute_time

>+ifeq ($(MOZ_WIDGET_TOOLKIT),cocoa)

ifeq ($(OS_ARCH),Darwin)

>+CPPSRCS += TimeStamp_mac.cpp

Call this TimeStamp_darwin.

>+ * Contributor(s):
>+ *  Chris Jones <[e-mail]>

No need to cite me, I didn't do anything.

>+//
>+// Implement TimeStamp::Now() with mach_absolute_time
>+//
>+// The "tick" unit for mach_absolute_time is defined using mach_timebase_info() which
>+// gives a conversion ratio to nanoseconds. For more information see Apple's QA1398.

The documentation for mach_absolute_time() is embarrassingly
atrociously horrible.  Is there any guarantee the timebase doesn't
change with frequency scaling?  If it can then your impl won't work
(and I think it would be just about impossible to get one right from
userspace).  Given that, I would assume timebase doesn't change with
frequency scaling, but boy would it be nice to get some official
assurance.

>+// This code is inspired by Chromium's time_mac.cc. The biggest
>+// differences are that we explicitly initialize using
>+// TimeStamp::Initialize() instead of lazily in Now() and that
>+// we store the time value in ticks and convert when needed instead
>+// of storing the time value in nanoseconds.

What values for numer/denom do you see in the timebase on your dev
machine?  If denom is high and numer is small, we might need to worry
about tick overflow.

>+  if (0 == minres) {
>+    // measurable resolution is either incredibly low, ~1ns, or very
>+    // high.  fall back on NSPR's resolution
>+    // assumption

Reflow plz.

>+  kern_return_t kr = mach_timebase_info(&sTimebaseInfo);
>+  if (kr != KERN_SUCCESS)
>+    NS_RUNTIMEABORT("CLOCK_MONOTONIC is absent!");

Fix msg plz.

Can't r+ without answers to questions above.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #10)
> >+ * Contributor(s):
> >+ *  Chris Jones <[e-mail]>
> 
> No need to cite me, I didn't do anything.

The resolution finding and digits significance code is from TimeStamp_posix. But I can drop your name if you prefer.

> The documentation for mach_absolute_time() is embarrassingly
> atrociously horrible.  Is there any guarantee the timebase doesn't
> change with frequency scaling?  If it can then your impl won't work
> (and I think it would be just about impossible to get one right from
> userspace).  Given that, I would assume timebase doesn't change with
> frequency scaling, but boy would it be nice to get some official
> assurance.

No, there are no guarantees. However, http://developer.apple.com/library/mac/#qa/qa1398/_index.html suggests a pattern that would break if this were not true.


> >+// This code is inspired by Chromium's time_mac.cc. The biggest
> >+// differences are that we explicitly initialize using
> >+// TimeStamp::Initialize() instead of lazily in Now() and that
> >+// we store the time value in ticks and convert when needed instead
> >+// of storing the time value in nanoseconds.
> 
> What values for numer/denom do you see in the timebase on your dev
> machine?  If denom is high and numer is small, we might need to worry
> about tick overflow.

numer and denom are always 1 as far as I can tell:

http://google.com/codesearch#pFm0LxzAWvs/darwinsource/tarballs/apsl/xnu-792.tar.gz%7CO_WJd_UyLFk/xnu-792/osfmk/i386/rtclock.c&type=cs&l=1291

Further, the arithmetic using numer and denom are done with doubles so overflow won't occur.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #11)
> (In reply to comment #10)
> > >+ * Contributor(s):
> > >+ *  Chris Jones <[e-mail]>
> > 
> > No need to cite me, I didn't do anything.
> 
> The resolution finding and digits significance code is from TimeStamp_posix.
> But I can drop your name if you prefer.

Sure.

> 
> > The documentation for mach_absolute_time() is embarrassingly
> > atrociously horrible.  Is there any guarantee the timebase doesn't
> > change with frequency scaling?  If it can then your impl won't work
> > (and I think it would be just about impossible to get one right from
> > userspace).  Given that, I would assume timebase doesn't change with
> > frequency scaling, but boy would it be nice to get some official
> > assurance.
> 
> No, there are no guarantees. However,
> http://developer.apple.com/library/mac/#qa/qa1398/_index.html suggests a
> pattern that would break if this were not true.

How much do you trust the people who write those articles?

> > >+// This code is inspired by Chromium's time_mac.cc. The biggest
> > >+// differences are that we explicitly initialize using
> > >+// TimeStamp::Initialize() instead of lazily in Now() and that
> > >+// we store the time value in ticks and convert when needed instead
> > >+// of storing the time value in nanoseconds.
> > 
> > What values for numer/denom do you see in the timebase on your dev
> > machine?  If denom is high and numer is small, we might need to worry
> > about tick overflow.
> 
> numer and denom are always 1 as far as I can tell:
> 
> http://google.com/codesearch#pFm0LxzAWvs/darwinsource/tarballs/apsl/xnu-792.
> tar.gz%7CO_WJd_UyLFk/xnu-792/osfmk/i386/rtclock.c&type=cs&l=1291
> 
> Further, the arithmetic using numer and denom are done with doubles so
> overflow won't occur.

I meant overflow of the tick counter, which would mean we'd need an impl like the NSPR-backed TimeStamp.cpp.  But looks like this ought not to be a problem.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #12)
> > No, there are no guarantees. However,
> > http://developer.apple.com/library/mac/#qa/qa1398/_index.html suggests a
> > pattern that would break if this were not true.
> 
> How much do you trust the people who write those articles?

A fair bit, because it suggests a pattern that's used in a bunch of software which would also break if they broke these assumptions.

> I meant overflow of the tick counter, which would mean we'd need an impl
> like the NSPR-backed TimeStamp.cpp.  But looks like this ought not to be a
> problem.

It currently looks mach_absolute_time returns the number of nanoseconds since boot, which wont overflow for 500+ years of uptime.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #13)
> (In reply to comment #12)
> > > No, there are no guarantees. However,
> > > http://developer.apple.com/library/mac/#qa/qa1398/_index.html suggests a
> > > pattern that would break if this were not true.
> > 
> > How much do you trust the people who write those articles?
> 
> A fair bit, because it suggests a pattern that's used in a bunch of software
> which would also break if they broke these assumptions.

Alright.  If that's the best we have, it's the best we have.

> 
> > I meant overflow of the tick counter, which would mean we'd need an impl
> > like the NSPR-backed TimeStamp.cpp.  But looks like this ought not to be a
> > problem.
> 
> It currently looks mach_absolute_time returns the number of nanoseconds
> since boot, which wont overflow for 500+ years of uptime.

Yep.

Would like to see version with nits fixed, r+ on important stuff.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Attachment: TimeStamp implementation for OS X using mach_absolute_time v2

This fixes the nits, adds comments about the questions you asked and uses a static double sNsPerTick computed at startup for conversion instead of using the timebase_info directly.

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 546977 [details] [diff] [review]
TimeStamp implementation for OS X using mach_absolute_time v2

>+static double sNsPerTicks;

You'll want to call this |sNsPerTick|.

r=me if you get it compiling ;).

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 546977 [details] [diff] [review]
TimeStamp implementation for OS X using mach_absolute_time v2

>--- /dev/null
>+++ b/xpcom/ds/TimeStamp_darwin.cpp
>+ * The Initial Developer of the Original Code is the Mozilla Corporation.

It's the Mozilla Foundation, and that should go on a new line.

Comment 18

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/4053fa47daac