Closed Bug 508165 Opened 10 years ago Closed 10 years ago

PR_Now taking 1-2ms on Windows CE

Categories

(NSPR :: NSPR, defect)

ARM
Windows CE
defect
Not set

Tracking

(status1.9.2 beta1-fixed)

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: vlad, Assigned: vlad)

Details

(Whiteboard: [nv])

Attachments

(2 files, 3 obsolete files)

The current PR_Now impl (which is the complex desktop windows impl) seems to take around 1.6ms under CE, at least under CE6.  CE6 has GetSystemTimeAsFileTime which gives 1ms precision, but it's not present pre-CE6.

It's possible that this is a problem on CE6 only, and that things like mutexes are more expensive there.. but the impl needs to do a lot of work that can't be cheap.  We should measure how expensive it is first to verify this.
Testing on an HTC Touch Pro, PR_Now is averaging 0.02465ms per call as measured by GetTickCount(). Measuring with PR_Now it averages 26ms, so I think the real problem might be drift.
Attached patch use new WinCE6 API if available (obsolete) — Splinter Review
This uses the new GetSystemTimeAsFileTime API available in CE6.  It seems to provide 1ms resolution.
Assignee: nobody → vladimir
Attachment #393390 - Flags: review?
Attachment #393390 - Flags: review? → review?(wtc)
Comment on attachment 393390 [details] [diff] [review]
use new WinCE6 API if available

Thanks for the patch.

>+static PRTime OffsetFileTimeValueToPRTime(const PRTime t) {
>+    return (t - _pr_filetime_offset) / 10LL;
>+}

This function should take FILETIME instead of PRTime as argument.
FILETIME is a structure of two DWORDs, so it may be unsafe to cast
it to PRTime.  You need to use a union or do something like
  FILETIME ft;
  PRTime t = ft.dwHighDateTime;
  t = (t << 32) + ft.dwLowDateTime;

Also, we can just name this function FileTimeToPRTime.

Let's move the following declaration to
mozilla/nsprpub/pr/include/md/_win95.h:

+#ifdef WINCE
+extern PRStatus _MD_InitTime();
+extern void _MD_CleanupTime();
+#endif
(In reply to comment #3)
> (From update of attachment 393390 [details] [diff] [review])
> Thanks for the patch.
> 
> >+static PRTime OffsetFileTimeValueToPRTime(const PRTime t) {
> >+    return (t - _pr_filetime_offset) / 10LL;
> >+}
> 
> This function should take FILETIME instead of PRTime as argument.
> FILETIME is a structure of two DWORDs, so it may be unsafe to cast
> it to PRTime.  You need to use a union or do something like
>   FILETIME ft;
>   PRTime t = ft.dwHighDateTime;
>   t = (t << 32) + ft.dwLowDateTime;

_PR_FileTimeToPRTime already makes this assumption at

http://mxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/md/windows/w95io.c#778

The only reason why I didn't use it is because it's silly to make a function call in performance critical code like this, given that PR_Now gets called a lot all over the place.

It also copies to a different location, instead of just doing the math in-place.  It doesn't really need to be a function though, I can just put the math right inside PR_Now, so I'll do that.

> Also, we can just name this function FileTimeToPRTime.

Collision with the already existing _PR_FileTimeToPRTime that has slightly different semantics.

> Let's move the following declaration to
> mozilla/nsprpub/pr/include/md/_win95.h:
> 
> +#ifdef WINCE
> +extern PRStatus _MD_InitTime();
> +extern void _MD_CleanupTime();
> +#endif

Will do.
Attached patch updated (obsolete) — Splinter Review
Updated patch with review comments; timing indicated that (time/10LL - offset/10LL) was a good chunk faster than (time - offset) * 0.1 and (time - offset) / 10LL (in that order -- (a-b)/10LL was the slowest).
Attachment #393390 - Attachment is obsolete: true
Attachment #396792 - Flags: review?(wtc)
Attachment #393390 - Flags: review?(wtc)
Vlad,

In the interest of time, I edited your patch and checked it in
on the NSPR trunk (NSPR 4.8.1).

Checking in pr/include/md/_win95.h;
/cvsroot/mozilla/nsprpub/pr/include/md/_win95.h,v  <--  _win95.h
new revision: 3.37; previous revision: 3.36
done
Checking in pr/src/md/windows/ntmisc.c;
/cvsroot/mozilla/nsprpub/pr/src/md/windows/ntmisc.c,v  <--  ntmisc.c
new revision: 3.28; previous revision: 3.27
done
Checking in pr/src/misc/prtime.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prtime.c,v  <--  prtime.c
new revision: 3.43; previous revision: 3.42
done

I made _MD_InitTime return void (it had to return PRStatus when
it was passed to PR_CallOnce).  In C, functions that take no
parameters need to declared with 'void' as parameter list.

Just curious: why do we need to cast the return value of
mozce_GetEnvString to PRUnichar * ?

Please attach a follow-up patch to address the following issues.

1. The function pointer type name PGETSYSTEMTIMEASFILETIME
is hard to read.  Please rename it GetSystemTimeAsFileTimeFcn.

The FILETIME2INT64 macro should also be renamed
FILETIME_TO_INT64.

2. In _MD_InitTime, we call LoadLibraryW to load coredll.dll.
The library handle is leaked.  We should save the library
handle and pass it to FreeLibrary in _MD_CleanupTime.
Better yet, if NSPR is already linked with coredll.dll,
we can just call GetModuleHandle.

3. The following code has alignment problem:

    PRInt64 returnedTime;
    ...
        PR_ASSERT(sizeof(FILETIME) == sizeof(PRTime));

        ce6_GetSystemTimeAsFileTime((FILETIME*) &returnedTime);

The assertion ensures that FILETIME and PRTime are the
same size, but that doesn't imply they have the same
alignment requirement.  FILETIME is a structure of two
DWORDs, while PRTime is a 64-bit integer.  You can use
a union to ensure alignment:

    union {
        FILETIME ft;
        PRTime prt;
    } systime;
    ...
        ce6_GetSystemTimeAsFileTime(&systime.ft);'
    ...
        return systime.prt/_pr_filetime_divisor -
               _pr_filetime_offset/_pr_filetime_divisor;
Attachment #396792 - Attachment is obsolete: true
Attachment #396792 - Flags: review?(wtc)
Attached patch followup (obsolete) — Splinter Review
Thanks, wtc -- here's the followup patch as requested.

Regarding the cast -- I forgot that was in the patch, I was getting a compile warning there (casting int to PRUnichar), but I think that was due to a shunt compiler bug on my end.
Attachment #396875 - Flags: review?(wtc)
I also renamed FILETIME2INT64 to FILETIME_TO_INT64.

Note: I removed the (PRUnichar *) typecast.  The compiler warning
(casting int to PRUnichar) usually means you aren't declaring the
mozce_GetEnvString() function.  We should fix that by declaring
the function (by including some header?).
Attachment #396875 - Attachment is obsolete: true
Attachment #396875 - Flags: review?(wtc)
Status: NEW → ASSIGNED
Component: General → NSPR
Product: Core → NSPR
QA Contact: general → nspr
Target Milestone: --- → 4.8.1
Version: Trunk → other
Comment on attachment 396900 [details] [diff] [review]
followup patch (as checked in)

>-        ce6_GetSystemTimeAsFileTime((FILETIME*) &returnedTime);

I also figured out why this didn't crash for you.  PRTime has
more stringent alignment requirement than FILETIME, so it is
safe to cast it to FILETIME.
Ah yes, since that's the 64-bit type.  The union is clearer anyway. :-)

Yeah, removing the cast is fine.  The header it comes from is force-included everywhere by the WinCE compiler wrapper disaster, but I had the wrong version of the compiler wrapper built when I was testing it.
How do we get this checked in to m-c and 1.9.2?
I will take care of that, no later than this weekend.
Could you get the necessary approval for 1.9.2?  Thanks.
I pushed the patches as part of the NSPR_HEAD_20090828 CVS tag to:
- mozilla-central in changeset 5375876a9319
  http://hg.mozilla.org/mozilla-central/rev/5375876a9319
- mozilla-1.9.2 in changeset 8227c178816b
  http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8227c178816b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: fixed1.9.2
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.