Skip to content
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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Add Asyncify support #107

wants to merge 12 commits into from

Conversation

yonihemi
Copy link
Member

@yonihemi yonihemi commented Nov 5, 2020

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

  • This adds a bit of complexity on the JS runtime side, especially the need to provide a 'restart' method for re-entering module execution. Should not affect existing usage as all additions are optional.
  • At Swift build time, there is no way of knowing if module will be asyncified so no way to block usage of new methods. The Asyncify pass is extremely slow as well, so not a good candidate to add e.g. to carton's build process.

@yonihemi yonihemi added the enhancement New feature or request label Nov 5, 2020
@swiftwasm swiftwasm deleted a comment from MaxDesiatov Nov 5, 2020
@MaxDesiatov
Copy link
Contributor

Thanks for this work, and I appreciate you mentioned the concerns. I'm not sure how it would compare to the async/await support that's coming in Swift 5.4 likely in spring 2021. But we'd definitely want to support async/await together with the structured concurrency proposal in SwiftWasm in general and JavaScriptKit specifically.

It would make sense to have some utility async function on the JSPromise class that allows Swift code to await for a completion. At least in my current understanding that's not very hard to implement. I'd like to start experimenting with this as soon it's available upstream and we have that merged in, hopefully in the next few months.

Is it worth investing in asyncify in addition to tha native async/await support in Swift? What kind of use cases do you see for asyncify at the moment?

Runtime/src/index.ts Outdated Show resolved Hide resolved
Comment on lines 632 to 653
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);
},
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

@yonihemi yonihemi Jan 8, 2021

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?

  1. Leave as is. Cons: Extra C function, extra JS function.
  2. Have Swift construct the delay promise on first request with JSPromise-JSTimer code, cache it at JSObject.global for future use, then call it. Cons: Swift code will be ugly and less efficient, polluting global namespace.
  3. Remove it from the Swift API and let users define their global JS promise if they want. Cons: While sleep is a special case of wait, 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?

@yonihemi
Copy link
Member Author

yonihemi commented Nov 6, 2020

Thanks for reviewing this!
Async/await is exciting but its implementation in SwiftWasm might not be related or solve the exact same issue this is solving.

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.
Focusing on the url(forResource..) and Data(contentsOf:) calls, they are both synchronous, but if we wanted to fulfill them from a JS host to Wasm we'd have to use async JS methods. Without asyncify there is no way for us to do that (the problem of having the Wasm module pause while still processing incoming events). Also applies as well to imported C code. (Side note: 'Asyncify' might not be the best name to a framework that does the exact opposite).

A year from today, will there be 100% coverage for all methods with new async counterparts and wide adoption in the industry? Maybe. For this example, any code that relies on dogs will have to change to async as well, probably down the entire call tree of any app, so I don't see that happening so soon.

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.

@MaxDesiatov
Copy link
Contributor

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 async/await.

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 yield_to_host() function, which would be equivalent to sleep() in the basic Asyncify example, but making a plugin instance sleep indefinitely until the plugin host wakes it up at some later point.

Do we currently have anything that blocks this PR? I think after this is merged, we could add --asyncify flag or --optimize asyncify option to carton.

@yonihemi
Copy link
Member Author

yonihemi commented Jan 7, 2021

There is a bug with processing incoming events while suspended. Let me push a fix, though not before tomorrow.
Even so, I'm not sure about merging this. I have some thoughts on the position of JSKit/HostKit on a lower level and as an augmentation to WASI. I'll try and jot it down more coherently and we can discuss.

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 yonihemi marked this pull request as ready for review January 20, 2021 13:32
@kateinoigakukun
Copy link
Member

@yonihemi Could you add some test cases to ensure that asyncify works fine?

@yonihemi yonihemi marked this pull request as draft January 30, 2021 14:37
@yonihemi
Copy link
Member Author

Made a working sample.

@yonihemi yonihemi marked this pull request as ready for review January 31, 2021 07:12
@MaxDesiatov
Copy link
Contributor

@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?

@kateinoigakukun
Copy link
Member

I'm sorry to have kept you waiting. At least, we can't merge this feature without more test suites within this repository.
So tagging a minor version without this PR would be reasonable at this time.

@yonihemi
Copy link
Member Author

yonihemi commented Apr 28, 2021

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 think best for now would be to close this PR. I'll of course keep my branch alive as I'm using it.
In future, if someone else comes up with the need for sync C / Swift we can pick it up.

@MaxDesiatov
Copy link
Contributor

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?

@kateinoigakukun
Copy link
Member

@yonihemi @MaxDesiatov

I concern about these two things:

  1. Low test coverage

We need to test the feature more because there are many things to be considered:

  • What happens when asyncify buffer is overflow
  • What happens when exported Swift function is called while sieeping
  • etc...
  1. async / await covers almost the same use cases as Asyncify (in my understanding)

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.
But unlike C, it is very easy to make a synchronous function an asynchronous function in Swift by just putting async keyword, so even in such a use case, we can solve it by making the API on the Swift side asynchronous.
In addition, providing features with similar use cases at the same time can be confusing to users. So, instead of implementing it directly in JSKit, I think it's better to make the Asyncify things an optional feature to avoid confusion. Explicit importing or opt-in operation can imply to users that they need to prepare with wasm-opt.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants