164841 - NSPR uses low-resolution timer on OS/2

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P3)

Description

[Julien Pierre]

NSPR currently implements the _PR_MD_GET_INTERVAL() function with
DosQuerySysInfo(QSV_MS_COUNT, QSV_MS_COUNT, &msCount, sizeof(msCount)); .
This only returns a value in milliseconds.

OS/2 has had a high-resolution 1.2 MHz timer function from day 1. The functions
to query the frequency and timestamps are DosTmrQueryFreq and DosTmrQueryTime .
They are basically the same as NT's QueryPerformanceFrequency and
QueryPerformanceCounter. Actually Microsoft just renamed the OS/2 functions when
they released NT.

It just so happens that I have a requirement to measure very short interval for
performance work on NSS. All intervals under 1 millisecond were showing as zero
in my tests and that was a big issue.

Comment 1

[Julien Pierre]

Attachment: make NSPR use high-resolution timer on OS/2. Patch borrowed heavily from NT's NSPR interval implementation

Comment 2

[Julien Pierre]

FYI, I have tested this patch and it works.

Test without patch :

[e:\dev\nss\36\mozilla\dist\os22.45_icc_dbg.obj\bin]revoked.cmd
Time for initial uncached verification :  0: 6.965000

Time for 2000 cached verifications :  0: 0.555000

Average time per cached verification :  0: 0.     0

E:\DEV\NSS\36\MOZILLA\DIST\OS22.45_ICC_DBG.OBJ\BIN\CERTUTIL.EXE: certificate is
invalid: Peer's Certificate has been revoked.

Test with patch :

[e:\dev\nss\36\mozilla\dist\os22.45_icc_dbg.obj\bin]revoked.cmd
Time for initial uncached verification :  0: 6.933648

Time for 2000 cached verifications :  0: 0.535710

Average time per cached verification :  0: 0.   255

E:\DEV\NSS\36\MOZILLA\DIST\OS22.45_ICC_DBG.OBJ\BIN\CERTUTIL.EXE: certificate is
invalid: Peer's Certificate has been revoked.

Comment 3

[Mike Kaply [:mkaply]]

This was done deliberately.

Please take a look at the log for that file, specifically the bug for which we
put in the low resolution timer and make sure that bug doesn't still happen.

http://bugzilla.mozilla.org/show_bug.cgi?id=136958

CVS log is your friend.

Comment 4

[Julien Pierre]

Mike,

I looked at that other bug. The problem in the browser code, not in NSPR. It's
just using the wrong timer function. For measuring intervals longer than 5
hours, it must call PR_Now() rather than PR_IntervalNow(). This is because of
the semantics of the PR_IntervalNow() function. I don't think the fact that one
NSPR application is broken is good reason to lower the timer resolution for
every application. The broken application - in this case the browser - needs to
be fixed.

From what I see in that defect, this lowering of the resolution only happened in
the OS/2 version of NSPR. NSPR on Windows is still using the high performance
timer. So the wrapping bug still exists in the browser on Windows.

My suggestion is :
- 136958 should be made an XP bug to replace incorrect PR_IntervalNow() calls
with PR_Now() in places that may need >5 hrs intervals.

- the high-res timer should be put back in OS/2 NSPR

Comment 5

[Mike Kaply [:mkaply]]

r=mkaply for timer patch

Comment 6

[Wan-Teh Chang]

Attachment: Patch v2

I made some minor changes to Julien's patch.

1. useHighResTimer is initialized to PR_FALSE instead of
PR_TRUE because the variables needed by the high resolution
timer (_os2_bitShift, _os2_highMask, and _os2_ticksPerSec)
do not have usable initial values.

2. I declare _os2_ticksPerSec as a PRIntervalTime instead of
a PRInt32.

3. I rewrote the assertion so that it is not the only
statement in the body of "else".  Some compilers complain
about an empty "else" body in an optimized build (in which
assertions are compiled away).

4. In _PR_MD_GET_INTERVAL(), I have it return -1 instead
of PR_FAILURE on failure.  PR_FAILURE has the value of -1,
but it is supposed to be a value of the PRStatus type, and
this function returns a PRIntervalTime.  Actually -1 is not
an appropriate error return value for this function either.
(All the values in the range of PRIntervalTime are valid.)

5. Miscellaneous coding style changes.

Comment 7

[Wan-Teh Chang]

Attachment: Patch v3

Fixed two bugs in the _PR_MD_INTERVAL_INIT() function in
patch v2.

1. useHighResTimer should be initialized to PR_TRUE if
the DosTmrQueryFreq() call succeeds because it is initialized
to PR_FALSE.

2. The setting of _os2_bitShift, _os2_highMask, and
_os2_ticksPerSec should only be done if useHighResTimer is
true.

Comment 8

[Wan-Teh Chang]

Patch checked into NSPR TIP (NSPR 4.3) and the
NSPRPUB_PRE_4_2_CLIENT_BRANCH (mozilal 1.4alpha).

Comment 9

[Sam Emrick]

For documentation here: 
The subject patch causes strangeness when used with an arcane os/2 
command which dynamicly alters the high-resolution timer frequency.
 STRACE SETRATE n 
will set the frequency to n ticks per second. n=0 returns to default. 

Thus command STRACE SETRATE 1000 with this patch will cause, for 
example, animated gifs to display at very fast rates. Issue the command 
STRACE SETRATE 0 and then animated gifs return to normal speed. 

This command and its usage is arcane enough that I don't think the 
patch should be changed to accomodate it. The purpose of this comment 
is to document the problem and the workaround