-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Add emscripten_queue_microtask() API #25741
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
base: main
Are you sure you want to change the base?
Conversation
35f230a to
91daa78
Compare
|
I had a version of this API in #24481... but sure how it compares |
test/test_browser.py
Outdated
| self.btest_exit('emscripten_queue_microtask.c') | ||
|
|
||
| @requires_shared_array_buffer | ||
| def test_emscripten_queue_microtask_pthread(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other tests in this file do it this way:
@parameterized({
'': ([],),
'proxy_to_pthread': (['-pthread', '-sPROXY_TO_PTHREAD'],),
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I didn't, was to get the @requires_shared_array_buffer annotation be precise.
Though now I remember I have a pthread magic regex catch for the same purpose, so changed to this form.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be consistent and use @parameterize everywhere.
There must be magic handling that injects @requires_shared_array_buffer I suppose? If you don't like that we could make a new @also_with_proxy_to_pthread that takes care of adding the cflags and also adding requires_shared_array_buffer.
src/lib/libhtml5.js
Outdated
|
|
||
| emscripten_queue_microtask: (cb, userData) => { | ||
| queueMicrotask(() => { | ||
| {{{ makeDynCall('vp', 'cb') }}}(userData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably needs to be wrapped in callUserCallback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah.. Both the push and pop callback and callUserCallback are nice for some uses, but result in bloat :/
Added these, maybe callUserCallback() can be made zero-cost some day. (Or maybe it is so at least with Closure, not sure)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do need callUserCallback whenever we enter user code from the event loop otherwise lots of things can break. A classic one is calling exit() from within the callback.
IIRC the overhead of callUserCallback in MINIMAL_RUNTIME, where we don't have the same requirements is very close to zero. If you want to make reduce it to absolutely zero that seems like a good thing, but unrelated to this specific change.
Maybe just copy the version I had in that PR? Should probably live in libeventloop.js too. I was recently chatting to someone on chrome team about why we this API event exists given that its pretty much indistinguishable from |
| void emscripten_cancel_animation_frame(int requestAnimationFrameId); | ||
| void emscripten_request_animation_frame_loop(bool (*cb)(double time, void *userData), void *userData); | ||
|
|
||
| void emscripten_queue_microtask(void (*cb)(void *userData), void *userData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably go in emscripten/eventloop.h.
Add emscripten_queue_microtask() API to enable use case described in this thread: #25670 (comment).