Skip to content

Multithreading 17/N: Sync performance.now() #6084

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 2, 2018

Conversation

juj
Copy link
Collaborator

@juj juj commented Jan 15, 2018

Synchronize emscripten_get_now() to return values that are comparable across threads. Otherwise each thread would return time compared to the start of each worker hosting a thread, causing extremely subtle bugs in multithreaded task timing code.

One might ask "why don't we spec emscripten_get_now() to count time from 0 from each pthread start" - the reasons for this is are:

  • workers are reused to host multiple pthreads. If one pthread quits, the worker is not killed but will stay around. performance.now() returns time counting from 0 from the start of that worker, and not the start of the pthread. Also, workers can be pooled up front with -s PTHREAD_POOL_SIZE=x, and the time from Worker launching to worker having compiled and loaded asm.js/wasm code for execution is nontrivial, so if we wanted to have a model that counted from start of each pthread, we'd still have to compute an offset to account for all of these deltas.
  • in other platforms there has not ever existed a wallclock timing function that was not thread coherent (to my knowledge), so a thread incoherent timing function would be a first, and all codebases would likely have to work through this gotcha specifically for the web. We could entertain a emscripten_thread_coherent_get_now() which was a synchronized variant and keep the default one unsynchronized, but that feels like leading a path towards a "best practices" tip "remember that emscripten_get_now() is broken, always use emscripten_thread_coherent_get_now()".

We could perhaps have a -s THREAD_COHERENT_EMSCRIPTEN_GET_NOW=0/1 flag that was default enabled, but for users that know they don't need it, they could disable it for a tiny code size win; but that might be premature for now, so opting to just do this by default in multithreaded builds, and keep it off from singlethreaded build mode.

@juj juj changed the title Multithreading 17/N: Sync performance now Multithreading 17/N: Sync performance.now() Jan 15, 2018
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Instead of delete obj.prop it's better to obj.prop = null (deleting changes the shape of the object, which affects optimization, and then later when you test obj.prop for existence later it changes the shape a second time).

@juj
Copy link
Collaborator Author

juj commented Jan 18, 2018

Good point about the shape. Though setting obj.prop = null; uses more memory - it still has to carry that null, and this is used only at initial thread startup time, so I don't think there's much optimization in play that could happen?

@kripken
Copy link
Member

kripken commented Jan 19, 2018

Yeah, maybe it doesn't matter much. The null by itself though would take just an immediate JS value (same as a float). Anyhow, lgtm either way.

@juj juj force-pushed the sync_performance_now branch from cc639fb to c8c9992 Compare January 31, 2018 17:35
@juj juj force-pushed the sync_performance_now branch from c8c9992 to a936dcb Compare February 2, 2018 15:56
@juj juj merged commit 08ccc11 into emscripten-core:incoming Feb 2, 2018
@juj juj deleted the sync_performance_now branch July 21, 2018 20:34
RReverser added a commit that referenced this pull request Nov 29, 2022
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 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 browser), and it's explicitly documented as "is only meaningful in comparison to other calls to this function".

I verified that test_pthread_reltime still passes too.
RReverser added a commit that referenced this pull request Nov 29, 2022
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 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 browser), and it's explicitly documented as "is only meaningful in comparison to other calls to this function".

I verified that test_pthread_reltime still passes too.
RReverser added a commit that referenced this pull request Nov 30, 2022
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 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 browser), and it's explicitly documented as "is only meaningful in comparison to other calls to this function".

I verified that test_pthread_reltime still passes too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants