487515 - Date.now() is only accurate to 1 second on Windows Mobile

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: panning badness

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I tracked with hg bisect when the issue has checked in. 

The changeset is http://hg.mozilla.org/mobile-browser/rev/850eab6866a5
I  don't know what is wrong in the changeset yet.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

The problem stemmed from the timestamps in the momentum buffer. Multiple events had the same timestamp. The math in .startKinetic resulted in Nan values. We use Date.now() which uses PR_Now which uses GetSystemTime. From the MSDN for WinCE:

"Millisecond granularity may not be supported by a hardware platform. The caller of this function should not rely on more than second granularity."

After digging a bit, it seems we need to change some code in:
http://mxr.mozilla.org/mozilla-central/source/js/src/prmjtime.cpp

so it allows Windows Mobile to use the high res timing code and not fallback to the low res code. Windows Mobile fails the HAVE_GETSYSTEMTIMEASFILETIME tests so it uses the low res code.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Err, Date.now() does _not_ use PR_Now. It uses PRMJ_Now, but the implementation we fallback on is the same.

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: wince uses simular code path to desktop

Mark, thanks for diagnosing this problem.

This patch creates a LOW_RES_TIME helper macro to be substituted for GetSystemTimeAsFileTime().  Now windows desktop and windows ce can use basically the same code path.

One caveat, windows ce doesn't have the spincounts for mutexes.  I stubbed out that functionality without looking at why it was needed too deeply.  It works as is in my limited testing, but if the spincounts are required I'll have to add that back in for windows ce.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

I tested a version of this patch on Windows Mobile and it does give millisecond resolution.

Comment 7

[Brian Crowder]

Comment on attachment 372619 [details] [diff] [review]
wince uses simular code path to desktop

>-#ifdef HAVE_GETSYSTEMTIMEASFILETIME
>+#if defined(HAVE_GETSYSTEMTIMEASFILETIME) || defined(HAVE_SYSTEMTIMETOFILETIME)
>+
>+#if defined(HAVE_GETSYSTEMTIMEASFILETIME)
>+#define LOW_RES_TIME(lpft) GetSystemTimeAsFileTime(lpft);
>+#elif defined(HAVE_SYSTEMTIMETOFILETIME)
>+#define LOW_RES_TIME(lpft) \
>+    do {  \

"\" always line up on col 79 (for both of the lines above), but you won't care if you follow the inlining suggestion below.


>+       SYSTEMTIME st;                                  \
>+       GetSystemTime(&st);                             \
>+       SystemTimeToFileTime(&st,lpft);                 \
>+    }while(0);

Need a space between "}" and "while", but again, you won't care if you inline.


>+#else
>+#error "No implementation of PRMJ_Now was selected."
>+#endif

We can reliably use inline functions instead of macros now, and that might indeed be better stylistically.

> 
> typedef struct CalibrationData
> {

Cuddle the "{" to the line above.


> #define MUTEX_LOCK(m) EnterCriticalSection(m)
> #define MUTEX_TRYLOCK(m) TryEnterCriticalSection(m)
> #define MUTEX_UNLOCK(m) LeaveCriticalSection(m)
>+#ifdef WINCE
>+#define MUTEX_SETSPINCOUNT(m, c)
>+#else
> #define MUTEX_SETSPINCOUNT(m, c) SetCriticalSectionSpinCount((m),(c))
>-
>+#endif

Put this vertical space back in, after the endif


>+#ifdef WINCE
>+            /* Assume the Windows CE clock ticks every 15.6ms */
>+            skewThreshold = 156000;
>+#else

Where does this assumption come from (other than the pre-existing comment below)?


>             /* Rather than assume the NT kernel ticks every 15.6ms, ask it */


Will r+ the next spin...

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.2, updated based on crowder's comments

Comment 9

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #7)
> Where does this assumption come from (other than the pre-existing comment
> below)?
> 
> 
> >             /* Rather than assume the NT kernel ticks every 15.6ms, ask it */

As discussed on IRC, my testing on an HTC Touch Pro shows 1s granularity for GetSystemTime (which in hind sight we already knew).  I adjusted the assumption, but I'm open to suggestions on how to detect it on start up or first run.

Comment 10

[Brian Crowder]

Comment on attachment 372663 [details] [diff] [review]
patch v.2, updated based on crowder's comments


>+#if defined(HAVE_GETSYSTEMTIMEASFILETIME)
>+inline void lowResTime(LPFILETIME lpft) { GetSystemTimeAsFileTime(lpft); }

Static/inline functions in js/src look like (sorry I didn't mention before):
inline void
LowResTime(lpft)   <--- note the capitalization, please
{ <--- not cuddled, unlike struct/class
...
}

Go ahead and break this out, I don't think it should be a one-liner.

>+#elif defined(HAVE_SYSTEMTIMETOFILETIME)
>+inline void lowResTime(LPFILETIME lpft) {

Again, type\nFunctionName()\n{, as above.


>+#ifdef WINCE
>+            /* Assume the Windows CE clock ticks every 1s */
>+            skewThreshold = 1000000;
>+#else

Still not sure this is right, either.  :(  Hopefully Rob will have better advice.

I'm very, very likely to r+ the next spin.  ;)

Comment 11

[Rob Arnold [:robarnold]]

Comment on attachment 372663 [details] [diff] [review]
patch v.2, updated based on crowder's comments

>@@ -198,9 +209,9 @@ NowCalibrate()
>         /* By wrapping a timeBegin/EndPeriod pair of calls around this loop,
>            the loop seems to take much less time (1 ms vs 15ms) on Vista. */
>         timeBeginPeriod(1);
>-        GetSystemTimeAsFileTime(&ftStart);
>+        lowResTime(&ftStart);
>         do {
>-            GetSystemTimeAsFileTime(&ft);
>+            lowResTime(&ft);
>         } while (memcmp(&ftStart,&ft, sizeof(ft)) == 0);
>         timeEndPeriod(1);

This will help you find a decent value for skew threshold. (ft-ftStart) should be the tick rate of the kernel of whatever device you are on assuming that timeBeginPeriod has no immediate effect. If it does have an immediate effect, then (ft-ftStart) will be less than the actual tick rate.

Depending on how often you see recalibration on your devices, you could disable the time*Period(1) calls - they are only an optimization.

We use the tick rate as the skew threshold (well, sorta. There's some math involved to smooth things out).

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.3, uses ftStart - ft

Comment 13

[Rob Arnold [:robarnold]]

Comment on attachment 372778 [details] [diff] [review]
patch v.3, uses ftStart - ft

>+#ifdef WINCE
>+        calibration.granularity = ft.dwLowDateTime - ftStart.dwLowDateTime;
>+#endif

This should be a 64bit subtraction with the division as seen below so that the granularity is in microseconds (15600 is 15.6 ms).

>         /*
>         calibrationDelta = (FILETIME2INT64(ft) - FILETIME2INT64(ftStart))/10;
>         fprintf(stderr, "Calibration delta was %I64d us\n", calibrationDelta);

Comment 14

[Brian Crowder]

Comment on attachment 372778 [details] [diff] [review]
patch v.3, uses ftStart - ft

>bug 487515 - PRMJ_Now needs better granularity for panning in fennec

I know this is very nit-picky, but you might not want to suggest in your checkin comment that this fix only benefits panning in Fennec.  :)


>+#elif defined(HAVE_SYSTEMTIMETOFILETIME)
>+inline void
>+LowResTime(LPFILETIME lpft)
> {
>+    SYSTEMTIME st;
>+    GetSystemTime(&st);
>+    SystemTimeToFileTime(&st,lpft);
>+}

No GetSystemFT()?


I'm happy with this patch, these notes and nits are just to make me feel better.  :)

Comment 15

[Brian Crowder]

What kind of numbers are you typically getting for calibration.granularity?

Comment 16

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #14)
> (From update of attachment 372778 [details] [diff] [review])
> >bug 487515 - PRMJ_Now needs better granularity for panning in fennec
> 
> I know this is very nit-picky, but you might not want to suggest in your
> checkin comment that this fix only benefits panning in Fennec.  :)

well...it benefits web content too I would assume. 

> 
> >+#elif defined(HAVE_SYSTEMTIMETOFILETIME)
> >+inline void
> >+LowResTime(LPFILETIME lpft)
> > {
> >+    SYSTEMTIME st;
> >+    GetSystemTime(&st);
> >+    SystemTimeToFileTime(&st,lpft);
> >+}
> 
> No GetSystemFT()?

ehh... no reason to change other than its prettier

> 
> I'm happy with this patch, these notes and nits are just to make me feel
> better.  :)


(In reply to comment #15)
> What kind of numbers are you typically getting for calibration.granularity?

The ftStart - ft is the same as my test program where I was getting 1s

Comment 17

[Brian Crowder]

> > No GetSystemFT()?
> 
> ehh... no reason to change other than its prettier
> 

Prettier is a good enough reason for me; this module is already ugly and hairy, let's not contribute more.  :)


> (In reply to comment #15)
> > What kind of numbers are you typically getting for calibration.granularity?
> 
> The ftStart - ft is the same as my test program where I was getting 1s

Too bad....

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch v.4, uses GetCurrentFT() and FILETIME2INT64

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

granularity is getting set to 1000000, which is 1s

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/89e3d635456f