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 context creation #272

Closed
huningxin opened this issue Jun 9, 2022 · 9 comments · Fixed by #274
Closed

Support asynchronous context creation #272

huningxin opened this issue Jun 9, 2022 · 9 comments · Fixed by #274

Comments

@huningxin
Copy link
Contributor

The context creation might be time consuming. For example, the implementation (e.g., Chromium) may use inter-process-communication (IPC) to query hardware capabilities and allocate hardware resources in another process. The synchronous call of current ml.createContext may block the main thread and impact the responsiveness of the user interface.

The proposal is to introduce an async version of createContext and restrict the sync version to be only used by worker thread. This also aligns with the discussions of #229 and #230.

The model loader API has an async version of createContext. It would be better to have a consistent naming for sync and async methods between the two WebML APIs. There are two options:

option async sync
async postfix createContextAsync createContext
sync postfix createContext createContextSync

There is a corresponding issue webmachinelearning/model-loader#38 in model loader repo.

/cc @yuhonglin

@huningxin
Copy link
Contributor Author

@huningxin huningxin added the cr label Jun 13, 2022
@huningxin
Copy link
Contributor Author

Label this issue as "cr" blocker because it blocks #229 and #230 which are labeled "cr".

@huningxin
Copy link
Contributor Author

For option "sync postfix", we also need to make the naming of compute and build consistent.

method async sync
create context ML.createContext ML.createContextSync
build graph MLGraphBuilder.build MLGraphBuilder.buildSync
compute graph MLContext.compute MLContext.computeSync

In particular, @wacky6 proposed to change the default build as async and buildSync for sync version.

huningxin added a commit to huningxin/webnn that referenced this issue Aug 28, 2022
This PR also restricts the sync version of create context to worker.

Fix webmachinelearning#272
@domenic
Copy link

domenic commented Aug 30, 2022

I was asked to chime in here with some background about how other web platform APIs have approached this.

In general, the preferred approach is to not have sync APIs like this at all. They are an antipattern on the web platform, and one we've moved away from. This includes in a worker-only context; creating worker-only sync APIs bifurcates the platform and makes it easy for developers to write libraries which are not available in some contexts. It also blocks the worker thread, which can cause its own problems in some cases.

This does make using such APIs more work from WebAssembly. There is some hope that in the future this will get easier via the JS promise integration proposal. In the meantime, WebAssembly developers are encouraged to use technologies such as asyncify. Asyncify does have a performance cost, however.

If your API (a) really needs to ship before the JS promise integration proposal; and (b) is used repeatedly in a tight loop in core application logic, so that the performance cost of asyncify will basically make the API unusable: then, you might be able to go for an exception, and ship a worker-only sync API. This was done recently for the File System Access Handle API, which has a createSyncAccessHandle() method. The returned SyncAccessHandle instance has sync methods, instead of the normal AccessHandle's async ones. However, this is generally thought of as regrettable, so try to avoid this path if you can.

(Note, however, that createSyncAccessHandle() itself is async! This is because that method doesn't meet criterion (b); only the actual reads/writes, i.e. syncAccessHandle.read()/write() methods, meet criterion (b).)

If you do go the worker-only sync API route, as for naming, there are not many strong precedents here. createSyncAccessHandle() is one; the old, Chrome-only requestFileSystemSync() and its various pieces is another. (In doing research I also found this 2013 blog post arguing against the idea of sync-only worker APIs!) On the other end, there's Atomics.waitAsync(). My slight preference would be to distinguish the sync API as the unusual one, so follow the x() + xSync() pattern.

@anssiko
Copy link
Member

anssiko commented Aug 30, 2022

Thank you @domenic. This new background information is very helpful for the WG and was delivered just in time. Those of you who don't yet know @domenic: he wears many hats such as ex-TAG and TC39 to name a few and would be my go-to person for this type of questions.

Reading @domenic's comment and reflecting on it, I'm hearing a slight preference to x() + xSync() naming. Sync APIs are considered an anti-pattern (and as such at risk of deprecation in the future) and thus shall be "earmarked" as unusual (vendor prefixed APIs come to my mind here as an analogy).

In the (perfect) future we'd have all APIs async and thus there's no need to emphasize this default in naming, and should strive for conciseness in naming instead. This design is staged in PR #274.

The another approach discussed in the WG is to align with the WebGU API's emerging naming convention xAsync() + x():

  • createRenderPipelineAsync() / createRenderPipeline()
  • createComputePipelineAsync() / createComputePipeline()
  • mapAsync() / (no sync counterpart)

This design is staged in PR #285.

To close, let me quote Joshua Bloch just in case this provides some food for thought to someone watching:

If it’s hard to find good names, go back to the drawing board. Don’t be afraid to split or merge an API, or embed it in a more general setting. If names start falling into place, you’re on the right track.
Names matter. Strive for intelligibility, consistency, and symmetry. Every API is a little language, and people must learn to read and write it. If you get an API right, code will read like prose.

All - let's continue this design discussion in this issue instead of the two PRs to not fork our discussion.

@huningxin
Copy link
Contributor Author

  • mapAsync() / (no sync counterpart)

The sync counterpart mapSync() is being discussed in gpuweb/gpuweb#2217.

@wacky6
Copy link

wacky6 commented Aug 31, 2022

+1 on @domenic's comment.

@anssiko
Copy link
Member

anssiko commented Sep 9, 2022

@zolkis
Copy link
Collaborator

zolkis commented Oct 19, 2022

I am scanning issues on topics of current interest (among which, in this case, context), and found this.

I was about to ask why don't we use async API by default, and check whether resolving a promise immediately is good enough approximation for sync use cases. Then I've read Domenic's comment and realized WASM is the reason, and it's a valid use case to be able to specify fully synchronous behavior.

Anyway, since APIs stay, a good practice IMHO would be to use async API wherever possible, and postfix the exceptions, i.e. use the Sync postfix (as exception), rather than the Async postfix (which should be the default).

As I studied the API structure, I was wondering if we can do some simplification about context creation, since the algorithms (not necessarily the implementation) would become simpler, and might solve this, too. I create a separate issue.

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