176881 - PRInterval timers may be incorrect on Windows with processor speed > 4 GHz

Status: RESOLVED FIXED

(NSPR :: NSPR, defect, P1)

Description

[Wan-Teh Chang]

In ntmisc.c, we have

void
_PR_MD_INTERVAL_INIT()
{
    LARGE_INTEGER count;

    if (QueryPerformanceFrequency(&count)) {
        while(count.LowPart > PR_INTERVAL_MAX) {
            count.LowPart >>= 1;
            _nt_bitShift++;
            _nt_highMask = (_nt_highMask << 1)+1;
        }

        _nt_ticksPerSec = count.LowPart;
        PR_ASSERT(_nt_ticksPerSec > PR_INTERVAL_MIN);
    } else
        _nt_ticksPerSec = -1;
}

Note that the code assumes count.HighPart is 0
and ignores it.  count.LowPart is an unsigned
32-bit integer (DWORD).

On some computers, the result returned by
QueryPerformanceFrequency will be the clock speed
of the CPU, and our code will break if the
processor runs faster than 2^32 Hz, or ~ 4 GHz.

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch

Mike, I'd appreciate it if you could review this patch.

With this patch, NSPR no longer ignores the high 32
bits of the performance counter frequency.  I also
cleaned up the code a little bit.

1. I found that _nt_highMask is not really necessary
and deleted it.

2. I replaced the multiple-step computation of the
current time stamp into a single arithmetic expression
in _PR_MD_GET_INTERVAL.  I don't know why the original
code does the shifting and addition in separate
statements and reuses count.LowPart to store the
intermediate and final results.  I think combining
these statements into one expression makes the code
easier to understand.

Comment 2

[Wan-Teh Chang]

Attachment: Proposed patch v2

Please use this patch instead.	The previous patch
was generated from an unreleased version of the file.

Comment 3

[mike belshe]

I tested this code on my machine, which has a frequency of 3579545ticks/sec 
(less than 32 bits).  It tests fine in that case and does not differ from the 
original implementation.  But I don't have a higher frequency clock to test 
with here, so I can't test that case.  Visual inspection looks right.

To be honest, I wonder if we aren't over-engineering here.  Do we really need a 
sub-millisecond timer?  Maybe just obliterate all this stuff and use 
GetTickCount() (units in milliseconds, 10ms resolution).  There are enough 
platform-specific warnings in the MSFT documentation that I don't really trust 
QueryPerformanceCounter() to work in a portable way (MP issues, clock speed 
issues, HW issues).  I'm pretty sure that attempting to use it will have future 
incompatibilities which we can probably avoid by using a more supported call.

Comment 4

[Wan-Teh Chang]

Attachment: Test program for NSPR's interval timer

On current processors we need to patch the NSPR
library to artificially make the performance
counter frequency greater than 4 GHz.  The patches
will be attached next.

Comment 5

[Wan-Teh Chang]

Attachment: Patch for the old (rev. 3.8) ntinrval.c

Apply this patch to mozilla/nsprpub/pr/src/md/windows/ntinrval.c,
rev. 3.8.

Adjust the value of SHIFT until 'freq' is more than 32 bits.

Then run the test program against the modified NSPR library.
You'll see that the elapsed time is larger than 1000 ms (the
expected value).

Comment 6

[Wan-Teh Chang]

Attachment: Patch for the new (rev. 3.10) ntinrval.c

Apply this patch to mozilla/nsprpub/pr/src/md/windows/ntinrval.c,
rev. 3.10.

Adjust the value of SHIFT until 'freq' is more than 32 bits.
(This step actually requires running the test program, which
prints the value of 'freq'.)

Then run the test program against the modified NSPR library.
You'll see that the elapsed time is 1000 ms (the expected value).

Comment 7

[Wan-Teh Chang]

Mike,

Thank you for the code review.  I've checked in
the patch.

I agree that QueryPerformanceCounter() has many
problems and I will consider using GetTickCount().
The purpose of this bug is to document a potential
problem of our code, just in case someone will
install old NSPR releases (<= 4.2.2) on a 4GHz
machine.

I hope we can track down the causes of the problems
reported in the original bug about PRIntervalTime
(bug 115865) before making the decision to switch
from QueryPerformanceCounter() to GetTickCount().
In the meantime I want to fix this known bug in
our code.