493756 - PR_Now() and Date.now() get out of sync

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[Doug Turner (:dougt)]

I was doing some simply profiling on windows mobile using printf's and dump's which used PR_Now() and Date.now().  What I saw was that dumps of Date.now() printed out values that were about 500ms ahead of the printf's, but the log file showed that the dump's were printed to the log before the printfs.

sample console log:

(1242755056070) WM_LBUTTONDOWN start  <-- from a printf call using PR_Now()
(1242755055580) js mousedown start    <-- from a dump using Date.now()
(1242755055594) js mousedown end      <-- from a dump using Date.now()

To work around this problem, I made PRMJ_Now just return the value of PR_Now().

This might be an acceptable solution on Windows Mobile now since we do a similar calibration as JS does.  Maybe we can use PR_Now on all platforms?

Comment 1

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

PR_Now doesn't have fine enough resolution for js on all platforms.  Using PR_Now for js would be good for Windows CE though, since we're paying the initialization penalty twice as it stands.

Comment 2

[Nochum Sossonko [:Natch]]

A similar bug has been investigated about Date's timing issues, bug 472233. I
don't think NSPR's functions are accurate enough for js, I mentioned using them
but was told they are within ~15 ms or some such...

Comment 3

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

(In reply to comment #2)
> A similar bug has been investigated about Date's timing issues, bug 472233. I
> don't think NSPR's functions are accurate enough for js, I mentioned using them
> but was told they are within ~15 ms or some such...

We're using essentially the same code in js and NSPR for windows ce since the landing of bug 488621.  So, for windows ce NSPR's resolution is fine.

Comment 4

[Brendan Eich [:brendan]]

Don't think so... cc'ing Rob Arnold.

/be

Comment 5

[Rob Arnold [:robarnold]]

This hasn't been a problem in the past as far as I know even though PRMJ_Now and PR_Now used different timing methods. Some clock drift is expected but 500ms is very unusual.

I do plan to change the implementation of the desktop version of PRMJ_Now to use the multimedia timers when the system is on AC. This should give more stable results, especially if we can get NSPR to accept the changes. Though the multimedia timer API gives 15ms resolution by default, we can turn up the kernel's tick rate while on AC (see bug 454660) to give 1ms resolution. This should result in no change to NSPR's current PR_Now impl for other clients while allowing us to benefit from a better resolution. After that is completed and checked in, we can switch PRMJ_Now to call PR_Now which should make both desktop and mobile happy since their PR_Now impl will still differ (mm timers are not stable on WinMo from what I hear? Perhaps someone more familiar with WinMo can confirm - this is not as well documented and I don't have any WinMo devices). And of course as a result PR_Now will agree with PRMJ_Now.

I'm not sure at this point why there are separate implementations; is it because SpiderMonkey has different performance requirements than NSPR's clients or is it because SpiderMonkey does not want to depend on NSPR (or both)?

Comment 6

[Jeff Walden [:Waldo]]

SpiderMonkey has different perf requirements, or at the least better accuracy/perf is more important to SpiderMonkey than to NSPR clients in general.

SpiderMonkey depends on NSPR only when built in the threadsafe configuration, which is simpler for ST embedders.  Simple enough to care about if sharing an implementation were deemed desirable?  Dunno, we'd have to ask them.

Comment 7

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

Since PR_Now and  PRMJ_Now have the same basic implementations, can we just make PRMJ_Now call PR_Now now?

Comment 8

[Brian Crowder]

I still think introducing this dependency on NSPR for non-threadsafe builds of Spidermonkey isn't a good idea.

Comment 9

[Stuart Parmenter]

It would really be nice for these to be the same.

Comment 10

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

(In reply to comment #8)
> I still think introducing this dependency on NSPR for non-threadsafe builds of
> Spidermonkey isn't a good idea.

can we have the dependency conditional on it being a threadsafe build? otherwise fall back to the existing code.

Comment 11

[Ed Morley [:emorley]]

WinCE/Windows Mobile support has been removed from the main build system, Spidermonkey, mobile installer, in-app updater and so on (see bug 614720, bug 554087 and all their dependants). Until such point where MS decide to release a Windows Phone 7 NDK and the decision is made to port to that platform, this is WONTFIX.

Filter bugmail on WinCEMassWONTFIX.