Skip to content

Support --preload-file in the shell environment and d8 #15337

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

Closed
wants to merge 3 commits into from
Closed

Conversation

kripken
Copy link
Member

@kripken kripken commented Oct 20, 2021

This does so using a polyfill for XHR. That has downsides, but the shell
environment is only really used for local testing, so that seems ok.

Other options to fix the general issue of embed-file being slow on huge
files is to optimize embed-file's output. However, testing of emitting the
base64-encoded data in chunks is inconclusive: faster to parse in our
JS optimizer but slower in closure.

@kripken kripken requested review from sbc100 and ngzhian October 20, 2021 20:10
@ngzhian
Copy link
Collaborator

ngzhian commented Oct 20, 2021

Nice! Thank you, can confirm the compile is really fast now, and also the polyfill works with d8.

Copy link
Collaborator

@ngzhian ngzhian left a comment

Choose a reason for hiding this comment

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

Forgot to LGTM.

@sbc100
Copy link
Collaborator

sbc100 commented Oct 20, 2021

We already have a helper function called readAsync which works on all platforms.. can we not instead use that existing abstraction instead of relying on a web API and then poly-filling it?

IIRC that hard part of allowing file package to run both with and without the emscripten glue code.. but maybe file_packager can use readAsync when it knows the output will be embedded in the emscripten glue?

@kripken
Copy link
Member Author

kripken commented Oct 20, 2021

Yes, the problem is the file packager code is separate. It can't depend on readAsync being present for it, as it may have been compiled separately. We can't even export readAsync for it, as the file packager output is designed to start to run before the emscripten code even arrives - so that it can fire off async fetches while the emscripten code is being loaded.

(Note that I don't think polyfilling a web API is that bad. In fact, it makes the code simpler: all the code uses the Web API, and the polyfill is the one place that has special code for d8. That is the approach suggested in #12203. In comparison the node support in the file packager atm depends on having special node code in that file.)

@sbc100
Copy link
Collaborator

sbc100 commented Oct 20, 2021

Yes, the problem is the file packager code is separate. It can't depend on readAsync being present for it, as it may have been compiled separately. We can't even export readAsync for it, as the file packager output is designed to start to run before the emscripten code even arrives - so that it can fire off async fetches while the emscripten code is being loaded.

If the file packager output is designed to run before the emscripten code arrives.. then how would this polyfil work.. since the polyfill itself lives in the emscripten code?

@kripken
Copy link
Member Author

kripken commented Oct 20, 2021

Ah, good point... it won't work in totally separate output, you're right.

It does work in emcc --preload-file, that is, when file packager code is included as part of the build. That code appears first, before almost anything else, to allow it to start the fetches as early as possible. That's why the polyfill appears where it does.

@sbc100
Copy link
Collaborator

sbc100 commented Oct 20, 2021

Ah, good point... it won't work in totally separate output, you're right.

It does work in emcc --preload-file, that is, when file packager code is included as part of the build. That code appears first, before almost anything else, to allow it to start the fetches as early as possible. That's why the polyfill appears where it does.

Thats what I man. Is file_packer aware when its running in --preload-file mode? can it call readAsync.. in that case?

@@ -43,6 +43,10 @@ var Module = typeof {{{ EXPORT_NAME }}} !== 'undefined' ? {{{ EXPORT_NAME }}} :
#include "promise_polyfill.js"
#endif

#if ENVIRONMENT_MAY_BE_SHELL
Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, how about an internal settings called INCLUDE_XHR_POLYFILL which can be set when the file packager is used? Then we can avoid this unless is needed?

@kripken
Copy link
Member Author

kripken commented Oct 28, 2021

Thinking more on this, when the new FS is ready we should be able to embed files entirely in C. That would avoid this issues that led to this PR, as it would be easy and efficient. So let's close this, which would only help the old FS.

@kripken kripken closed this Oct 28, 2021
@kripken kripken deleted the preshell branch October 28, 2021 17:00
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.

3 participants