508165 - PR_Now taking 1-2ms on Windows CE

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 1

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

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.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: use new WinCE6 API if available

This uses the new GetSystemTimeAsFileTime API available in CE6.  It seems to provide 1ms resolution.

Comment 3

[Wan-Teh Chang]

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

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(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.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated

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).

Comment 6

[Wan-Teh Chang]

Attachment: use new WinCE6 API if available (as checked in)

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;

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: followup

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.

Comment 8

[Wan-Teh Chang]

Attachment: followup patch (as checked in)

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?).

Comment 9

[Wan-Teh Chang]

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.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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.

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

How do we get this checked in to m-c and 1.9.2?

Comment 12

[Wan-Teh Chang]

I will take care of that, no later than this weekend.
Could you get the necessary approval for 1.9.2?  Thanks.

Comment 13

[Wan-Teh Chang]

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