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

Support asynchronous graph compilation #263

Closed
wchao1115 opened this issue Apr 29, 2022 · 5 comments · Fixed by #266
Closed

Support asynchronous graph compilation #263

wchao1115 opened this issue Apr 29, 2022 · 5 comments · Fixed by #266

Comments

@wchao1115
Copy link
Collaborator

Feedback from @wacky6 and @huningxin. Detail is in PR #257.

@anssiko
Copy link
Member

anssiko commented May 3, 2022

Reproducing the discussion from #257 here for the record:

@wacky6 #257 (comment)

Should we change build() to return Promise? Having the API default to async gives us lots of flexibility in the future.

Also, it's easy to write async calls in synchronous style with JS async-await. Converting sync / blocking calls to async / non-blocking calls is much harder.

@huningxin #257 (comment)

Thanks @wacky6

Also, it's easy to write async calls in synchronous style with JS async-await.

It's true.

However, it is hard or inefficient for a C++ inference code of Wasm-based ML framework to integrate. That C++ inference code usually expects a sync method call. The sync methods (build/compute) are designed for that use case.

My suggestion is to keep the sync build method but restrict it in worker scope and introduce the async buildAsync for main thread / JS usage.

@wacky6 #257 (comment)

Thanks for the explanation!

I think there're libraries to wrap async Web API calls in JavaScript. Like: https://web.dev/asyncify/ .

Are there any constraints limiting such libraries wrapping async WebNN methods to WASM sync method calls?

For Web APIs in general, I don't think "Async" suffic to JavaScript API is used elsewhere. Async functions are first-class citizens in JS, and most web developers expect async APIs nowadays.

If buildAsync() can be trivially converted to buildSync in WASM, I think just providing the Promise build() method is better in terms of simplicity and expandability.

@wchao1115 #257 (comment)

Thanks @wacky6 and @huningxin for the feedback. I've created issue #263 to track this request separately.

@huningxin
Copy link
Contributor

huningxin commented May 5, 2022

@wacky6

I think there're libraries to wrap async Web API calls in JavaScript. Like: https://web.dev/asyncify/ .

Are there any constraints limiting such libraries wrapping async WebNN methods to WASM sync method calls?

To my experience, asyncify injects quite a bit supporting code to wrap the async call that would introduce non-trivial cost and impact the performance.

In today's WG call, @RafaelCintron also mentioned

In terms of the WebGPU example - the feedback we've gotten from game engine developers is that mapAsync wouldn't work for them when porting native code
… async poses problems and asyncify doesn't address them

Regarding to the "Async" suffix, there are other APIs follow this convention, for example WebGPU:

GPUBuffer.mapAsync
GPUDevice.createComputePipelineAsync

@dontcallmedom may have more insights on the naming.

@wacky6
Copy link

wacky6 commented May 5, 2022

Thanks for the explanation.

I think having a sync version of the method is justified considering the performance overhead and easier interoperability with C++ code. It's a good starting point for WebNN implementation.

I feel the async version can lower the barrier to adoption (developers won't need workers). Perhaps we can (in the spec), expose build() to frames, but make it very clear that this blocks main thread (and save a round-trip to workers). WDYT?

@huningxin
Copy link
Contributor

Thanks @wacky6

I feel the async version can lower the barrier to adoption (developers won't need workers).

+1. I'll make a PR to add the async build method.

Perhaps we can (in the spec), expose build() to frames, but make it very clear that this blocks main thread (and save a round-trip to workers).

I agree adding such kind of notice would be helpful.

However, according to the feedback of #229, only exposing sync APIs in worker would protect the user from main-thread jank via developers using such APIs. Do you think the WG should revisit that issue?

@wacky6
Copy link

wacky6 commented May 6, 2022

However, according to the feedback of #229, only exposing sync APIs in worker would protect the user from main-thread jank via developers using such APIs. Do you think the WG should revisit that issue?

I agree limiting sync API to only workers, if there's an async alternative in window (e.g. buildAsync). Otherwise, we are just limiting API's usage (esp. during experimentation) and forcing developers to use workers.

Personally, I think the user experience is the responsibility of application developers. API and spec authors should provide the tools for developers to deliver such experience. Developers should make the final call on whether to do X in where. If a developer really want to block the main thread, I don't think spec authors should try to stop that.

Of course, we should warn developers about potential caveats (e.g. this method could take a long time and block the main thread).

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

Successfully merging a pull request may close this issue.

4 participants