[test] Convert select and poll tests to using clock_gettime#26401
[test] Convert select and poll tests to using clock_gettime#26401brendandahl wants to merge 2 commits intoemscripten-core:mainfrom
Conversation
…pten-core#26280) Prefer `clock_gettime` with `CLOCK_MONOTONIC` instead of `gettimeofday` whose implementation isn't guaranteed to be monotonic.
test/core/test_poll_blocking.c
Outdated
| if (delta_nsec < 0) { | ||
| delta_nsec += 1000000000; | ||
| delta_sec -= 1; | ||
| } |
There was a problem hiding this comment.
Why doesn't this work if you just allow delta_nsec to remain negative and then it will act as a subtraction below? (like this the version of this PR that I landed).
There was a problem hiding this comment.
The previous version would round down. For example:
(0 * 1000) + (299999999 / 1000000) would become 299ms. In the end I don't think this change really matters much, because I've seen the duration off by as much as 17ms locally.
There was a problem hiding this comment.
I see so that + 500000 here is the fix to avoid the rounding down? And that means delta_nsec cannot be left as negative?
There was a problem hiding this comment.
Yeah, changes it to round to nearest.
There was a problem hiding this comment.
Actually don't we want to round down here.
i.e if wait for 999 ms we don't want report that a waiting for whole second, because a whole second has not elapsed yet.
Same with 1 millisecond + 999 microsecond .. that still is not 2ms and if 2ms was the target 1.999 is not enough time.
There was a problem hiding this comment.
I guess it depends how precise the clock is expected to be. The new code rounds to nearest microsecond. e.g.
999'500'000 becomes 1000ms
999'499'999 becomes 999ms
Anyway, I can revert part as it doesn't really matter since I see the duration is multiple milliseconds early locally.
test/core/test_poll_blocking.c
Outdated
|
|
||
| #define TIMEOUT_MS 300 | ||
| // It is possible for the Javascript timers (such as setTimeout or Atomics.wait) | ||
| // to wake up slightly earlier than requested. Because we measure times accurately using |
There was a problem hiding this comment.
Is this actually true? What source did you use for this? All the specs I could find out setTimeout said it would always block for at least that number of milliseconds and never less.
There was a problem hiding this comment.
I'm not sure about atomic wait, but
The callback will likely not be invoked in precisely delay milliseconds. Node.js makes no guarantees about the exact timing of when callbacks will fire, nor of their ordering. The callback will be called as close as possible to the time specified.
https://nodejs.org/docs/latest/api/timers.html#settimeoutcallback-delay-args
There was a problem hiding this comment.
Can you perhaps call out node in particular here because I think the webplatform setTimeouts does not allow any kind of easrly callback (at least if MDN is to be believed).
test/core/test_poll_blocking.c
Outdated
|
|
||
| #define TIMEOUT_MS 300 | ||
| // It is possible for the Javascript timers (such as setTimeout or Atomics.wait) | ||
| // to wake up slightly earlier than requested. Because we measure times accurately using |
There was a problem hiding this comment.
Can you perhaps call out node in particular here because I think the webplatform setTimeouts does not allow any kind of easrly callback (at least if MDN is to be believed).
brendandahl
force-pushed
the
flaky-select
branch
from
1b62318 to
82e1df1
Compare
brendandahl
force-pushed
the
flaky-select
branch
from
82e1df1 to
a3c4f9d
Compare
3 participants
Prefer
clock_gettimewithCLOCK_MONOTONICinstead ofgettimeofdaywhose implementation isn't guaranteed to be monotonic.
Add some margin for error in the timeout asserts.
Normalize negative nano seconds in time delta.