-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix late-binding symbols with JSPI #23619
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
55ff7e0
to
33707ea
Compare
Late-binding symbols get a JS stub import that resolves the symbol and then makes an onward call. This breaks JSPI. (It also canonicalizes NaNs by round tripping them through JS). To fix this, we have to walk the import table and make wasm module stubs for each of the missing function-type symbols. These stubs will call a JS function to resolve the symbol but then insert it into a table and return control back to the wasm stub which has to make the onward call. This currently relies on --experimental-wasm-type-reflection. To avoid that, we need to parse the import table of the wasm binary manually. Also, without wasm-type-reflection, we have no way to handle the case where someone calls `loadWebAssemblyModule()` on a WebAssembly module instead of a Uint8Array.
33707ea
to
ed49619
Compare
Why do you say this? Is that simply because wasm type reflection is not standardized yet? Presumably if it was standardized this would be good tool to solve this problem? |
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.
Oh wow, we really have to jump through a bunch of hoops to avoid JS thunks huh?
Yes in particular my concern is that I expect jspi to become available without a feature flag before wasm-type-reflection so if we depend on it there will be engines that otherwise would be able to use our jspi but can't because they haven't yet shipped type reflection.
Absolutely it's the right tool. |
@sbc100 do you have any specific feedback on this PR? |
It feel like a lot of complexity to be adding. Perhaps we can find way to reduce it somehow? I need to take a deeper look at some point. |
Unfortunately, I don't think we can do anything much simpler than this unless we change the shared object files. |
@sbc100 would you mind looking into this when you have a chance? It would be nice to make dynamic linking work correctly with JSPI. |
This is an automatic change generated by tools/maint/rebaseline_tests.py. The following (2) test expectation files were updated by running the tests with `--rebaseline`: ``` other/codesize/test_codesize_hello_dylink.gzsize: 5860 => 5869 [+9 bytes / +0.15%] other/codesize/test_codesize_hello_dylink.jssize: 12839 => 12877 [+38 bytes / +0.30%] Average change: +0.22% (+0.15% - +0.30%) ```
@sbc100 I think this is ready for another round of review when you have a chance. |
@sbc100 would appreciate review on this one as well. |
// (import "e" "r" (func $resolve)) | ||
// (global $isResolved (mut i32) i32.const 0) | ||
// | ||
// (func (export "o") (param $p1 i32) (result i32) |
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.
Can we use more meaningful names here maybe? What is p1
, for example?
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.
Well this is the argument to the function that we are going to resolve. We're going to pass it onward. I'm not sure what we call wrapper arguments like this -- in the invoke_
functions, the arguments go unnamed.
@sbc100 this is ready for another round of review |
When discussing this change with @kripken he suggested that we could use an alternative approach which used a little extra machinary in the JS wrapper to avoid the need the wasm code generation. He can probably explain better than me. I'm not sure if this alternative approach would avoid the wasm type reflection dependency? |
I wonder if another approach here could be to "forward" the Promise through JS? That is, if we have a call stack with JS in the middle,
The VM can't suspend the JS like it can the wasm. All it can do is suspend the wasm on top, which then returns a Promise to the JS in the middle. But that JS can await that Promise, leading to the wasm on the bottom being suspended. So we don't have a single wasm suspension, but two of them, with JS "glueing" them together. For this to work, the middle JS would need to add the JSPI wrapper stuff for suspending, on both sides. This might not be that simple, I'm really not sure, but maybe it's another option to consider here? |
When I tried that approach previously I found the performance was unsatisfactory. But there is a decent chance that v8 has improved its stack switching machinery since then. I'll try implementing it that way and we can see if it sucks less than this. |
I think even if the perf isn't great it might be better to have something that works without depending on type reflection (and therefore works without flags). |
Well I have a way to make this work without type reflection by just directly parsing out the imports section of the binary. It just introduces more complexity so I was planning to implement that in a followup. |
So the alternative approach could look something like: if (!(prop in stubs)) {
var resolved;
var func = (...args) => {
resolved ||= resolveSymbol(prop);
try {
resolved = WebAssembly.promising(resolved);
} catch(e) {}
return resolved(...args);
}
stubs[prop] = new WebAssembly.Suspending(func);
} |
The other idea we talked about was doing something similar to wasm-split in binaryen. It would work roughly like this with two modules
After the load of |
Late-binding symbols get a JS stub import that resolves the symbol and then makes an onward call. This breaks JSPI. (It also canonicalizes NaNs by round tripping them through JS).
To fix this, we have to walk the import table and make wasm module stubs for each of the missing function-type symbols. These stubs will call a JS function to resolve the symbol but then insert it into a table and return control back to the wasm stub which has to make the onward call.
This currently relies on --experimental-wasm-type-reflection. To avoid that, we will need to parse the import table of the wasm binary manually. Also, without wasm-type-reflection, we have no way to handle the case where someone calls
loadWebAssemblyModule()
on a WebAssembly module instead of a Uint8Array.This half way fixes #23395. I say half way because to be truly fixed the solution shouldn't rely on wasm type reflection. See also #23600.