Implements Promise-based API for modularized builds #10697

lourd · 2020-03-16T05:32:53Z

This removes the Module#then function and replaces it with a proper
Promise being returned from the initialization function.

Fixes #5820

As part of followup work I'll be updating the types repo as well, making a separate interface for the module factory function, which should make the EmscriptenModule interface simpler to use and extend.

tests/test_browser.py

emcc.py

This removes the `Module#then` function and replaces it with a proper Promise being returned from the initialization function. Fixes #5820

lourd · 2020-03-18T01:37:31Z

The test failures are all related to threaded builds. In debugging the tests, it seems that some aspect of the changes are causing the threads to never stop. I'm seeing console warnings like:

Pthread 0x0 completed its pthread main entry point with an unwind, keeping the pthread worker alive for asynchronous operation.

I'm also unsure of how to deal with the MODULARIZE_INSTANCE case which hits this codepath

emscripten/emcc.py

Lines 3405 to 3412 in b4ec2f4

    
               # Create the MODULARIZE_INSTANCE instance 
        
               # Note that we notice the global Module object, just like in normal 
        
               # non-MODULARIZE mode (while MODULARIZE has the user create the instances, 
        
               # and the user can decide whether to use Module there or something 
        
               # else etc.). 
        
               src = ''' 
        
           var %(EXPORT_NAME)s = (%(src)s)(typeof %(EXPORT_NAME)s === 'object' ? %(EXPORT_NAME)s : {}); 
        
           ''' % {

which executes the exported function, changing what is later expected to be the WASM Module instance into a Promise.

I'm a little lost on all of the build options and the mechanics of WASM pthreads, so any help would be greatly appreciated.

Also removes exports_object reference from emcc.py. It's probably not correct, but highlights that it's not being used if a promise is always returned.

kripken · 2020-03-18T20:48:42Z

About those pthread errors, I would check to see if you also get them without this PR - it's possible those tests do actually exit their main with an unwind (as the error says) intentionally.

About MODULARIZE_INSTANCE, it does seem that if the modularize function returns a promise, then the singleton instance should be a promise. (These are changes we'll need to document carefully, but if everyone is on board, then sounds good to me, despite the risk of breakage for some users.)

lourd · 2020-03-18T21:27:27Z

Thanks @kripken, I'll check. Regardless of the presence of the warnings, the pthread-related tests are failing because this change now makes the workers/threads go into an infinite loop without reporting back results to the test server.

Understood about the MODULARIZE_INSTANCE impact. I'll make the changes to the code and docs accordingly.

Turns out that the pthreads don't execute `run`, so need their own promise resolution.

Also removes onRuntimeInitialized implicit export in MODULARIZE builds

lourd · 2020-04-15T19:15:26Z

Alright, I think I've handled all of the cases now, please re-review @kripken. The only tests I'm seeing failing on some targets in CI are a mismatch about the expected code sizes for test_minimal_runtime_code_size, and test_webidl. Those are passing for the heavy majority of test targets though, and I'm not able to reproduce either on my local machine. Please let me know if/how I should handle those.

The documentation will definitely need to be updated -- should I do that as part of this PR?

lourd · 2020-04-15T19:17:25Z

emcc.py

    src = '''
-var %(EXPORT_NAME)s = (%(src)s)(typeof %(EXPORT_NAME)s === 'object' ? %(EXPORT_NAME)s : {});
+var %(EXPORT_NAME)s%(promise_variable_suffix)s = (%(src)s)(typeof %(EXPORT_NAME)s === 'object' ? %(EXPORT_NAME)s : {});


Highlighting that this introduces a new variable when using MODULARIZE_INSTANCE -- Module_promise, or {EXPORT_NAME}_promise. This was necessary because we don't want to overwrite the Module variable with a Promise, we only want to override it with the actual instance.

How about Module.promise, a property, instead of a completely new global?

This harkens back to the discussion from #5820 where we decided to go with a function that returns a Promise for modularized builds, rather than a making a new property like ready or promise on Module. I think adding a new property just in the MODULARIZE_INSTANCE case would be more confusing for users than adding a new global.

If you want to add a property in this case, I think we should go with that design for all cases, even non-modularized ones, removing onRuntimeInitialized.

Interesting... Well, I admit I don't remember the full discussion there, but maybe let's go back to everyone there and make sure this is what people wanted?

To me it seems kind of awkward to have Module, Module_promise globals. We should really have just one global in modularize mode, as one goal is to minimize global stuff. And, Module_promise seems like a typo of Module.promise to me personally...

I agree on the awkwardness. I don't think it's that much more confusing than the existing behavior though -- that Module might be a factory function or an instance, depending on the build options. Or that Module is first used as the seed, and then gets overwritten as the instance. At least with this behavior, Module is just the seed. Since this isn't the default behavior, I think it'll be ok with clear docs.

To be clear Module_promise variable will only exist for MODULARIZE_INSTANCE, when EXPORT_ES6 isn't used. It will not exist for MODULARIZE. Do you have a sense of how much MODULARIZE_INSTANCE is used?

The terminology has always been awkward. There's always been two "modules" in MODULARIZE mode, which doesn't really modularize, it's a factory!

In regular MODULARIZE mode, you don't get the internal Module until the promise resolves, right? It should be the same for MODULARIZE_INSTANCE. So it can't be a property.

I think I see... are you saying that Module doesn't have a value until Module_promise resolves? So initially, in MODULARIZE_INSTANCE, Module is undefined while Module_promise is a promise?

How about if it does have an initial value of

var Module = { promise: ..the value of Module_promise.. };

and there is no Module_promise global? That seems backwards compatible + avoids a new global, but I may be missing something...

Correct. Module only has a value if the user has defined it. If it exists it's used as the argument to the factory function, seen here. If the user never provides it, Module is never defined. The only global is Module_promise.

I don't think we could provide an initial value for Module given that behavior.

We could also change the suffix to Promise instead of _promise to keep with the camelCasing convention.

I'm a little unclear on if that description is about how it's always been, or if it's a new limitation in this PR?

AFAIK, there wasn't such a problem that required a new Module_promise before, so I'd guess it's new? If so, why is it needed? Before we also had to be able to access something, and it worked somehow?

This is a description of the existing behavior. It hasn't been highlighted before because the factory function executed synchronously, so the overwriting of any existing Module global variable wasn't as obvious, and accessing properties on it wasn't as difficult.

src/postamble.js

lourd · 2020-05-08T00:02:10Z

Sounds good. I'm pretty experienced with the various JS module systems, and I feel confident saying this doesn't change anything regarding how users will import the generated code through any of them.

Without knowing more about Binaryen, it appears that may have been able to be resolved by using MINIMAL_RUNTIME, which prevents the export footer from being added to the code.

What else is needed to land this change?

curiousdannii · 2020-05-08T00:49:58Z

Reminds me of a suggestion I made before, to deprecate EXPORT_ES6 and introduce EXPORT_METHOD with none, umd, and esm options (perhaps in the future moving to esm being the default).

Sorry if this has been covered before, but this PR only fixes promises for MODULARIZE mode right? Promises will need fixing for non-MODULARIZE mode too. We can do that as a followup PR, unless it would be simpler to do it as part of this one? (But probably not, this one has been a lot of work and deserves being merged as soon as it can be.)

lourd · 2020-05-08T04:10:48Z

All good ideas eventually see their day 😄

kripken

Ok, I think this looks good, thanks! Just a few minor comments below.

And one last important thing is documentation. Please mention this in ChangeLog.md, hopefully with a quick explanation of what people need to change if they are using this option - if that's can't be concise enough, then a link to src/settings.js perhaps. And either way that file should be updated (including what changed etc.).

Also the website docs have 3 files mentioning MODULARIZE,

site/source/docs/tools_reference/emcc.rst
site/source/docs/porting/connecting_cpp_and_javascript/WebIDL-Binder.rst
site/source/docs/getting_started/FAQ.rst

Looks like the last two mention the Promise, so they may need updating (not sure about the first). In particular the FAQ part with Be careful with mentions the bug that this fixes, so that should be removed.

There is still a small code size increase here, but given the major issues this fixes, that pings have not gotten any worries reported, and that it actually decreases code size in the non-minimal case (showing it's a better design), I think this is worth it. If this is an issue we can consider other options later.

src/postamble.js

kripken · 2020-05-08T16:56:06Z

src/postamble.js

@@ -427,9 +407,17 @@ function exit(status, implicit) {
    // if exit() was called, we may warn the user if the runtime isn't actually being shut down
    if (!implicit) {
 #if EXIT_RUNTIME == 0
-      err('program exited (with status: ' + status + '), but EXIT_RUNTIME is not set, so halting execution but not exiting the runtime or preventing further async execution (build with EXIT_RUNTIME=1, if you want a true shutdown)');
+      var msg = 'program exited (with status: ' + status + '), but EXIT_RUNTIME is not set, so halting execution but not exiting the runtime or preventing further async execution (build with EXIT_RUNTIME=1, if you want a true shutdown)';
+      err(msg);


is the err needed with MODULARIZE? that is, could it be in an else of the ifdef? (same also below)

I'm not sure. As far as I can tell, err normally just prints the string? Except here

emscripten/src/emrun_postjs.js

Line 45 in 58b49d3

err = function(text) { post('^err^'+(emrun_http_sequence_number++)+'^'+encodeURIComponent(text)); prevErr(text); };

So maybe err should be happening regardless of MODULARIZE?

I've never seen that emrun code before, but looks like it's doing something really weird there, which is not a problem for us.

So yes, err should just log to "stderr" basically. And so I think the current code will end up both printing the error and also rejecting the Promise, which as you said above would get printed out anyhow. I think it would be better to put it behind the if-else. But we can leave that for this PR I guess, I'd like to do a follow on this myself, and I can do it there.

Sounds good

src/shell.js

lourd · 2020-05-08T20:14:31Z

Alright, docs and changelog are updated.

kripken

Thanks! Ok, I think this is all good except I don't see a settings.js doc change?

Also, about ChangeLog.md, I imagine a user might see that and not be sure how to check if they're doing something that will break with this change, or not break (and if so, what they need to do). And the link goes to this PR, which is far too long for that person to read.

As I suggested earlier, one option might be to put such a "if you have this, you must change it to this" explanation in the Changelog itself. If it's too big, then in settings.js and add a reference to see that example there.

kripken · 2020-05-08T20:24:21Z

src/postamble.js

@@ -427,9 +407,17 @@ function exit(status, implicit) {
    // if exit() was called, we may warn the user if the runtime isn't actually being shut down
    if (!implicit) {
 #if EXIT_RUNTIME == 0
-      err('program exited (with status: ' + status + '), but EXIT_RUNTIME is not set, so halting execution but not exiting the runtime or preventing further async execution (build with EXIT_RUNTIME=1, if you want a true shutdown)');
+      var msg = 'program exited (with status: ' + status + '), but EXIT_RUNTIME is not set, so halting execution but not exiting the runtime or preventing further async execution (build with EXIT_RUNTIME=1, if you want a true shutdown)';
+      err(msg);


I've never seen that emrun code before, but looks like it's doing something really weird there, which is not a problem for us.

So yes, err should just log to "stderr" basically. And so I think the current code will end up both printing the error and also rejecting the Promise, which as you said above would get printed out anyhow. I think it would be better to put it behind the if-else. But we can leave that for this PR I guess, I'd like to do a follow on this myself, and I can do it there.

lourd · 2020-05-08T21:42:44Z

Oops, forgot settings.js -- updated now and tweaked the changelog description.

kripken

Great, thanks so much @lourd ! I know this was a long process, very grateful for your work and patience here.

Also thanks @curiousdannii and everyone else that commented here and in the issues before this!

curiousdannii · 2020-05-08T23:58:17Z

Really good docs in settings.js too, thanks!

juj · 2020-05-09T05:11:13Z

Wait.. what happens here? This PR regresses code size of MINIMAL_RUNTIME, and adds a new Module['ready'] core property to MINIMAL_RUNTIME that should have Module deprecated. Neither should be acceptable for MINIMAL_RUNTIME. I am sorry that I did not make time to review this, but I don't think this should have landed?

kripken · 2020-05-09T21:10:17Z

@juj I think you're already seen #11116 but that should remove the new overhead in the main case (I left it to a followup as this PR was already very long).

Get MINIMAL_RUNTIME + MODULARIZE code size back to where it was before that PR, when emitting HTML: the HTML in that mode will create a single instance of the app and run it automatically. It does so without needing the Promise, so we don't need to emit it. Diffing the output JS to before #10697 shows essentially no difference. Also move the changelog entry to the right place (we had a release meanwhile), expand it a little, and ifdef some logging in postamble.js (if we reject the Promise on an error, we log it out that way anyhow).

…DULARIZE (#11118) It used to be possible to do Module().onRuntimeInitialized = ... but with new MODULARIZE (#10697) Module() returns a Promise instead. The Promise of course allows a property to be placed on it, but nothing would happen, so it's potentially annoying for users. With this change, when ASSERTIONS are on we will throw an error with an explanation, something like You are setting onRuntimeInitialized on the Promise object, instead of the instance. Use .then() to get called back with the instance, see the MODULARIZE docs in src/settings.js Likewise, it used to be possible to do var instance = Module(); // .. later .. instance.exportedThing() but again, the "instance" is a Promise now, and the exports aren't there. This will crash, but ".. is not a function" is not that helpful. With this change it will give an explanation of what's wrong.

* Updates Emscripten types with new `MODULARIZE` API This corresponds with the changes to Emscripten in emscripten-core/emscripten#10697, creating a type for the new Promise-based API of the factory function generated when using `MODULARIZE`. Removes the obsolete `then` method from EmscriptenModule and separates the type for the factory function generated when using the `MODULARIZE` build option from the rest of the `EmscriptenModule` interface. This will make it easier to extend the interface. Also removes the global `Module` variable declaration as this is not applicable for every application. This was not ideal as it left a very generic variable name in the global scope for any application that used Emscripten, regardless of whether there was actually a global variable. This now leaves it up to the user to declare their module instance as appropriate for them. Fixes the reference in sql.js, using the EmscriptenModule interface directly. * Runs prettier on the emscripten types

lourd commented Mar 16, 2020

View reviewed changes

tests/test_browser.py Show resolved Hide resolved

lourd mentioned this pull request Mar 16, 2020

Undocumented Module.then causes infinite loop #5820

Closed

curiousdannii reviewed Mar 16, 2020

View reviewed changes

emcc.py Outdated Show resolved Hide resolved

lourd changed the title ~~Begins work on promise-based API when modularized~~ Implements Promise-based API for modularized builds Mar 17, 2020

lourd added 2 commits March 17, 2020 17:06

Changes modularized build to promise-based API

f30e444

This removes the `Module#then` function and replaces it with a proper Promise being returned from the initialization function. Fixes #5820

Adds modularized promise functions to externs

b4ec2f4

Changes worker.js to account for promise

6f4997a

Also removes exports_object reference from emcc.py. It's probably not correct, but highlights that it's not being used if a promise is always returned.

Louis DeScioli added 2 commits March 19, 2020 01:44

Tries different structure for src/worker.js

fc9e049

Clarifies a comment

49ae9d2

kripken mentioned this pull request Mar 26, 2020

ENV and IDBFS are undefined in 1.38.47-upstream & above #9827

Closed

Louis DeScioli added 10 commits April 2, 2020 17:00

Merge remote-tracking branch 'origin/master'

af6689d

Adds promise handler for MODULARIZE_INSTANCE case in worker

af82a9c

Adds additional promise resolution in postamble.js

b402022

Turns out that the pthreads don't execute `run`, so need their own promise resolution.

Merge remote-tracking branch 'origin/master'

63bb3af

Removes some whitespace

f205061

Changes promise variable name back to EXPORT_NAME

69ba48b

Adds _promise suffix to the instance promise var

cc89009

Also removes onRuntimeInitialized implicit export in MODULARIZE builds

Adds module promise resolution for MINIMAL_RUNTIME

0ab8841

Updates the expected code sizes, adding about 220B

30209eb

Removes whitespace

b44e3a3

lourd commented Apr 15, 2020

View reviewed changes

src/postamble.js Show resolved Hide resolved

Louis DeScioli added 4 commits April 15, 2020 16:02

Updates code sizes for hello_webgl_fastcomp_wasm

9219dc2

Changes arrow functions to function expressions

0f0166b

Updates code sizes for hello_webgl2_fastcomp_wasm

98eadf4

Updates code sizes for hello_webgl_fastcomp_asmjs

a7cd5ee

kripken approved these changes May 8, 2020

View reviewed changes

Louis DeScioli added 3 commits May 8, 2020 15:28

Adds some whitespace

fc0078c

Updates the docs with new MODULARIZE behavior

2c84ebf

Updates changelog with MODULARIZE change

48ab6dd

kripken reviewed May 8, 2020

View reviewed changes

Updates settings.js for new MODULARIZE behavior

86208bb

kripken approved these changes May 8, 2020

View reviewed changes

kripken merged commit 423a77f into emscripten-core:master May 8, 2020

lourd mentioned this pull request May 9, 2020

Updates Emscripten types with new MODULARIZE API DefinitelyTyped/DefinitelyTyped#44590

Merged

9 tasks

kripken mentioned this pull request May 9, 2020

MODULARIZE followups after #10697 #11116

Merged

kripken mentioned this pull request May 9, 2020

Add useful errors on bad use of Promise instead of instance in new MODULARIZE #11118

Merged

reighley-christopher mentioned this pull request May 28, 2020

recent emcc with legacy support breaks Promise based modularization #11271

Closed

huningxin mentioned this pull request Jun 3, 2020

[SIMD] investigate wasm simd implementation huningxin/opencv#332

Closed

3 tasks

curiousdannii mentioned this pull request Jul 27, 2020

add MODULARIZE=2 #9325

Closed

manzt mentioned this pull request Sep 1, 2020

Promisify emscripten modules & fix webp examples GoogleChromeLabs/squoosh#817

Merged

sbc100 mentioned this pull request Oct 30, 2020

Synchronous access to module compiled with WASM=0 #12647

Closed

pearswj mentioned this pull request Nov 10, 2020

Emscripten 1.39.6 breaks JavaScript build mcneel/rhino3dm#194

Closed

shamrin mentioned this pull request Feb 6, 2021

Consider removing manual modularization sql-js/sql.js#438

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements Promise-based API for modularized builds #10697

Implements Promise-based API for modularized builds #10697

lourd commented Mar 16, 2020 •

edited

Loading

lourd commented Mar 18, 2020 •

edited

Loading

kripken commented Mar 18, 2020

lourd commented Mar 18, 2020

lourd commented Apr 15, 2020

lourd Apr 15, 2020

kripken Apr 22, 2020

lourd Apr 22, 2020

kripken Apr 23, 2020

lourd Apr 23, 2020 •

edited

Loading

curiousdannii Apr 23, 2020 •

edited

Loading

kripken Apr 24, 2020

lourd Apr 24, 2020

kripken Apr 27, 2020

lourd Apr 28, 2020

lourd commented May 8, 2020

curiousdannii commented May 8, 2020 •

edited

Loading

lourd commented May 8, 2020

kripken left a comment

kripken May 8, 2020

lourd May 8, 2020 •

edited

Loading

kripken May 8, 2020

lourd May 8, 2020

lourd commented May 8, 2020

kripken left a comment

kripken May 8, 2020

lourd commented May 8, 2020

kripken left a comment

curiousdannii commented May 8, 2020

juj commented May 9, 2020

kripken commented May 9, 2020

Implements Promise-based API for modularized builds #10697

Implements Promise-based API for modularized builds #10697

Conversation

lourd commented Mar 16, 2020 • edited Loading

lourd commented Mar 18, 2020 • edited Loading

kripken commented Mar 18, 2020

lourd commented Mar 18, 2020

lourd commented Apr 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lourd Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

curiousdannii Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lourd commented May 8, 2020

curiousdannii commented May 8, 2020 • edited Loading

lourd commented May 8, 2020

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lourd May 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lourd commented May 8, 2020

kripken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lourd commented May 8, 2020

kripken left a comment

Choose a reason for hiding this comment

curiousdannii commented May 8, 2020

juj commented May 9, 2020

kripken commented May 9, 2020

lourd commented Mar 16, 2020 •

edited

Loading

lourd commented Mar 18, 2020 •

edited

Loading

lourd Apr 23, 2020 •

edited

Loading

curiousdannii Apr 23, 2020 •

edited

Loading

curiousdannii commented May 8, 2020 •

edited

Loading

lourd May 8, 2020 •

edited

Loading