-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[jspi] Require async js functions when used with __async decorator. #25480
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -476,11 +476,11 @@ addToLibrary({ | |
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does flagging this I understand the rationale for 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
emscripten_wget_data__deps: ['$asyncLoad', 'malloc'], | ||
emscripten_wget_data__async: true, | ||
emscripten_wget_data: (url, pbuffer, pnum, perror) => Asyncify.handleAsync(async () => { | ||
emscripten_wget_data: async (url, pbuffer, pnum, perror) => Asyncify.handleAsync(async () => { | ||
/* no need for run dependency, this is async but will not do any prepare etc. step */ | ||
try { | ||
const byteArray = await asyncLoad(UTF8ToString(url)); | ||
|
@@ -497,7 +497,7 @@ addToLibrary({ | |
|
||
emscripten_scan_registers__deps: ['$safeSetTimeout'], | ||
emscripten_scan_registers__async: true, | ||
emscripten_scan_registers: (func) => { | ||
emscripten_scan_registers: async (func) => { | ||
return Asyncify.handleSleep((wakeUp) => { | ||
// We must first unwind, so things are spilled to the stack. Then while | ||
// we are pausing we do the actual scan. After that we can resume. Note | ||
|
@@ -585,7 +585,7 @@ addToLibrary({ | |
|
||
emscripten_fiber_swap__deps: ["$Asyncify", "$Fibers", '$stackSave'], | ||
emscripten_fiber_swap__async: true, | ||
emscripten_fiber_swap: (oldFiber, newFiber) => { | ||
emscripten_fiber_swap: async (oldFiber, newFiber) => { | ||
if (ABORT) return; | ||
#if ASYNCIFY_DEBUG | ||
dbg('ASYNCIFY/FIBER: swap', oldFiber, '->', newFiber, 'state:', Asyncify.state); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -402,12 +402,19 @@ ${functionBody} | |
#if ASYNCIFY | ||
_emval_await__deps: ['$Emval', '$Asyncify'], | ||
_emval_await__async: true, | ||
#if ASYNCIFY == 1 | ||
_emval_await: (promise) => { | ||
return Asyncify.handleAsync(async () => { | ||
var value = await Emval.toValue(promise); | ||
return Emval.toHandle(value); | ||
}); | ||
}, | ||
#else | ||
_emval_await: async (promise) => { | ||
var value = await Emval.toValue(promise); | ||
return Emval.toHandle(value); | ||
}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately, I'd really like to unify all this code and have it so if you mark a library function with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGTM |
||
#endif | ||
#endif | ||
|
||
_emval_iter_begin__deps: ['$Emval'], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3242,10 +3242,11 @@ def test_embind_asyncify(self): | |
''') | ||
self.do_runf('main.cpp', 'done', cflags=['-lembind', '-sASYNCIFY', '--post-js', 'post.js']) | ||
|
||
@also_with_wasm64 | ||
@parameterized({ | ||
'': [['-sDYNAMIC_EXECUTION=1']], | ||
'no_dynamic': [['-sDYNAMIC_EXECUTION=0']], | ||
'dyncall': [['-sALLOW_MEMORY_GROWTH', '-sMAXIMUM_MEMORY=4GB']], | ||
'': (['-sDYNAMIC_EXECUTION=1'],), | ||
'no_dynamic': (['-sDYNAMIC_EXECUTION=0'],), | ||
'dyncall': (['-sALLOW_MEMORY_GROWTH', '-sMAXIMUM_MEMORY=4GB'],), | ||
}) | ||
@requires_jspi | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about |
||
def test_embind_jspi(self, args): | ||
|
@@ -3484,6 +3485,24 @@ def test_jspi_async_function(self): | |
'-Wno-experimental', | ||
'--post-js=post.js']) | ||
|
||
@requires_jspi | ||
def test_jspi_bad_library_function(self): | ||
create_file('lib.js', r''' | ||
addToLibrary({ | ||
foo__async: true, | ||
foo: function(f) {}, | ||
}); | ||
''') | ||
create_file('main.c', r''' | ||
#include <emscripten.h> | ||
extern void foo(); | ||
EMSCRIPTEN_KEEPALIVE void test() { | ||
foo(); | ||
} | ||
''') | ||
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) | ||
|
||
@requires_dev_dependency('typescript') | ||
@parameterized({ | ||
'commonjs': [['-sMODULARIZE'], ['--module', 'commonjs', '--moduleResolution', 'node']], | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh.. interesting. TIL about this.