Closed Bug 488621 Opened 11 years ago Closed 11 years ago

PR_Now() resolution on Windows Mobile is 1 second.

Categories

(NSPR :: NSPR, defect, P2)

ARM
Windows Mobile 6 Professional
defect

Tracking

(fennec1.0a1-wm+)

RESOLVED FIXED
Tracking Status
fennec 1.0a1-wm+ ---

People

(Reporter: hiro, Assigned: blassey)

Details

(Keywords: mobile)

Attachments

(1 file, 8 obsolete files)

PR_Now() resolution on Windows Mobile is 1 second.

Is it possible to incorporate PRMJ_Now efforts in NSPR?

see Bug 487515
Attached patch First try (obsolete) — Splinter Review
Almost copy and paste from js/src/prmjtime.cpp.
The patch can be built on both WinMo and WinXP and seems to to work well at least in timeline log.
Attached patch removes start up counter (obsolete) — Splinter Review
Assignee: wtc → bugmail
Attachment #373600 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #374372 - Flags: review?
Attachment #374372 - Flags: review? → review?(wtc)
Rob, this is basically a cut and paste from your implementation of PRMJ_Now(). When this lands we should probably have PRMJ_Now call PR_Now to avoid the duplication
Comment on attachment 374372 [details] [diff] [review]
removes start up counter

Whoa!
Is all this thought to be needed on all windows platforms? 
Or only on WinCE?

If it is believed to be needed only on WinCE, then it should all 
be in a big #ifdef WINCE.

If it is believed to be needed on all Windows platforms, I'd like
to see some analysis backing that belief.
Comment on attachment 374372 [details] [diff] [review]
removes start up counter

The description of this patch says "removes start up counter",
but it doesn't remove any counter from NSPR at all, and in fact
it adds over 300 lines of new code.  It needs more explanation 
that that description.
This is needed on NT sadly. It's not clear if all of it is needed for CE since that platform is not used as much. See bug 363258 for more details. Our solution for NT will hopefully be changing to something simpler in the next few months, but for CE, this will likely remain the best solution.
Just to clarify, this would have landed in NSPR a while ago if it weren't for the fact that NSPR supported Win9x up until recently (Win9x doesn't have this problem and doesn't support this solution).
Bug 487515 has some CE details
What is the expected effect of this patch on the total execution time spent
in each call to PR_Now on Windows desktop?  

A decrease?
An increase of 10%?  100% ? 1000% ?  
Has it been measured?
(In reply to comment #6)
> (From update of attachment 374372 [details] [diff] [review])
> The description of this patch says "removes start up counter",
> but it doesn't remove any counter from NSPR at all, and in fact
> it adds over 300 lines of new code.  It needs more explanation 
> that that description.

That's a description of the delta between this patch and the one that Wan-Teh had a look at earlier this afternoon. Admittedly, that patch wasn't on this bug, so I apologize for any confusion.
It has not been measured because the amount of time it takes will depend on the
hardware it is running on. It could be fast (one shared memory read and a
register read) or slow (several system calls, one BIOS call, two ACPI calls, total is ~timer resolution).
The effect of increasing the resolution of the date from 15ms (let alone 1s) to
1ms (on par with other platforms) was worth the cost. There are some ways it
could be made faster, but not much. There is no guarantee that QPC is stable on
WinCE (it is not on 2k/XP) which is why the code is so complicated. The
existing APIs on Windows for getting the time are not sufficient. Please read
the discussion in bug 363258 or the "Resolution" section of
http://www.robarnold.org/measuring-performance
Rob, You write:

"It has not been measured" ... but ... "was worth the cost."

Maybe that's true for the browser, but NSPR is used in many other products,
such as servers, where performance is seen as crucial (and 15ms actual 
resolution is considered acceptable).  The servers won't see it until it's
been released.   I very much don't want to hear a big loud complaint from 
the servers that use NSPR on Windows that this patch gave them a noticeable 
hit on performance.  So, I'd like to know in advance what that hit is going 
to be.
Perhaps it goes without saying, but but 1s resolution is not acceptable for windows ce.  If the performance hit turns out to be unacceptable for windows desktop, we'll need to take this (or some other fix) ifdef'd for windows ce.
Attached patch fixes ws to match file style (obsolete) — Splinter Review
Attachment #374372 - Attachment is obsolete: true
Attachment #374392 - Flags: review?(wtc)
Attachment #374372 - Flags: review?(wtc)
(In reply to comment #13)
> Rob, You write:
> 
> "It has not been measured" ... but ... "was worth the cost."
> 
> Maybe that's true for the browser, but NSPR is used in many other products,
> such as servers, where performance is seen as crucial (and 15ms actual 
> resolution is considered acceptable).  The servers won't see it until it's
> been released.   I very much don't want to hear a big loud complaint from 
> the servers that use NSPR on Windows that this patch gave them a noticeable 
> hit on performance.  So, I'd like to know in advance what that hit is going 
> to be.

Well perhaps this can be WINCE only. My plan for PR_Now was to use timeGetTime, which has a 15ms resolution by default, and use the service from bug 454660 to toggle the high resolution on and off. This solves the server, mobile and desktop cases.
I totally agree with your comment 14, Brad.  
WinCE needs a fix, even if there is a performance hit.
Comment on attachment 374392 [details] [diff] [review]
fixes ws to match file style

As one of NSPR's module owners, I'm offering this unsolicited set 
of review comments. 

1. Please make all this code conform to NSPR's coding style, rather
than Firefox's coding style.  We try pretty hard to keep all of NSPR's
source in one consistent coding style.  That means:
a) indentation is 4 spaces, 
b) tabs every 8 (despite a comment to the contrary)
c) lines longer than 80 columns are wrapped
d) block comments have a column of asterisks down the front (side),
   with closing comment delimiter usually on a line by itself

2. The constant initialized below is also found elsewhere in NSPR.

>+static const PRInt64 win2un = 0x19DB1DED53E8000;

The definitions should be consistent.  Please see
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/src/md/windows/ntio.c&rev=3.46&mark=103-112#103

3. for this definition (which is too long):

>+#define FILETIME2INT64(ft) (((PRInt64)ft.dwHighDateTime) << 32LL | (PRInt64)ft.dwLowDateTime)

the following would be preferred.  (the 32 needn't be LL)

>+#define FILETIME2INT64(ft) \
>+(((PRInt64)ft.dwHighDateTime) << 32 | (PRInt64)ft.dwLowDateTime)
Attachment #374413 - Attachment description: Beautified patch, also with missing #includes → Beautified patch
Comment on attachment 374414 [details] [diff] [review]
Beautified patch, also with missing #includes

This patch makes NSPR rely on functions that did not exist before Win2K.  
To get this patch to build, I had to modify the makefile as follows:

>-DEFINES += -D_NSPR_BUILD_
>+DEFINES += -D_NSPR_BUILD_ -DWINVER=0x0500 -D_WIN32_WINNT=0x0500

to tell the compiler to include declarations from Windows header
files for functions that were first implemented in Win2K.  

That is obviously a mere temporary hack to get it to build.  To take 
this patch, we will need to do this the right way in the Makefiles.

I need to check with the servers that use NSPR to determine if it is 
acceptable to require functions that were not present before Win2K.  
I'm pretty sure it will be, but I must double check.
Attachment #374414 - Flags: review?(wtc)
Attachment #374392 - Attachment is obsolete: true
Attachment #374392 - Flags: review?(wtc)
(In reply to comment #21)
> I need to check with the servers that use NSPR to determine if it is 
> acceptable to require functions that were not present before Win2K.  
> I'm pretty sure it will be, but I must double check.

NSPR 4.8 is dropping support for Win9x. (I assume that no one is running a server on 9x). I think any support for NT 3.x/4.0 is going away then too. From the announcement: "NSPR 4.8 will only support Windows 2000 or higher."
Rob, So, the Makefiles of NSPR must be changed to define the symbols that
I outlined in comment 21.  That is a prerequisite to the patch above being
accepted.
Nelson, right I didn't talk about that (though I am surprised it has not already happened on NSPR's trunk since the announcement was a few months ago). I was questioning the need to check the servers that use NSPR since it was made very clear that such functions were allowed to be required.
We should get this in ASAP.  This really hoses a lot of things.
tracking-fennec: --- → 1.0a1-wm+
Servers confirm, Win2K is the floor version supported.
Attachment #374502 - Flags: superreview?(wtc)
Attachment #374502 - Flags: review?(ted.mielczarek)
Attachment #374414 - Flags: review?(ted.mielczarek)
Those defines feel like they belong in configure. Any reason you went with putting them in the Makefile, or was it just the fastest way to make it work?
I guess the question in comment 27 is intended for me.

I think this property, the minimum version of Windows which must be 
specified in order to build NSPR, is a property of the source code, not
of the system on which it is being run.  So I don't see this as something
we'd expect would be a "tunable" parameter that the system builder can 
choose.  

As to why I put it into the Makefile, it was monkey see, monkey do.
The Makefile already defined some DEFINES, so I just extended it.
It could go somewhere else if a good case for doing so can be made.
Comment on attachment 374502 [details] [diff] [review]
Allow use of functions first declared in Win2k in NSPR 4.8 (obsoleted by patch v4)

Ok, that's fine. In Mozilla's configure this is a configurable parameter, and we've changed some things in bug 472093 so that you can in fact target a lower version and have it work properly. Not really necessary here.
Attachment #374502 - Flags: review?(ted.mielczarek) → review+
I found a few functions whose definitions lacked the explicit void argument.
Attachment #374414 - Attachment is obsolete: true
Attachment #374963 - Flags: superreview?(wtc)
Attachment #374963 - Flags: review?(ted.mielczarek)
Attachment #374414 - Flags: review?(wtc)
Attachment #374414 - Flags: review?(ted.mielczarek)
Comment on attachment 374963 [details] [diff] [review]
Beautified patch v2, with missing includes and void declarations

Nelson, thank you for reformatting Brad's patch.

Brad, thank you for the patch.  Could you ask the original
author of this code to review this patch?  I don't have time
to review it carefully.

Why do we need this code for desktop Windows?  Isn't it just
WINCE that has poor resolution for PR_Now()?

>+#include "math.h"     /* for fabs() */
>+#include "WinBase.h"

Please use <> for system headers -- <math.h> and <winbase.h>.
In NSPR we spell Windows system headers in all lowercase.

Please include <windows.h> even though the functions you're
using are declared in <winbase.h>.  See this comment at the
bottom of the MSDN page:
http://msdn.microsoft.com/en-us/library/ms724397(VS.85).aspx

  Header:  Winbase.h (include Windows.h)

Nit: In NSPR it's best to format a multi-line comment like this

  /*
   * line 1
   * line 2
   */

rather than this

  /* line 1
   * line 2
   */

It's OK if you don't fix this.

>+#ifdef __GNUC__
>+static const PRTime _pr_filetime_offset = 116444736000000000LL;
>+#else
>+static const PRTime _pr_filetime_offset = 116444736000000000i64;
>+#endif

Nit: this code is also in ntio.c/w95io.c.  Ideally we should just
have a copy of this code.  ntmisc.c (this file) is a better place
for this code because ntmisc.c is used by both the WINNT and WIN95
build configurations, whereas ntio.c is only used by the WINNT
configuration and w95io.c only by the WIN95 configuration.

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

Delete this commented-out code.

>+#define CALIBRATIONLOCK_SPINCOUNT 0
>+#define DATALOCK_SPINCOUNT 4096
>+#define LASTLOCK_SPINCOUNT 4096
>+
>+static PRStatus
>+NowInit(void)
>+{
>+    memset(&calibration, 0, sizeof(calibration));
>+    NowCalibrate();
>+#ifdef WINCE
>+    InitializeCriticalSection(&calibration.calibration_lock);
>+    InitializeCriticalSection(&calibration.data_lock);
>+#else
>+    InitializeCriticalSectionAndSpinCount(&calibration.calibration_lock, 
>+                                          CALIBRATIONLOCK_SPINCOUNT);
>+    InitializeCriticalSectionAndSpinCount(&calibration.data_lock, 
>+                                          DATALOCK_SPINCOUNT);
>+#endif
>+    return PR_SUCCESS;
>+}

Unless you really know what you're doing, it's best to just
use the default spin count (i.e., just call InitializeCriticalSection).

>+PR_NowShutdown(void)

This should be renamed _MD_CleanupTime.  It needs to
be called by _PR_CleanupTime (right now it's not being
called):
http://mxr.mozilla.org/nspr/ident?i=_PR_CleanupTime

NowInit should be renamed _MD_InitTime and be called by
_PR_InitTime rather than using PR_CallOnce (unless
NowCalibrate() takes a long time):
http://mxr.mozilla.org/nspr/ident?i=_PR_InitTime

>+#define MUTEX_LOCK(m) EnterCriticalSection(m)
>+#define MUTEX_TRYLOCK(m) TryEnterCriticalSection(m)
>+#define MUTEX_UNLOCK(m) LeaveCriticalSection(m)

Why do we need these macros?  To save some typing?
Attachment #374963 - Flags: superreview?(wtc)
Attachment #374963 - Flags: superreview-
Attachment #374963 - Flags: review?(ted.mielczarek)
Attachment #374502 - Flags: superreview?(wtc) → superreview+
Comment on attachment 374502 [details] [diff] [review]
Allow use of functions first declared in Win2k in NSPR 4.8 (obsoleted by patch v4)

r=wtc.  Please remove "for NSPR 4.8" from the comment.

A better solution is to do this in primpl.h, so
that it applies to all NSPR files:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/nsprpub/pr/include/private/primpl.h&rev=3.89&mark=57-67#53

We'd need to change #ifdef WINNT to #ifdef WIN32 and
change 0x0400 to 0x0500, and add the definition of
WINVER.
Attachment #374963 - Flags: review?(tellrob)
Attached patch patch v.3 (obsolete) — Splinter Review
I believe this addresses all of wtc's comments except for calling _MD_InitTime from PR_InitTime because I believe NowCalibrate does take a while.

Also, its not clear to me whether PR_CleanupTime should call _MD_CleanupTime for all platforms or just WINCE.  This patch only calls it for WINCE.  It also only changes the PR_Now implementation for WINCE and leaves the original implementation for other windows platforms.
Attachment #374963 - Attachment is obsolete: true
Attachment #375305 - Flags: review?(tellrob)
Attachment #374963 - Flags: review?(tellrob)
Comment on attachment 375305 [details] [diff] [review]
patch v.3

Since ntmisc.c, ntio.c and w95io.c are separately compiled, _pr_filetime_offset cannot be static.  If it is static, it will be uninitialized in all but one of those files.
Attachment #375305 - Flags: review-
In the interest of time, I produced this patch, based on Brad's v3, which 
makes _pr_filetime_offset an extern const, declared in primpl.h.
It also implements Wan-Teh's suggestion of defining WINVER and 
_WIN32_WINNT in primpl.h.
Attachment #375305 - Attachment is obsolete: true
Attachment #375429 - Flags: superreview?(wtc)
Attachment #375429 - Flags: review?(tellrob)
Attachment #375305 - Flags: review?(tellrob)
Attachment #374502 - Attachment description: Allow use of functions first declared in Win2k in NSPR 4.8 → Allow use of functions first declared in Win2k in NSPR 4.8 (obsoleted by patch v4)
Attachment #374502 - Attachment is obsolete: true
Comment on attachment 375429 [details] [diff] [review]
patch v4 - includes suggested primpl.h changes

r=wtc.  Thanks, Nelson.  I only reviewed the coding style.  I
didn't review the calibration algorithm.

Note that some code is incorrectly indented, for example, the
body of the NowCalibrate function.  You can easily spot the
indentation problems if you go over this patch.

Please remove these spin count macros, hopefully they're not
used:

>+#define CALIBRATIONLOCK_SPINCOUNT 0
>+#define DATALOCK_SPINCOUNT 4096
>+#define LASTLOCK_SPINCOUNT 4096
Attachment #375429 - Flags: superreview?(wtc) → superreview+
(In reply to comment #36)
> Please remove these spin count macros, hopefully they're not used:

Wan-Teh, One of them is used.  Does that give you second thoughts?
Comment on attachment 375429 [details] [diff] [review]
patch v4 - includes suggested primpl.h changes

Committed on NSPR trunk.
pr/src/misc/prtime.c;       new revision: 3.40; previous revision: 3.39
pr/include/private/primpl.h;new revision: 3.90; previous revision: 3.89
pr/src/md/windows/ntio.c;   new revision: 3.47; previous revision: 3.46
pr/src/md/windows/ntmisc.c; new revision: 3.25; previous revision: 3.24
pr/src/md/windows/w95io.c;  new revision: 3.41; previous revision: 3.40
Attachment #375429 - Flags: review?(tellrob)
Nelson, thanks for committing to CVS.

Wan-Teh, can you push these changes to mozilla-central?
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Priority: -- → P2
Resolution: --- → FIXED
Target Milestone: --- → 4.8
timeless pushed http://hg.mozilla.org/mozilla-central/rev/47e935bd7f3b, for bug 491441, picks up cvs tag  NSPR_HEAD_20090505 which includes these changes
You need to log in before you can comment on or make changes to this bug.