Skip to content

Commit

Permalink
fix formatting and a bug in clock_getres, and add unittest
Browse files Browse the repository at this point in the history
  • Loading branch information
@NWilson
NWilson committed Sep 5, 2014
1 parent c8bf0da commit ef48462
Show file tree
Hide file tree
Showing 6 changed files with 86 additions and 14 deletions.
2 changes: 1 addition & 1 deletion emscripten-version.txt
View file
Original file line number Diff line number Diff line change
@@ -1,2 +1,2 @@
1.23.2
1.23.3

26 changes: 14 additions & 12 deletions src/library.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5783,13 +5783,13 @@ LibraryManager.library = {
}
return _usleep((seconds * 1e6) + (nanoseconds / 1000));
},
clock_gettime__deps: ['emscripten_get_now','emscripten_get_now_monotonic','$ERRNO_CODES', '__setErrNo'],
clock_gettime__deps: ['emscripten_get_now', 'emscripten_get_now_is_monotonic', '$ERRNO_CODES', '__setErrNo'],
clock_gettime: function(clk_id, tp) {
// int clock_gettime(clockid_t clk_id, struct timespec *tp);
var now;
if (clk_id === {{{ cDefine('CLOCK_REALTIME') }}}) {
now = Date.now();
} else if (clk_id === {{{ cDefine('CLOCK_MONOTONIC') }}} && _emscripten_get_now_monotonic()) {
} else if (clk_id === {{{ cDefine('CLOCK_MONOTONIC') }}} && _emscripten_get_now_is_monotonic()) {
now = _emscripten_get_now();
} else {
___setErrNo(ERRNO_CODES.EINVAL);
Expand All @@ -5803,23 +5803,24 @@ LibraryManager.library = {
clock_settime: function(clk_id, tp) {
// int clock_settime(clockid_t clk_id, const struct timespec *tp);
// Nothing.
___setErrNo(ERRNO_CODES.EPERM);
___setErrNo(clk_id === {{{ cDefine('CLOCK_REALTIME') }}} ? ERRNO_CODES.EPERM
: ERRNO_CODES.EINVAL);
return -1;
},
clock_getres__deps: ['emscripten_get_now_res','emscripten_get_now_monotonic','$ERRNO_CODES', '__setErrNo'],
clock_getres__deps: ['emscripten_get_now_res', 'emscripten_get_now_is_monotonic', '$ERRNO_CODES', '__setErrNo'],
clock_getres: function(clk_id, res) {
// int clock_getres(clockid_t clk_id, struct timespec *res);
var nsec;
if (clk_id === {{{ cDefine('CLOCK_REALTIME') }}}) {
nsec = 1000 * 1000;
} else if (clk_id === {{{ cDefine('CLOCK_MONOTONIC') }}} && _emscripten_get_now_monotonic()) {
now = _emscripten_get_now_res();
nsec = 1000 * 1000; // educated guess that it's milliseconds
} else if (clk_id === {{{ cDefine('CLOCK_MONOTONIC') }}} && _emscripten_get_now_is_monotonic()) {
nsec = _emscripten_get_now_res();
} else {
___setErrNo(ERRNO_CODES.EINVAL);
return -1;
}
{{{ makeSetValue('res', C_STRUCTS.timespec.tv_sec, '1', 'i32') }}};
{{{ makeSetValue('res', C_STRUCTS.timespec.tv_nsec, 'nsec', 'i32') }}} // resolution is milliseconds
{{{ makeSetValue('res', C_STRUCTS.timespec.tv_sec, '(nsec/1000000000)|0', 'i32') }}};
{{{ makeSetValue('res', C_STRUCTS.timespec.tv_nsec, 'nsec', 'i32') }}} // resolution is nanoseconds
return 0;
},

Expand Down Expand Up @@ -8502,11 +8503,12 @@ LibraryManager.library = {
}
},

emscripten_get_now_monotonic__deps: ['emscripten_get_now'],
emscripten_get_now_monotonic: function() {
emscripten_get_now_is_monotonic__deps: ['emscripten_get_now'],
emscripten_get_now_is_monotonic: function() {
// return whether emscripten_get_now is guaranteed monotonic; the Date.now
// implementation is not :(
return (_emscripten_get_now() == Date.now)|0;
return ENVIRONMENT_IS_NODE || (typeof dateNow !== 'undefined') ||
(ENVIRONMENT_IS_WEB && window['performance'] && window['performance']['now']);
},

// Returns [parentFuncArguments, functionName, paramListName]
Expand Down
47 changes: 47 additions & 0 deletions tests/core/test_posixtime.in
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
#include <time.h>
#include <errno.h>
#include <stdio.h>

int main() {
clockid_t clocks[] = { CLOCK_REALTIME, CLOCK_MONOTONIC };
for (int i = 0; i < (int)(sizeof(clocks)/sizeof(*clocks)); ++i) {
printf("%sTests for clockid_t=%d\n-----------------\n",
i == 0 ? "" : "\n", clocks[i]);
struct timespec ts;
int rv = clock_getres(clocks[i], &ts);
if (rv)
printf("clock_getres failed\n");
else if (ts.tv_sec || ts.tv_nsec > 50000000)
printf("clock_getres resolution not enough (%ld.%09ld)\n",
(long)ts.tv_sec, ts.tv_nsec);
else
printf("clock_getres resolution OK\n");
rv = clock_gettime(clocks[i], &ts);
printf(rv ? "clock_gettime failed\n" : "clock_gettime OK\n");
errno = 0;
if (clock_settime(clocks[i], &ts) == 0)
printf("clock_settime should have failed\n");
else if (errno == EPERM && clocks[i] == CLOCK_REALTIME)
printf("clock_settime failed with EPERM (OK)\n");
else if (errno == EINVAL && clocks[i] == CLOCK_MONOTONIC)
printf("clock_settime failed with EINVAL (OK)\n");
else
printf("clock_settime failed with wrong error code\n");
}
clockid_t bogus = 42;
struct timespec ts;
printf("\nTests for clockid_t=%d\n-----------------\n", bogus);
if (clock_gettime(bogus, &ts) == 0 || errno != EINVAL)
printf("clock_gettime should have failed\n");
else
printf("clock_gettime failed with EINVAL (OK)\n");
if (clock_getres(bogus, &ts) == 0 || errno != EINVAL)
printf("clock_getres should have failed\n");
else
printf("clock_getres failed with EINVAL (OK)\n");
if (clock_settime(bogus, &ts) == 0 || errno != EINVAL)
printf("clock_settime should have failed\n");
else
printf("clock_settime failed with EINVAL (OK)\n");
return 0;
}
17 changes: 17 additions & 0 deletions tests/core/test_posixtime.out
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
Tests for clockid_t=0
-----------------
clock_getres resolution OK
clock_gettime OK
clock_settime failed with EPERM (OK)

Tests for clockid_t=1
-----------------
clock_getres resolution OK
clock_gettime OK
clock_settime failed with EINVAL (OK)

Tests for clockid_t=42
-----------------
clock_gettime failed with EINVAL (OK)
clock_getres failed with EINVAL (OK)
clock_settime failed with EINVAL (OK)
6 changes: 6 additions & 0 deletions tests/test_core.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4629,6 +4629,12 @@ def test_unistd_misc(self):
Building.COMPILER_TEST_OPTS += ['-D' + fs]
self.do_run(src, expected, js_engines=[NODE_JS])

def test_posixtime(self):
test_path = path_from_root('tests', 'core', 'test_posixtime')
src, output = (test_path + s for s in ('.in', '.out'))

self.do_run_from_file(src, output)

def test_uname(self):
test_path = path_from_root('tests', 'core', 'test_uname')
src, output = (test_path + s for s in ('.in', '.out'))
Expand Down
2 changes: 1 addition & 1 deletion tests/time/src.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ int main() {
time_t t4 = 0;
time(&t4);
timespec ts;
assert(clock_gettime(0, &ts) == 0);
assert(clock_gettime(CLOCK_REALTIME, &ts) == 0);
assert(abs(ts.tv_sec - t4) <= 2);
printf("time: %d\n", t4 > 1309635200ll && t4 < 1893362400ll);

Expand Down

9 comments on commit ef48462

@sbalko
Copy link
Contributor

@sbalko sbalko commented on ef48462 Oct 31, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

I traced back a newly occurring problem re clock_gettime() to this patch of yours. Specifically, I have built a legacy code base which invokes clock_gettime() with CLOCK_MONOTONIC. This used to work fine before a recent chunk of patches that start using the window.performance high-precision timing API. While I like this change, I don't think that rejecting clock_gettime() invocations with CLOCK_MONOTONIC on platforms not supporting the Performance API is justified. Even if Date.now() is not always monotonic (even though I don't understand when it wouldn't be), this is Javascript land, and I don't think that cross-compiled code should be/has to be that picky. After all, Javascript is ill-suited to all purposes where this would matter, such as programming real-time software.

As a side note: the Performance API is available in Workers on some platforms (Chrome, Opera). I will submit a patch for this isolated problem and also submit another patch, proposing to relax the "monotonicity" criterion such that a Date.now() fallback qualifies.

Soeren

@kripken
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Date.now can just call an underlying system call, which might not be monotonic. I'm not sure how common that is, but performance.now was designed in part to solve the problem of Date.now not being monotonic.

I think we should default to rejecting MONOTONIC if all we have is Date.now as we currently do, to be as compatible with other platforms as possible. However, what do you think about the following options?

  1. If we don't have performance.now, we can "monotonize" Date.now (by ensuring we return values >= than the last).
  2. Have an option to report Date.now as monotonic.

@kripken
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I remember now, Date.now can easily be non-monotonic if the system time changes. Given that, option 1 that I just mentioned is probably not a good idea.

@sbalko
Copy link
Contributor

@sbalko sbalko commented on ef48462 Oct 31, 2014 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kripken
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my last comment, one main reason it can be nonmonotonic is that the system time can change. Date.now is literally the time since the epoch, so if daylight savings happens or the system syncs with a network clock, Date.now can definitely be nonmonotonic.

Still, we could add an option, off by default, to ignore that, as in 2 above.

@sbalko
Copy link
Contributor

@sbalko sbalko commented on ef48462 Oct 31, 2014 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NWilson
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to "monotonize" the wall clock if you prefer monotonicity to constant-length seconds. One option is to do something like this:

(global vars) bias = null, last = null;
var now = Date.time();
if (bias === null) bias = now;
var rv = now - bias;
if (last !== null && rv < last) { bias -= last - rv; rv = last; }
last = rv;
return rv;

Of course, it's all trade-offs! You get monotonicity, but you don't get constant-length seconds with this API because the values returned by the API don't advance when the underlying clock skips. In an extreme case, if the system clock jumps every second (say) then you could call the API every second and it would always return the same value, even though time is advancing -- it just can't detect that.

To be honest, the reason I wanted to prevent CLOCK_MONOTONIC from use Date.now is that our codebase already has sophisticated workarounds for platforms (embedded and old desktop OSes) without a monotonic timer -- in particular, we have cunning rescheduling of all our timer events when we detect clock skips forwards and backwards.

Forwards jumps are the killers! All you have to do is close a laptop lid and reopen it, and Date.now has basically jumped forwards, which a good CLOCK_MONOTONIC should not do (it doesn't include suspend time). I keep meaning to file a bug on Chrome and Firefox, which have different behaviour on this one depending on the OS they're running on in their window.performance implementation.

@sbalko
Copy link
Contributor

@sbalko sbalko commented on ef48462 Oct 31, 2014 via email

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kripken
Copy link
Member

@kripken kripken commented on ef48462 Nov 1, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, how about an option called "ASSUME_DATE_NOW_MONOTONIC" for option 2 from before?

Please sign in to comment.