-
-
Notifications
You must be signed in to change notification settings - Fork 45
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
Add Asyncify support #107
base: main
Are you sure you want to change the base?
Add Asyncify support #107
Conversation
Thanks for this work, and I appreciate you mentioned the concerns. I'm not sure how it would compare to the It would make sense to have some utility Is it worth investing in |
Runtime/src/index.ts
Outdated
swjs_sleep: (ms: number) => { | ||
syncAwait(delay(ms)); | ||
}, | ||
swjs_sync_await: ( | ||
promiseRef: ref, | ||
kind_ptr: pointer, | ||
payload1_ptr: pointer, | ||
payload2_ptr: pointer | ||
) => { | ||
const promise: Promise<any> = this.heap.referenceHeap(promiseRef); | ||
syncAwait(promise, kind_ptr, payload1_ptr, payload2_ptr); | ||
}, | ||
swjs_sync_await_with_timeout: ( | ||
promiseRef: ref, | ||
timeout: number, | ||
kind_ptr: pointer, | ||
payload1_ptr: pointer, | ||
payload2_ptr: pointer | ||
) => { | ||
const promise: Promise<any> = this.heap.referenceHeap(promiseRef); | ||
syncAwait(promiseWithTimout(promise, timeout), kind_ptr, payload1_ptr, payload2_ptr); | ||
}, |
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 expose only swjs_sync_await
to minimize runtime functions? I think others can be implemented on Swift side.
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.
await_with_timeout definitely can be removed and easily implemented by callers. Not sure about the sleep one, as setTimout
doesn't return a proper Promise implementing that from Swift side would be kind of ugly, and sleep is the most useful for imported c code.
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've removed the withTimeout function. @kateinoigakukun What do you think would be best regarding sleep?
- Leave as is. Cons: Extra C function, extra JS function.
- Have Swift construct the
delay
promise on first request with JSPromise-JSTimer code, cache it atJSObject.global
for future use, then call it. Cons: Swift code will be ugly and less efficient, polluting global namespace. - Remove it from the Swift API and let users define their global JS promise if they want. Cons: While
sleep
is a special case ofwait
, it is arguably more useful and a surprise to users when they can't find it. And while adding the global JS function is trivial if you know about it, connecting these dots cold be difficult to newcomers. Plus, I hope actual C code can use it directly.
Or any other ideas?
Thanks for reviewing this! Let's take this code snippet for example: public var dogs: [Dog] {
if let dogs = _cachedDogs { return dogs }
guard
let url = Bundle.module.url(forResource: "dogs", withExtension: "json"),
let data = try? Data(contentsOf: url),
let dogs = try? JSONDecoder().decode([Dog].self, from: data) else {
fatalError()
}
_cachedDogs = dogs
return dogs
} One can argue it has some glaring mistakes but no doubt similar code appears in many code bases. A year from today, will there be 100% coverage for all methods with new I expect the async/await actual implementation for SwiftWasm to be more straightforward than this - matching async expectation with async external implementations (vs. this fix - matching sync expectations with external async implementations), and in any case not interfering with that implementation. I wish there was an easy way to have this as an entirely different package or as a plugin of some sort, but this other package would have to reimplement most of JavaScriptKit, or we'd have to expose a bunch of internal implementation details, which would not be great either. |
I had another look at this and actually think it would be a really useful addition after all. Firstly, it would potentially allow us writing async tests for DOM so that they look synchronously and work with standard XCTest without waiting (pardon the pun) until it supports Secondly, I'm thinking about apps that could utilize Wasm for plugins written in any language. These plugins could be long running or resource intensive, or both. At the same time, it would be useful to allow loading a lot of such plugins, potentially hundreds of them, but allowing them to run concurrently without blocking each other. Creating a dedicated OS thread for every plugin instance is impractical, that's where Asyncify comes into play. A plugin host could provide its own Do we currently have anything that blocks this PR? I think after this is merged, we could add |
There is a bug with processing incoming events while suspended. Let me push a fix, though not before tomorrow. Meanwhile about the long build times - my hypothesis is we might be able to leverage asyncify-imports (aka asyncify+list), limited to 1 method as @kateinoigakukun suggested, in a prebuilt Foundation+JSKit module, then ChibiLink it with a (fast) build of a project. Haven't made much progress and still not convinced it's doable. |
@yonihemi Could you add some test cases to ensure that asyncify works fine? |
Made a working sample. |
@kateinoigakukun what are your thoughts on this? I'm thinking of tagging a new version, so I'm wondering if that should be a minor 0.10.1 without this change, or a major-ish 0.11.0 with this included if there's any chance for it to be reviewed and merged soon? |
I'm sorry to have kept you waiting. At least, we can't merge this feature without more test suites within this repository. |
Looks like I haven't managed to present the case for Asyncify successfully. Not surprising, as this was the fate of similar patches in most projects except Emscripten itself. It's a tough sell. |
I hope there's a way to keep this open and eventually get merged. @kateinoigakukun is the amount of test coverage your only concern here? |
I concern about these two things:
We need to test the feature more because there are many things to be considered:
If the Wasm side wants a synchronous API, but the actual JS implementation requires asynchronous operations, asyncify is useful and async/await cannot be used in that case. |
What this adds
Calls out to modules with
wasm-opt
's--asyncify
pass to unwind and rewind the module's call stack when we want to put Swift execution to sleep while allowing incoming events to be handled by JavaScript.Further Reading
Alon Zakai's Blog
Alon Zakai's Talk
Binaryen Docs
Experimental JS wrapper from Google
Concerns