-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Add basic support for Fibers with JSPI #25111
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
d8 used for tests doesn't provide that
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.
Looks good overall. Maybe we want to add an assertion that we're using a fiber on the same thread it was created from? I'm not sure the asyncify version even supports resuming on different threads, so that may not really be important.
@@ -569,6 +569,10 @@ addToLibrary({ | |||
|
|||
$Fibers__deps: ['$Asyncify', 'emscripten_stack_set_limits', '$stackRestore'], | |||
$Fibers: { | |||
#if ASYNCIFY == 2 | |||
lastContinuationId: 0, | |||
continuations: [], |
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.
It looks like we add to this but never remove anything.
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.
Did you mean nullifying the old continuation references [commited] or was it about the fact that ids only grow but are never reclaimed? Ids work just like rewindId
and I don't see callStackIdToFunc
being cleared either.. (not that it's perfect)
'legacy': (1,), | ||
}) | ||
def test_fibers_setjmp(self, legacy): | ||
self.maybe_closure() |
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.
Please add a brief comment explaining what the test is doing.
That assertion would be helpful but I'd need some guidance on how to implement it without compromising performance.. Also looking at the documentation of That saying, adding an assertion that would also affect non-JSPI fibers would break qemu-wasm (for https://zb3.me/qemu-wasm-test/jspi-noffi/ I made a patch, but it's not generic). UPDATE: I have no idea why some unrelated tests are now failing |
This PR is my attempt at supporting fibers using JSPI. My testing shows that they are indeed faster than asyncify (and the wasm file is noticeably smaller), however, there's a major limitation of using JSPI for coroutine support - a suspended coroutine might only be resumed in the same thread (worker), since the actual wasm stack is not serialized into memory. Additionally it's not possible to implement this in pure wasm as this still requires the JS trampoline.
Now let me advertise some self-noticed problems with the PR itself:
continuations
ids are not reused (does the same happen withAsyncify.callStackId
?)rewind_id
structure fieldmakeDynCall
might callpromising
when it was already done (maybegetWasmTableEntry
could have an argument whether an async return is expected?)What do you think about this? JSPI is already supported in chrome and might soon be supported in firefox. However the "stack switching" proposal will probably not be supported anytime soon.. so for me it would make sense to support fibers using JSPI.