[jspi] Require async js functions when used with __async decorator. #25480
Conversation
The `_emval_await` library function is marked `_emval_await__async: true`, but the js function is not async. With memory64 enabled we auto convert to bigint and look for the async keyword (which is missing) to apply the await before creating the BigInt. With my changes __async will require an async js function, which signals the function is used with JSPI and the appropriate awaits are then inserted.
|
|
||
| import assert from 'node:assert'; | ||
| import * as fs from 'node:fs/promises'; | ||
| import { isAsyncFunction } from 'node:util/types'; |
There was a problem hiding this comment.
Oh.. interesting. TIL about this.
| }); | ||
| }, | ||
| #endif | ||
| #if ASYNCIFY == 2 |
There was a problem hiding this comment.
Can this just be #else Since it can only be either 1 or 2 right? Or elif would work too.
| _emval_await: async (promise) => { | ||
| var value = await Emval.toValue(promise); | ||
| return Emval.toHandle(value); | ||
| }, |
There was a problem hiding this comment.
I don't really get why this functions needs to looks so different in ASYNCIFY 1 vs 2.
In other places in this PR we have async (...) => Asyncify.handleSleep(..... but not here?
There was a problem hiding this comment.
Unfortunately, handleAsync currently works slightly differently on old asyncify and JSPI. On JSPI, it returns the promise and on asyncify it pipes the return value magically through the wakeup. Adding the async keyword here for asyncify=1 causes the test to fail.
I'd really like to unify all this code and have it so if you mark a library function with __async you don't have to use handleSleep or handleAsync, but that's a bigger change.
| SDL_Delay__deps: ['emscripten_sleep'], | ||
| SDL_Delay__async: true, | ||
| SDL_Delay: (delay) => _emscripten_sleep(delay), | ||
| SDL_Delay: async (delay) => _emscripten_sleep(delay), |
There was a problem hiding this comment.
I kind of which #if ASYNCIFY case came first here, but that unrelated to this PR
| 'dyncall': [['-sALLOW_MEMORY_GROWTH', '-sMAXIMUM_MEMORY=4GB']], | ||
| 'wasm64': (['-sMEMORY64'],), | ||
| }) | ||
| @requires_jspi |
There was a problem hiding this comment.
How about also_with_wasm64 so that we get all 4 cases in wasm64 mode?
| } | ||
| ''') | ||
| err = self.expect_fail([EMCC, 'main.c', '-o', 'out.js', '-sJSPI', '--js-library=lib.js', '-Wno-experimental',]) | ||
| self.assertContained('error: foo is marked with the __async decorator but is not an async JS function.', err) |
There was a problem hiding this comment.
Maybe move this function alongside all the other test_jslib_xx functions? Or maybe its more suited to being grouped with JSPI? Not sure.
| emscripten_sleep__deps: ['$safeSetTimeout'], | ||
| emscripten_sleep__async: true, | ||
| emscripten_sleep: (ms) => Asyncify.handleSleep((wakeUp) => safeSetTimeout(wakeUp, ms)), | ||
| emscripten_sleep: async (ms) => Asyncify.handleSleep((wakeUp) => safeSetTimeout(wakeUp, ms)), |
There was a problem hiding this comment.
What does flagging this async gain here? (it costs code size)
I understand the rationale for async is that one can await for the return value, but for functions that return void, isn't the async keyword dead code?
Though Asyncify and JSPI build modes itself do not need the functions to be flagged async do they? (in WebGPU JSPI support I went with this)
There was a problem hiding this comment.
It really doesn't add anything here, but consistency. We have a few spots where we auto modify the library js (e.g. memory 64). It's much easier in those wrapper functions to assume we're in a async function and emit await code (also the await code is usually shorter).
The
_emval_awaitlibrary function is marked_emval_await__async: true, but the js function is not async. With memory64 enabled we auto convert to bigint and look for the async keyword (which is missing) to apply the await before creating the BigInt. With my changes __async will require an async js function, which signals the function is used with JSPI and the appropriate awaits are then inserted.Fixes #25468