JSPI and Asyncify #551
JSPI and Asyncify #551
Conversation
calvinrp
force-pushed
the
async-pr-first
branch
from
6abb998 to
34d0232
Compare
| let asyncifyResolved; | ||
| async function asyncifyInstantiate(module, imports) { | ||
| const instance = await instantiateCore(module, imports); | ||
| const memory = instance.exports.memory || (imports && imports.env && imports.env.memory); |
There was a problem hiding this comment.
I think we should be a bit smarter about this memory detection, and instead make the memoryExportName an argument into the asyncifyInstantiate function - we should be able to have the context at the Rust side to know this information?
Also flipping between the imports or the exports for the memory seems a bit too loose - can you explain the cases further? Could we make it a boolean argument from the Rust side whether the import or export memory should be used?
| async function asyncifyInstantiate(module, imports) { | ||
| const instance = await instantiateCore(module, imports); | ||
| const memory = instance.exports.memory || (imports && imports.env && imports.env.memory); | ||
| const realloc = instance.exports.cabi_realloc || instance.exports.cabi_export_realloc; |
There was a problem hiding this comment.
Same as for the memory, this should be an argument.
| const instance = await instantiateCore(module, imports); | ||
| const memory = instance.exports.memory || (imports && imports.env && imports.env.memory); | ||
| const realloc = instance.exports.cabi_realloc || instance.exports.cabi_export_realloc; | ||
| if (instance.exports.asyncify_get_state && memory) { |
There was a problem hiding this comment.
What if we don't have a memory here or asyncify_get_state? Does the fallthrough work, or should we throw an error?
| return instance; | ||
| } | ||
| function asyncifyState() { | ||
| return asyncifyModules[0]?.instance.exports.asyncify_get_state(); |
There was a problem hiding this comment.
Should this function take an asyncify module index?
| return asyncifyModules[0]?.instance.exports.asyncify_get_state(); | ||
| } | ||
| function asyncifyAssertNoneState() { | ||
| let state = asyncifyState(); |
There was a problem hiding this comment.
Should this assert across all asyncify module indices?
| function asyncifyWrapExport(fn) { | ||
| return async (...args) => { | ||
| if (asyncifyModules.length === 0) { | ||
| throw new Error(`none of the Wasm modules were processed with wasm-opt asyncify`); |
There was a problem hiding this comment.
If we didn't have the fall-through path on the asyncify instantiate, would we be able to turn this into a static instead of a runtime error?
| } | ||
| asyncifyAssertNoneState(); | ||
| let result = fn(...args); | ||
| while (asyncifyState() === 1) { |
There was a problem hiding this comment.
It sounds like the logic here is that the first module to be instantiated is the top of the call graph.
What happens if the first module we instantiate is not the top of the asyncify call graph?
| Intrinsic::AsyncifyWrapImport => output.push_str(" | ||
| function asyncifyWrapImport(fn) { | ||
| return (...args) => { | ||
| if (asyncifyState() === 2) { |
There was a problem hiding this comment.
Could this be something like asyncifyState(instantiateIdx) where instantiateIdx is the index of the imported function being executed when sorted by the instantiation order?
| @@ -1930,14 +2051,50 @@ impl<'a> Instantiator<'a, '_> { | |||
| export_name: &String, | |||
| resource_map: &ResourceMap, | |||
| ) { | |||
| let is_async = self.async_exports.contains(&func.name) | |||
There was a problem hiding this comment.
Because function names can clash between components, we should figure out a way of defining async_exports and async_imports so that they can be uniquely addressing what they refer to.
Options:
- Only support the shorthand forms for the outer component export and import interfaces, ie make sure we don't match any internal components, only publicly exposed import and export functions
- Update the import and export functions to include the iterface name / resource name / etc
- Go all in on supporting the world name and version in the async_exports and async_imports definitions
They're effectively just a world ID like any other function address, would be good to make sure we don't write ourselves into a corner where we need to make a breaking change in future.
I think the simple function names can work though, but with the restrictions of (1).
* fix(tests): guest types check Signed-off-by: Victor Adossi <[email protected]> * fix: re-comment check from upstream jco Signed-off-by: Victor Adossi <[email protected]> --------- Signed-off-by: Victor Adossi <[email protected]>
Signed-off-by: Victor Adossi <[email protected]>
Signed-off-by: Victor Adossi <[email protected]>
calvinrp
force-pushed
the
async-pr-first
branch
from
1e2b46e to
4027d11
Compare
Aiming for a relatively minimal diff by separating the WASI P2 shims into a separate PR. More PRs to follow with additional fixes and tests.