Restructed async load function #21665
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.
What is getProcAddress here?
There was a problem hiding this comment.
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.
I see. In that case I would suggest using the makeDynCall macro call: {{{ makeDynCal(...) }}};
impact-maker
force-pushed
the
asyncloadbranch
branch
from
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 |
impact-maker
force-pushed
the
asyncloadbranch
branch
2 times, most recently
from
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.
It looks like these 5 lines got duplicated.
There was a problem hiding this comment.
Removed the duplicate code @sbc100
There was a problem hiding this comment.
Just want to be sure whether I am heading in the right direction fundamentally for the api unification
impact-maker
force-pushed
the
asyncloadbranch
branch
from
18c02f7
to
7c9006c
Compare
src/library_browser.js
Outdated
| } | ||
| }); | ||
|
|
||
| }, |
There was a problem hiding this comment.
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.
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.
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.
@sbc100 converted calls into the old form
|
@sbc100 Could you review my pull request and suggest me with your valuable feedback |
Solves #21440