-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Restructuring emscripten_dlopen_js #21595
base: main
Are you sure you want to change the base?
Restructuring emscripten_dlopen_js #21595
Conversation
src/library_dylink.js
Outdated
} | ||
|
||
{{{ runtimeKeepalivePush() }}} | ||
runtimeKeepalivePush(); |
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.
I think you need to keep the {{{
and }}}
around these calls.
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.
Emscripten's JS is run though a preprocessing step that gathers all the libraries and resolves various macros before emitting the final JS. The triple curly braces {{{}}}
tell the preprocessor to replace this code fragment with the result of running the code inside the braces at build time.
src/library_dylink.js
Outdated
var promise = dlopenInternal(handle, { loadAsync: true }); | ||
if (promise) { | ||
promise.then(successCallback, errorCallback); | ||
} else { | ||
errorCallback(); | ||
errorCallback(new Error('Promise was not created by dlopenInternal.')); |
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.
Instead of this if/elfse
here I think we can just asset(promise)
here, but that sounds like a separate PR that we can land first.
src/library_dylink.js
Outdated
} | ||
|
||
{{{ runtimeKeepalivePush() }}} | ||
runtimeKeepalivePush(); | ||
var promise = dlopenInternal(handle, { loadAsync: true }); |
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.
So this is an example of API that already has a promise being returned from the underlying API.
Do I think we have two reasonable ways forward to make this more generic:
- Return a
promise_t
back to C/C++ here and let C/C++ handler wrapping this into a callback-based API. - Create a generic JS wrapper function called something like
promiseToCallback(successFnPtr, errorFrPtr)
that takes care of doing all theruntimeKeepalivePop
stuff and thecallUserCallback
stuff.
I think (2) might be the simplest for now as it probably involves a little less complexity since I believe the C/C++ promise API would end up creating an extra promise in order to do the emscripten_promise_then
operation.
@tlively would be OK with (1) here?
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.
Did you mean (2)? Either sounds good to me 👍
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.
@sbc100 @tlively Made the changes according to the feedback provided above
- Created a generic JS wrapper function called something like promiseToCallback(successFnPtr, errorFrPtr) that takes care of doing all the runtimeKeepalivePop stuff and the callUserCallback
- Added {{{ and }}} around calls.
- Added asset(promise) instead of if/elfse
src/library_dylink.js
Outdated
{{{ runtimeKeepalivePop() }}} | ||
callUserCallback(() => {{{ makeDynCall('vpp', 'onerror') }}}(handle, user_data)); | ||
// Utility function to encapsulate promise handling with callbacks | ||
function promiseToCallback(promise, successFnPtr, errorFnPtr, user_data) { |
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.
You need to use the $promiseToCallback: function(...) {
syntax here since this is inside a JS object.
You then need to add $promiseToCallback
to the __deps
for _emscripten_dlopen_js
src/library_dylink.js
Outdated
var filename = UTF8ToString(handle + {{{ C_STRUCTS.dso.name }}}); | ||
dlSetError(`'Could not load dynamic lib: ${filename}\n${e}`); | ||
{{{ runtimeKeepalivePop() }}} |
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.
It looks like the {{{ }}}
are still removed from this. Were you able to run the tests locally? They should be failing because of this problem.
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.
@tlively necessary changes were missed in the previous commit. I have now pushed the latest changes. I have gone through the documentation but still doubtful on what commands and test suites needs to run to confirm for the api restructuring changes.
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.
Looking at CI, it looks one the failing tests is test_pthread_dlopen_many
in the core0
suite. To run just that test locally, you would run test/runner core0.test_pthread_dlopen_many
. To run all the tests in core0
(which means the core tests with optimization level O0), you would run test/runner core0
. You can also use the --verbose
and --save-dir
flags to get more information and to save the files so you can manually run and debug the test yourself.
ff05732
to
b7dbaa9
Compare
@tlively @sbc100 I have done the whole setup for test suite and pushed the code. |
If you add |
435e835
to
09dcf70
Compare
5a25b4b
to
231135e
Compare
ca3932f
to
de9e66b
Compare
_emscripten_dlopen_js: (handle, onsuccess, onerror, user_data) => { | ||
/** @param {Object=} e */ | ||
// Utility function to encapsulate promise handling with callbacks | ||
$promiseToCallback: function(promise, successFnPtr, errorFnPtr, user_data) { |
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.
I don't see this utility being called anywhere
errorCallback(); | ||
} | ||
// Assert that the promise is provided | ||
assert(promise, 'promiseToCallback was called without a valid promise.'); |
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.
Put this line at the top of the function and wrap in #if ASSERTIONS
check
system/lib/libc/dynlink.c
Outdated
|
||
em_promise_t promise = emscripten_dlopen_promise(filename,flags); | ||
emscripten_promise_then(promise, onsuccess, user_data); | ||
emscripten_promise_catch(promise, onerror, user_data); |
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.
This nice, it seems like you have implemented something like promiseToCallback
in native code?
I think it might be fast it if was implemented JS though since using promises from the native does have some amount of overhead I think (just calling back and forth into JS has a cost).
} | ||
do_write_lock(); | ||
char buf[2*NAME_MAX+2]; | ||
filename = resolve_path(buf, filename, sizeof buf); |
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.
What happened to this call to resolve_path?
src/library_dylink.js
Outdated
|
||
{{{ runtimeKeepalivePush() }}} | ||
|
||
var filename = UTF8ToString(handle); |
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.
If the first argument of this function now a filename and not a handle then perhaps it should be renamed.
src/library_dylink.js
Outdated
}, | ||
|
||
_emscripten_dlopen_js__deps: ['$dlopenInternal', '$runtimeKeepalivePush', '$runtimeKeepalivePop', '$dlSetError', '$dynCall'], | ||
_emscripten_dlopen_js: function(handle, promise, onsuccess, onerror,) { |
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.
It seems a little odd to mix passing a promise here with passing callback. I would assume that we would want one or the other and not both.
In fact it looks like this code doesn't actually use the promise argument right now?
Solves #21440
emscripten_dlopen_js
Description:
Promise-Based Approach as the cannonical approach:
Anchoring asynchronous operations within a Promise object, giving a method to manage asynchronous tasks
Callback Support: Maintains backward compatibility with callback functions (onsuccess, onerror)
ASYNCIFY Integration: Bridges synchronous C/C++ code with asynchronous JavaScript operations by compiling the WebAssembly module with ASYNCIFY enabled and specifying asynchronous functions like _emscripten_dlopen_js in ASYNCIFY_IMPORTS