Fix relative time in pthreads #9799
Conversation
|
Awesome! On this branch, I am no longer observing any negative time differences. Neither in the minimal example, nor in stockfish.wasm. |
|
Great! Thanks for confirming @niklasf |
| // performance.now() is specced to return a wallclock time in msecs since that Web Worker/main thread launched. However for pthreads this can cause | ||
| // subtle problems in emscripten_get_now() as this essentially would measure time from pthread_create(), meaning that the clocks between each threads | ||
| // would be wildly out of sync. Therefore sync all pthreads to the clock on the main browser thread, so that different threads see a somewhat | ||
| // coherent clock across each of them (+/- 0.1msecs in testing) |
There was a problem hiding this comment.
Can you wrap these comments at 80?
| pthread_attr_init(&attr); | ||
|
|
||
| pthread_t thread; | ||
| pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED); |
There was a problem hiding this comment.
Do you need to set PTHREAD_CREATE_DETACHED here? Maybe we can just remove these attrs completely and pass NULL to keep the test cleaner.
There was a problem hiding this comment.
Since we cannot join the thread in this case, it seems best to explicitly detach it. Can try to remove, if you insist.
|
|
||
| cond_var.notify_one(); | ||
|
|
||
| return NULL; |
There was a problem hiding this comment.
Remove all these extra newlines?
| # clock. | ||
| @requires_threads | ||
| def test_pthread_reltime(self): | ||
| self.btest(path_from_root('tests', 'pthread', 'test_pthread_reltime.cpp'), expected='3', args=['-s', 'USE_PTHREADS=1', '-s', 'PTHREAD_POOL_SIZE=1']) |
There was a problem hiding this comment.
Can this in node now too? We should probably have a plan for centralizing the thread tests that run in both browser and node. Perhaps test_threads.py?
There was a problem hiding this comment.
Makes sense to me, but since it seems like a larger undertaking I hope this does not block landing this fix.
| TimePoint now() { | ||
| return std::chrono::duration_cast<std::chrono::milliseconds>( | ||
| std::chrono::steady_clock::now().time_since_epoch()).count(); | ||
| } |
There was a problem hiding this comment.
I think i'd find this test more readable and use fewer lines of code is it was just C and used clock_gettime.. but thats probably personal preference so I won't ask you to change that.
|
Hi everyone! I tried to address the review in #10137 (with two exceptions commented here). |
Pthreads get the current time from their parent thread during
startup, so they can return results that make sense relative to it.
Without that, the time on a pthread can be "earlier" than
the time on the main thread, even if the pthread checks the
time later.
This broke in #9569 when we removed the magic globals
from worker.js, as now such values must be passed
explicitly. So that global just had the initial 0 value. The fix
is to pass the value on Module and use it from there, safely.
This adds a test. Timing tests are always risky, but this
just checks monotonicity of the time, and looks 100%
consistent when I run it locally, so hopefully it's ok. If we
see flakes on CI we may need to remove it though.
Fixes #9783