Skip to content
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
f30e444
Changes modularized build to promise-based API
lourd Mar 16, 2020
b4ec2f4
Adds modularized promise functions to externs
lourd Mar 16, 2020
6f4997a
Changes worker.js to account for promise
lourd Mar 18, 2020
fc9e049
Tries different structure for src/worker.js
Mar 19, 2020
49ae9d2
Clarifies a comment
Mar 19, 2020
af6689d
Merge remote-tracking branch 'origin/master'
Apr 2, 2020
af82a9c
Adds promise handler for MODULARIZE_INSTANCE case in worker
Apr 3, 2020
b402022
Adds additional promise resolution in postamble.js
Apr 15, 2020
63bb3af
Merge remote-tracking branch 'origin/master'
Apr 15, 2020
f205061
Removes some whitespace
Apr 15, 2020
69ba48b
Changes promise variable name back to EXPORT_NAME
Apr 15, 2020
cc89009
Adds _promise suffix to the instance promise var
Apr 15, 2020
0ab8841
Adds module promise resolution for MINIMAL_RUNTIME
Apr 15, 2020
30209eb
Updates the expected code sizes, adding about 220B
Apr 15, 2020
b44e3a3
Removes whitespace
Apr 15, 2020
9219dc2
Updates code sizes for hello_webgl_fastcomp_wasm
Apr 15, 2020
0f0166b
Changes arrow functions to function expressions
Apr 15, 2020
98eadf4
Updates code sizes for hello_webgl2_fastcomp_wasm
Apr 15, 2020
a7cd5ee
Updates code sizes for hello_webgl_fastcomp_asmjs
Apr 15, 2020
ed0fa51
Updates the rest of the fastcomp code sizes
Apr 15, 2020
5e7e7e9
Updates test_webidl fixture to handle promise path
Apr 16, 2020
19a6bd0
Merge remote-tracking branch 'origin/master'
Apr 20, 2020
f867846
Changes variable names to camelCase
Apr 22, 2020
3be551e
Removes externs for promise variables
Apr 22, 2020
8f8e1df
Removes a trailing whitespace
Apr 22, 2020
c6d63fc
Merge remote-tracking branch 'origin/master'
Apr 22, 2020
5b02d47
Reworks webidl test fixture
Apr 22, 2020
a481aa1
Readds externs and corrects the comment
Apr 22, 2020
955401a
Moves the closure compiler declarations & updates code sizes
Apr 22, 2020
e731dd6
Updates comment in post.js
Apr 22, 2020
333eb3b
Moves closure compiler declarations back to externs
Apr 22, 2020
f5691da
Merge remote-tracking branch 'origin/master'
Apr 24, 2020
40bb6fc
Merge remote-tracking branch 'origin/master'
May 4, 2020
124234f
Updates code sizes
May 4, 2020
b5bab7c
Removes extra #endif
May 4, 2020
4226a98
Moves modularize promise initialization to shell
May 7, 2020
7e3c8a1
Merge remote-tracking branch 'origin/master'
May 7, 2020
fc0078c
Adds some whitespace
May 8, 2020
2c84ebf
Updates the docs with new MODULARIZE behavior
May 8, 2020
48ab6dd
Updates changelog with MODULARIZE change
May 8, 2020
86208bb
Updates settings.js for new MODULARIZE behavior
May 8, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 21 additions & 20 deletions emcc.py
Original file line number Diff line number Diff line change
Expand Up @@ -1328,9 +1328,6 @@ def has_c_source(args):

if shared.Settings.MODULARIZE:
assert not options.proxy_to_worker, '-s MODULARIZE=1 and -s MODULARIZE_INSTANCE=1 are not compatible with --proxy-to-worker (if you want to run in a worker with -s MODULARIZE=1, you likely want to do the worker side setup manually)'
# MODULARIZE's .then() method uses onRuntimeInitialized currently, so make sure
# it is expected to be used.
shared.Settings.INCOMING_MODULE_JS_API += ['onRuntimeInitialized']

if shared.Settings.EMULATE_FUNCTION_POINTER_CASTS:
shared.Settings.ALIASING_FUNCTION_POINTERS = 0
Expand Down Expand Up @@ -3400,22 +3397,20 @@ def modularize():
logger.debug('Modularizing, assigning to var ' + shared.Settings.EXPORT_NAME)
src = open(final).read()

# TODO: exports object generation for MINIMAL_RUNTIME. As a temp measure, multithreaded MINIMAL_RUNTIME builds export like regular
# runtime does, so that worker.js can see the JS module contents.
exports_object = '{}' if shared.Settings.MINIMAL_RUNTIME and not shared.Settings.USE_PTHREADS else shared.Settings.EXPORT_NAME

src = '''
function(%(EXPORT_NAME)s) {
var returned_promise_resolve, returned_promise_reject;
var promise = new Promise((resolve, reject) => (returned_promise_resolve=resolve, returned_promise_reject=reject));

%(EXPORT_NAME)s = %(EXPORT_NAME)s || {};

%(src)s

return %(exports_object)s
return promise;
}
''' % {
'EXPORT_NAME': shared.Settings.EXPORT_NAME,
'src': src,
'exports_object': exports_object
}

if not shared.Settings.MODULARIZE_INSTANCE:
Expand Down Expand Up @@ -3450,26 +3445,32 @@ def modularize():
'src': src
}
else:
# 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.).
promise_variable_suffix = '_promise'
# Create the promise for the MODULARIZE_INSTANCE instance. The name of the
# variable will be `Module_promise` by default. If the EXPORT_NAME option
# is specified, the variable named will be `{EXPORT_NAME}_promise`.
#
# Note that the global, custom-named `Module` object is passed to the
# factory function invocation, just like in non-MODULARIZE mode. This is
# different than code built with the MODULARIZE option where the user
# specifies any Module seed when calling the factory function themselves.
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 : {});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

@lourd lourd Apr 23, 2020

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@curiousdannii curiousdannii Apr 23, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

''' % {
'EXPORT_NAME': shared.Settings.EXPORT_NAME,
'src': src
'src': src,
'promise_variable_suffix': promise_variable_suffix,
}

final = final + '.modular.js'
with open(final, 'w') as f:
f.write(src)

export_name = shared.Settings.EXPORT_NAME
if (shared.Settings.MODULARIZE_INSTANCE):
export_name += promise_variable_suffix
# Export using a UMD style export, or ES6 exports if selected

if shared.Settings.EXPORT_ES6:
f.write('''export default %s;''' % shared.Settings.EXPORT_NAME)
f.write('''export default %s;''' % export_name)
elif not shared.Settings.MINIMAL_RUNTIME:
f.write('''if (typeof exports === 'object' && typeof module === 'object')
module.exports = %(EXPORT_NAME)s;
Expand All @@ -3478,7 +3479,7 @@ def modularize():
else if (typeof exports === 'object')
exports["%(EXPORT_NAME)s"] = %(EXPORT_NAME)s;
''' % {
'EXPORT_NAME': shared.Settings.EXPORT_NAME
'EXPORT_NAME': export_name
})

save_intermediate('modularized')
Expand Down
11 changes: 9 additions & 2 deletions src/closure-externs/closure-externs.js
Original file line number Diff line number Diff line change
Expand Up @@ -999,7 +999,6 @@ var noExitRuntime;
// https://github.com/google/closure-compiler/issues/3167
var BigInt;


// Worklet
/**
* @constructor
Expand Down Expand Up @@ -1069,4 +1068,12 @@ AudioWorkletNode.prototype.onprocessorerror;
var registerProcessor = function(name, obj) {};
var currentFrame;
var currentTime;
var sampleRate;
var sampleRate;

// These functions are declared in emcc.py but used in postamble.js; we need
// to explicitly tell Closure Compiler about their existence since the
// postamble is optimized independently
/** @type {Function} */
var returned_promise_resolve;
/** @type {Function} */
var returned_promise_reject;
69 changes: 32 additions & 37 deletions src/postamble.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,12 @@ if (memoryInitializer) {
};
var doBrowserLoad = function() {
readAsync(memoryInitializer, applyMemoryInitializer, function() {
throw 'could not load memory initializer ' + memoryInitializer;
var e = new Error('could not load memory initializer ' + memoryInitializer);
#if MODULARIZE
returned_promise_reject(e);
#else
throw e;
#endif
});
};
#if SUPPORT_BASE64_EMBEDDING
Expand Down Expand Up @@ -121,36 +126,6 @@ if (memoryInitializer) {

var calledRun;

#if MODULARIZE
#if MODULARIZE_INSTANCE == 0
// Modularize mode returns a function, which can be called to
// create instances. The instances provide a then() method,
// must like a Promise, that receives a callback. The callback
// is called when the module is ready to run, with the module
// as a parameter. (Like a Promise, it also returns the module
// so you can use the output of .then(..)).
Module['then'] = function(func) {
// We may already be ready to run code at this time. if
// so, just queue a call to the callback.
if (calledRun) {
func(Module);
} else {
// we are not ready to call then() yet. we must call it
// at the same time we would call onRuntimeInitialized.
#if ASSERTIONS && !expectToReceiveOnModule('onRuntimeInitialized')
abort('.then() requires adding onRuntimeInitialized to INCOMING_MODULE_JS_API');
#endif
var old = Module['onRuntimeInitialized'];
Module['onRuntimeInitialized'] = function() {
if (old) old();
func(Module);
};
}
return Module;
};
#endif
#endif

/**
* @constructor
* @this {ExitStatus}
Expand Down Expand Up @@ -319,6 +294,9 @@ function run(args) {

preMain();

#if MODULARIZE
returned_promise_resolve(Module);
#endif
#if expectToReceiveOnModule('onRuntimeInitialized')
if (Module['onRuntimeInitialized']) Module['onRuntimeInitialized']();
#endif
Expand Down Expand Up @@ -429,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);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@lourd lourd May 8, 2020

Choose a reason for hiding this comment

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

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

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

So maybe err should be happening regardless of MODULARIZE?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good

#if MODULARIZE
returned_promise_reject(msg);
#endif // MODULARIZE
#else
err('program exited (with status: ' + status + '), but noExitRuntime is set due to an async operation, so halting execution but not exiting the runtime or preventing further async execution (you can use emscripten_force_exit, if you want to force a true shutdown)');
var msg = 'program exited (with status: ' + status + '), but noExitRuntime is set due to an async operation, so halting execution but not exiting the runtime or preventing further async execution (you can use emscripten_force_exit, if you want to force a true shutdown)';
err(msg);
#if MODULARIZE
returned_promise_reject(msg);
#endif // MODULARIZE
#endif // EXIT_RUNTIME
}
#endif // ASSERTIONS
Expand Down Expand Up @@ -484,15 +470,24 @@ if (!ENVIRONMENT_IS_PTHREAD) // EXIT_RUNTIME=0 only applies to default behavior
#endif

#if USE_PTHREADS
if (!ENVIRONMENT_IS_PTHREAD) run();
if (!ENVIRONMENT_IS_PTHREAD) {
run();
} else {
#if EMBIND
else { // Embind must initialize itself on all threads, as it generates support JS.
// Embind must initialize itself on all threads, as it generates support JS.
Module['___embind_register_native_and_builtin_types']();
}
#endif // EMBIND
#if MODULARIZE
// The promise resolve function typically gets called as part of the execution
// of the Module `run`. The workers/pthreads don't execute `run` here, they
// call `run` in response to a message at a later time, so the creation
// promise can be resolved, marking the pthread-Module as initialized.
returned_promise_resolve(Module);
#endif // MODULARIZE
}
#else
run();
#endif
#endif // USE_PTHREADS

#if BUILD_AS_WORKER

Expand Down
3 changes: 3 additions & 0 deletions src/shell_minimal.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,9 @@ function err(text) {
// compilation is ready. In that callback, call the function run() to start
// the program.
function ready() {
#if MODULARIZE
returned_promise_resolve(Module);
#endif // MODULARIZE
#if INVOKE_RUN && hasExportedFunction('_main')
#if USE_PTHREADS
if (!ENVIRONMENT_IS_PTHREAD) {
Expand Down
29 changes: 22 additions & 7 deletions src/worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ this.onmessage = function(e) {
#endif
#else
Module['wasmModule'] = e.data.wasmModule;
#endif
#endif // MINIMAL_RUNTIME

{{{ makeAsmImportsAccessInPthread('wasmMemory') }}} = e.data.wasmMemory;

Expand All @@ -136,15 +136,17 @@ this.onmessage = function(e) {
}
#endif

#endif
#endif // WASM

#if !MINIMAL_RUNTIME || MODULARIZE
{{{ makeAsmImportsAccessInPthread('ENVIRONMENT_IS_PTHREAD') }}} = true;
#endif

#if MODULARIZE && EXPORT_ES6
import(e.data.urlOrBlob).then(function({{{ EXPORT_NAME }}}) {
Module = {{{ EXPORT_NAME }}}.default(Module);
return {{{ EXPORT_NAME }}}.default(Module);
}).then(function(instance) {
Module = instance;
postMessage({ 'cmd': 'loaded' });
});
#else
Expand All @@ -155,15 +157,28 @@ this.onmessage = function(e) {
importScripts(objectUrl);
URL.revokeObjectURL(objectUrl);
}
#if MODULARIZE && !MODULARIZE_INSTANCE
#if MODULARIZE_INSTANCE
{{{ EXPORT_NAME }}}_promise.then((instance) => {
Module = instance;
postMessage({ 'cmd': 'loaded' });
});
#else
#if MODULARIZE
#if MINIMAL_RUNTIME
Module = {{{ EXPORT_NAME }}}(imports);
{{{ EXPORT_NAME }}}(imports).then((instance) => {
Module = instance;
postMessage({ 'cmd': 'loaded' });
});
#else
Module = {{{ EXPORT_NAME }}}(Module);
{{{ EXPORT_NAME }}}(Module).then((instance) => {
Module = instance;
postMessage({ 'cmd': 'loaded' });
});
#endif
#endif
#endif

#if !MINIMAL_RUNTIME || !WASM
#if !MODULARIZE && (!MINIMAL_RUNTIME || !WASM)
// MINIMAL_RUNTIME always compiled Wasm (&Wasm2JS) asynchronously, even in pthreads. But
// regular runtime and asm.js are loaded synchronously, so in those cases
// we are now loaded, and can post back to main thread.
Expand Down
8 changes: 4 additions & 4 deletions tests/code_size/hello_webgl2_wasm.json
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"a.html": 563,
"a.html.gz": 377,
"a.js": 5123,
"a.js.gz": 2419,
"a.js": 5360,
"a.js.gz": 2494,
"a.wasm": 10894,
"a.wasm.gz": 6920,
"total": 16580,
"total_gz": 9716
"total": 16817,
"total_gz": 9791
}
8 changes: 4 additions & 4 deletions tests/code_size/hello_webgl2_wasm2js.json
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"a.html": 588,
"a.html.gz": 386,
"a.js": 23727,
"a.js.gz": 8896,
"a.js": 23964,
"a.js.gz": 8968,
"a.mem": 3168,
"a.mem.gz": 2711,
"total": 27483,
"total_gz": 11993
"total": 27720,
"total_gz": 12065
}
8 changes: 4 additions & 4 deletions tests/code_size/hello_webgl_wasm.json
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"a.html": 563,
"a.html.gz": 377,
"a.js": 4615,
"a.js.gz": 2248,
"a.js": 4852,
"a.js.gz": 2322,
"a.wasm": 10894,
"a.wasm.gz": 6920,
"total": 16072,
"total_gz": 9545
"total": 16309,
"total_gz": 9619
}
8 changes: 4 additions & 4 deletions tests/code_size/hello_webgl_wasm2js.json
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"a.html": 588,
"a.html.gz": 386,
"a.js": 23222,
"a.js.gz": 8731,
"a.js": 23459,
"a.js.gz": 8806,
"a.mem": 3168,
"a.mem.gz": 2711,
"total": 26978,
"total_gz": 11828
"total": 27215,
"total_gz": 11903
}
Loading