Skip to content

Remove hard dependency on Browser from Asyncify #12181

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

Merged
merged 2 commits into from
Sep 13, 2020

Conversation

curiousdannii
Copy link
Contributor

Split out from #12165

Remove the hard dependency on Browser from Asyncify. Could save 13KB from a minified build.

@sbc100 sbc100 requested a review from kripken September 12, 2020 12:22
@curiousdannii
Copy link
Contributor Author

curiousdannii commented Sep 12, 2020

I'm not sure if something similar should be done for FS. It doesn't actually declare a dependency, so I guess if Browser doesn't exist for any other reason then there will just be some hanging references, so you better not call createPreloadedFile?

emscripten/src/library_fs.js

Lines 1900 to 1930 in 5fb249c

createPreloadedFile: function(parent, name, url, canRead, canWrite, onload, onerror, dontCreateFile, canOwn, preFinish) {
Browser.init(); // XXX perhaps this method should move onto Browser?
// TODO we should allow people to just pass in a complete filename instead
// of parent and name being that we just join them anyways
var fullname = name ? PATH_FS.resolve(PATH.join2(parent, name)) : parent;
var dep = getUniqueRunDependency('cp ' + fullname); // might have several active requests for the same fullname
function processData(byteArray) {
function finish(byteArray) {
if (preFinish) preFinish();
if (!dontCreateFile) {
FS.createDataFile(parent, name, byteArray, canRead, canWrite, canOwn);
}
if (onload) onload();
removeRunDependency(dep);
}
var handled = false;
Module['preloadPlugins'].forEach(function(plugin) {
if (handled) return;
if (plugin['canHandle'](fullname)) {
plugin['handle'](byteArray, fullname, finish, function() {
if (onerror) onerror();
removeRunDependency(dep);
});
handled = true;
}
});
if (!handled) finish(byteArray);
}
addRunDependency(dep);
if (typeof url == 'string') {
Browser.asyncLoad(url, function(byteArray) {

Edit: actually I think that might be happening in the test_wget test that failed. emscripten_wget doesn't directly use Browser, so I didn't add it to its dependencies list. But it does call FS.createPreloadedFile, which uses Browser as I just showed.

I'm not sure what the best way to organise these dependencies is, probably adding Browser to the FS dependencies would be most consistent at present? However if we do that then I'll want to fix it somehow in the future, because it's a lot of needless code for anyone like me who uses FS but doesn't want or need Browser ;). Or maybe just add the dependency to emscripten_wget?

Edit: Okay, I've added a Browser dependency to emscripten_wget, not perfect, but good enough for now IMO.

Arguably the dependency should be on FS, but this is simpler and means if you use other FS functions Browser won't necessarily have to be included
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

I think this is a good first step.

The planned work to split out the main loop handling could be even better. We could in that case emit different code when emscripten_set_main_loop (the function that shows the main loop is actually used) is called by the user or not, maybe.

@kripken kripken merged commit 14bd2e2 into emscripten-core:master Sep 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants