Repository navigation
Add _promise variants for __async library functions - #27880
guybedford wants to merge 1 commit into
Conversation
7ec01b2 to
8ada0ee
Compare
8ada0ee to
1beedc4
Compare
1beedc4 to
7be18b7
Compare
7be18b7 to
fe6933a
Compare
tlively
left a comment
There was a problem hiding this comment.
Instead of building the _promise version of a function on top of the _fd version, I would expect the _promise version to be implemented in terms of emscripten_proxy_promise_with_ctx and the _fd version to be implemented in terms of emscripten_proxy_fd_with_ctx. That does not yet exist, but it would look very much like what you have in system/lib/libc/emscripten_fd_promise.c.
This would avoid the overhead of the FD system for the (hopefully) more common case of using promising calls, and I hope it would simplify the code as well.
da37514 to
33696f3
Compare
33696f3 to
1ce1c14
Compare
1ce1c14 to
3c6d5a2
Compare
@tlively thanks for taking a look, I've added a direct implementation now for the For the For now the new proxy attributes are fully manual - |
|
Can we now perhaps split out the Would, since we considering all the different falvors here, would it be worth also adding an auto-generated |
3c6d5a2 to
20682a4
Compare
20682a4 to
0382b64
Compare
Sure, I've split out the fd version from this PR.
A |
0382b64 to
900ac5a
Compare
322e972 to
72b406f
Compare
| deps: symbolDeps, | ||
| asyncFuncs, | ||
| extraLibraryFuncs, | ||
| asyncVariants: Object.keys(symbolDeps).filter(asyncVariantOf), |
There was a problem hiding this comment.
This extra exported metadata is only needed for gen_sig_info.py, IIUC?
I wonder if we can find a way to avoid it? And this avoid changing the JSON API between Python and JS compiler? At least maybe add a comment, maybe a TODO to remove it later?
There was a problem hiding this comment.
Variants have to be in the --symbols-only symbol graph, since that's what wasm-ld receives as the allowed JS imports, so that answer_promise links at all. With automatic expansion, that graph also contained variants of internal system functions (_emval_await_promise, _dlopen_js_promise, emscripten_promise_await_unchecked_promise). None of those have a header, so gen_sig_info needed the metadata to skip them.
Instead of force adding these headers for all async auto cases, I've actually refactored back again to a non-implicit convention in contrast to previous reviews for an __async: ['promise'] explicit variation system. This way, only listed variants now exist - for system libraries each one gets a header declaration and then gen_sig_info handles it normally.
I'm open to reconsidering the auto expansion, but we'd need to have a plan to avoid the header requirements on existing system: 'auto' APIs that we don't want to maintain these variant APIs for.
There was a problem hiding this comment.
Unless you want to just add C headers for all these system variants?
There was a problem hiding this comment.
Indeed _dlopen_js exists in system/lib/libc/emscripten_internal.h already so it think it makes sense to put _dlopen_js_promise alongside it, no? WDYT? I think its kind of nice that all callable JS functions are declared in some header.
There was a problem hiding this comment.
This means we would have _dlopen_js_promise as well as emscripten_dlopen_promise. If we're fine with that we can do that, it's more just a question of whether we want to do this for all auto functions now, or go back to the opt-in system? I haven't fully gone through all the 27 system functions to determine if they all make sense to be defined in the headers yet. Another one that doesn't really make sense is emscripten_promise_await_unchecked_promise - a promise of awaiting a promise.
26a9879 to
9f32d8d
Compare
9f32d8d to
a84a1e3
Compare
`__async` can now be a list of variants, as `__async: ['promise']`, which otherwise behaves as 'auto'. The `foo_promise` variant runs the same body (with `sync` always false), returning an em_promise_t of the result on the calling thread, emitted only where used. From a pthread it is emscripten_proxy_promise_with_ctx of the main-thread body; the result rides in the proxied call's arg.
a84a1e3 to
798d166
Compare
_promise variants for __async library functions
|
@sbc100 I separated out the first layer of this adding the In terms of the driving use cases:
I've included the |
Lets one asynchronous JS library body be called from any stack, by also exposing it as a C function returning a promise of its result.
__asynccan now also be a list of variants, as__async: ['promise'], which otherwise behaves as'auto'. Thepromisevariantfoo_promiseruns the same body (withsyncalways false), returning anem_promise_ton the calling thread, fulfilled with the result (NULLfor avoidfunction) or rejected withNULL. It is emitted only where used. Since variants are listed explicitly, only those C symbols exist: for system libraries each one gets a header declaration, andgen_sig_infohandles it like any other function. From a pthread it isemscripten_proxy_promise_with_ctxof the main-thread body, with the result carried in the proxied call'sargand athenon the completion promise handing it over; the pthread-side wrapper is a plain call returning the handle. This has its own entry point so nothing is linked into pthread builds that don't use it.proxying.his unchanged.Docs are added to the existing "Marking JS library functions as async" section.
Tests:
jslib.test_jslib_async_auto_variantswith pointer-returning andvoidfunctions in plain, ASYNCIFY, JSPI andPROXY_TO_PTHREADbuilds, wasm32 and wasm64, andjslib.test_jslib_async_variants_errorsfor invalid variant lists.Made with AI assistance under my review