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
Restructed async load function #21665
base: main
Are you sure you want to change the base?
Conversation
src/library_browser.js
Outdated
script.onload = () => { | ||
console.log(`Script ${url} loaded successfully.`); | ||
if (onloadPtr) { | ||
var onload = Module['getProcAddress'](onloadPtr, 'v'); |
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 is getProcAddress
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.
The getProcAddress function is expected to obtain the address of a web assembly function and return a callable JavaScript version of it.
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 see. In that case I would suggest using the makeDynCall
macro call: {{{ makeDynCal(...) }}};
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.
Added the macro calls now
2c47529
to
38ebad3
Compare
@sbc100 @tlively I have restructured function in a way the promise is implemented at the top and callbacks are implemented at the base of the promise. For asyncify I can add a handle sleep call but we can use a flag instead. I have run the test case and verified that the added function is creating no test failures. I am getting the following error while running tests on my local: error: undefined symbol: saveSetjmp (referenced by root reference (e.g. compiled C/C++ code)) Could you help me with your valuable feedback on the overall code as well the test failures. |
Taking a step back, can you describe your high-level goal here? I would have expected to see this PR define a new function called something like |
99b995e
to
18c02f7
Compare
I tried to change the original function in the promise -> callbacks and asyncify format but after the feedback, I have introduced a new function implementing the promise flavour of the api and reconstructed the original function aorund the promise version with backward compatibility with callbacks. I was working on resolving the test failures since some time now. I would appreciate your feedback on my fundamental appproach in the latest commit @tlively @sbc100 @kripken |
src/library_browser.js
Outdated
emscripten_async_run_script: (script, millis) => { | ||
// TODO: cache these to avoid generating garbage | ||
safeSetTimeout(() => _emscripten_run_script(script), millis); | ||
}, |
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 these 5 lines got duplicated.
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.
Removed the duplicate code @sbc100
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.
Just want to be sure whether I am heading in the right direction fundamentally for the api unification
18c02f7
to
7c9006c
Compare
src/library_browser.js
Outdated
} | ||
}); | ||
|
||
}, |
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 this approach is to be generalized then perhaps this can be replaced with a helper function. e.g:
emscripten_async_load_script: function(url, onload, onerror) {
promiseToCallbacks(emscripten_async_load_script_promise(url), onload, onerror);
}
However, I think it might be best to have two helpers: promiseToCallbacks
and callbacksToPromise
, and then use the one that makes sense for the underlying API. I suggest we only use promiseToCallbacks
for APIs where the underlying API is already promise based.
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.
defined and used the helper functions @sbc100
src/library_browser.js
Outdated
|
||
emscripten_async_load_script_promise(url).then(() => { | ||
if (onload){ | ||
onload(); |
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 can't call these onload
and onerro
callback backs directly link this I think. These arguments are C pointers (numbers) on not JS function. I think you need to do what the old code did with {{{ makeDynCall('v', 'onload') }}};
.
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 converted calls into the old form
@sbc100 Could you review my pull request and suggest me with your valuable feedback |
Solves #21440