Closed huningxin closed 2 years ago
The related discussion in Chromium CL review: https://chromium-review.googlesource.com/c/chromium/src/+/3684745/7/third_party/blink/renderer/modules/ml/ml.idl#16
Label this issue as "cr" blocker because it blocks #229 and #230 which are labeled "cr".
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.
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.
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.
mapAsync()
/ (no sync counterpart)
The sync counterpart mapSync()
is being discussed in https://github.com/gpuweb/gpuweb/issues/2217.
+1 on @domenic's comment.
Per https://www.w3.org/2022/09/08-webmachinelearning-minutes.html#r01 TAG recommendation requested https://github.com/w3ctag/design-reviews/issues/771
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.
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:createContextAsync
createContext
createContext
createContextSync
There is a corresponding issue https://github.com/webmachinelearning/model-loader/issues/38 in model loader repo.
/cc @yuhonglin