Improve & simplify time sync in pthreads

[RReverser]

This supersedes solution from #6084 / #10137 with performance.timeOrigin.

This property provides the high resolution baseline for performance.now() results, which is exactly what we need. Unlike custom __performance_now_clock_drift sent as a message to a worker, it won't suffer from the small time difference incurred by postMessage itself, making synchronisation more precise, while at the same time allows to remove some custom code, making it a bit simpler too.

I checked caniuse.com and it's supported in all browsers where SAB are supported, so should be always safe to use when pthreads are enabled.

Note that this changes the absolute value returned from emscripten_get_now, but this is fine because that value is already different across engines (e.g. it might already return Date.now() in some old browsers), and it's explicitly documented as "only meaningful in comparison to other calls to this function".

I verified that test_pthread_reltime still passes too.

[sbc100]

Fantasitic!

[RReverser]

Just in case - even if tests pass, want to get @kripken and/or @juj to take a look as well since they worked on the original implementation and might know if I missed some edge case.

[RReverser]

Hm, I can't reproduce that wasm2js failure locally:

> python .\test\runner.py wasm2js1.test_pthread_abort
Test suites:
['test_core']
Running test_core: (1 tests)
(checking sanity from test runner)
shared:INFO: (Emscripten: Running sanity checks)
test_pthread_abort (test_core.wasm2js1) ... ok

----------------------------------------------------------------------
Ran 1 test in 1.725s

OK

And I don't see how it could be related either. Flaky test?

[sbc100]

Hm, I can't reproduce that wasm2js failure locally:

> python .\test\runner.py wasm2js1.test_pthread_abort
Test suites:
['test_core']
Running test_core: (1 tests)
(checking sanity from test runner)
shared:INFO: (Emscripten: Running sanity checks)
test_pthread_abort (test_core.wasm2js1) ... ok

----------------------------------------------------------------------
Ran 1 test in 1.725s

OK

And I don't see how it could be related either. Flaky test?

Yup, its a known flaky test: #15014

[RReverser]

Ah thanks. So weird, it doesn't seem to rely on timing in any way :/

[RReverser]

And now it's bunch of pthread tests on Firefox instead :/ I guess one of you will have to force-merge this manually once reviews are done.

[sbc100]

And now it's bunch of pthread tests on Firefox instead :/ I guess one of you will have to force-merge this manually once reviews are done.

You should also be able to "re-run from failed" in circleci

[RReverser]

You should also be able to "re-run from failed" in circleci

Ah right. I think I tried that in the past on a different PR, but was just getting different set of failing tests each time. Might give it a go though.

[RReverser]

Yup, re-run didn't help. I sometimes get a feeling that once CI runs into this bad state, ~same pthread tests start falling more or less consistently.

[brendandahl]

I just ran into pthreads failing in #18252 too. Re-run fixed it for me though.

[sbc100]

Re-running here also seems to have fixed it.

[sbc100]

@kripken can you take a look?

[RReverser]

Re-running here also seems to have fixed it.

Heh so it's the 3rd rerun that does the charm. (I also did re-run above, but it didn't help.)

[RReverser]

Rebased & moved changelog item to 3.1.28 now that 3.1.27 is released.

[RReverser]

@kripken Ping? This is mostly a trivial change but would like another pair of eyes.

[juj]

Ops, sorry, missed the ping from before.

Unfortunately Safari only has support for performance.timeOrigin since about a year ago starting from Safari 15, so this change would regress compatibility with earlier Safari versions?

Would it be possible to add a polyfill for browsers that don't support timeOrigin? That way there won't be an impact to browser support.

[juj]

When polyfilling is not needed, this line

emscripten/src/library_pthread.js

Line 610 in 6b1e844

msg.time = performance.now();

has then become obsolete, and can be removed when only targeting browsers that do support performance.timeOrigin.

[juj]

Actually, I now realize that only Safari 15.2 adds support for multithreading, so there is no need to polyfill then. LGTM by removing the above redundant line msg.time = performance.now(); as well.

[RReverser]

Actually, I now realize that only Safari 15.2 adds support for multithreading

Yup, I was about to add my own polyfill too but then checked caniuse, as mentioned in the PR description, and found both are in the same version.

[RReverser]

LGTM by removing the above redundant line msg.time = performance.now(); as well.

Hm, I think that's the one I already removed in this PR?

[RReverser]

@juj Thanks for taking a look, sounds like this should be good to go then.

[juj]

Yeah, looks great!

[RReverser]

Ughhh these flaky tests are driving me crazy :( @sbc100 it seems to be always the same 3 that are flaky in FF, maybe we can at least disable them for now?

[sbc100]

Ughhh these flaky tests are driving me crazy :( @sbc100 it seems to be always the same 3 that are flaky in FF, maybe we can at least disable them for now?

If you have identified them could you open a bug and we can track the flakeyness. There is a tag you can use on the issue for flakey tests.

[RReverser]

There is a tag you can use on the issue for flakey tests.

Found it. #18307

[sbc100]

Apparently this change causing some safari-specific issues with SDL: #18677