win: more reliable uv__hrtime precision #292
Conversation
|
Possibly related to nodejs/node-v0.x-archive#8960 |
|
/cc @piscisaureus or @orangemocha care to review? |
| @@ -59,7 +59,7 @@ static char *process_title; | |||
| static CRITICAL_SECTION process_title_lock; | |||
|
|
|||
| /* Interval (in seconds) of the high-resolution clock. */ | |||
| static double hrtime_interval_ = 0; | |||
| static long hrtime_interval_ = 0; | |||
There was a problem hiding this comment.
long is 32 bits on Windows, I think you want uint64_t.
There was a problem hiding this comment.
The return from .QuadPart is LONGLONG apparently. Same thing as uint64_t?
https://msdn.microsoft.com/en-us/library/windows/desktop/aa383713(v=vs.85).aspx
There was a problem hiding this comment.
Almost. LONGLONG equals int64_t.
|
Ideally, |
| @@ -484,7 +484,7 @@ uint64_t uv__hrtime(double scale) { | |||
| * performance counter interval, integer math could cause this computation | |||
| * to overflow. Therefore we resort to floating point math. | |||
| */ | |||
| return (uint64_t) ((double) counter.QuadPart * hrtime_interval_ * scale); | |||
| return (uint64_t) (counter.QuadPart * (long) scale / hrtime_interval_); | |||
There was a problem hiding this comment.
Why use long. That's 32-bit!
Fishrock123
force-pushed
the
windows-hrtime
branch
from
a003e78
to
bf96784
Compare
|
Updated, PTAL. (sorry, forgot about the whole |
| @@ -59,7 +59,7 @@ static char *process_title; | |||
| static CRITICAL_SECTION process_title_lock; | |||
|
|
|||
| /* Interval (in seconds) of the high-resolution clock. */ | |||
There was a problem hiding this comment.
note to self: update this comment comment(s).
|
-1 |
|
@orangemocha perhaps it would be better to use a separate |
| @@ -59,7 +59,7 @@ static char *process_title; | |||
| static CRITICAL_SECTION process_title_lock; | |||
|
|
|||
| /* Interval (in seconds) of the high-resolution clock. */ | |||
| static double hrtime_interval_ = 0; | |||
| static uint64_t hrtime_interval_ = 0; | |||
There was a problem hiding this comment.
Rename it to hrtimer_frequency_ instead of interval?
Fishrock123
force-pushed
the
windows-hrtime
branch
from
bf96784
to
5d68ccf
Compare
|
Updated to use To actually do it with 128 bit, I think it would have to be one of these two to do the multiplication: https://msdn.microsoft.com/en-us/library/windows/desktop/hh802934(v=vs.85).aspx But I really don't grok how they work. Also, it looks like bits would be lost in the conversion back to 64bit anyways, since windows does not have a 128bit divide. |
Reduces floating-point operations that can cause precision loss. Thanks to @Arnavion for suggesting the fix: nodejs/node#1272 (comment)
Fishrock123
force-pushed
the
windows-hrtime
branch
from
5d68ccf
to
8efe97d
Compare
| /* Interval (in seconds) of the high-resolution clock. */ | ||
| static double hrtime_interval_ = 0; | ||
| /* Interval of the high-resolution clock. */ | ||
| static uint64_t hrtime_interval_ = 0; |
There was a problem hiding this comment.
updated the comments, but I'm not 100% sure what unit this is now in
There was a problem hiding this comment.
My old comment still stands. This isn't an interval now - it's a frequency (Hz).
Eliminates floating-point operations that can cause precision loss.
Thanks to @Arnavion for suggesting the fix:
nodejs/node#1272 (comment)
(And IRC discussion in #io.js: http://logs.libuv.org/io.js/latest#18:25:27.988)
Note: first time writing any C.
Also, I don't have a windows machine configured to test this on.