Skip to content

Fix EXPORT_NAME + MODULARIZE_INSTANCE + pthreads #10449

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.

Sign up for GitHub

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 8 commits into from
Feb 25, 2020
Merged

Fix EXPORT_NAME + MODULARIZE_INSTANCE + pthreads #10449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
00791ad
Fix EXPORT_NAME + MODULARIZE_INSTANCE + pthreads. fixes #9091
kripken Feb 14, 2020
ccf2871
flake8 [ci skip]
kripken Feb 14, 2020
5b5e528
ignore fastcomp
kripken Feb 14, 2020
c23c7f0
fix err/out
kripken Feb 18, 2020
bb4dab1
Merge remote-tracking branch 'origin/master' into export_modularize_p…
kripken Feb 18, 2020
6a6c0b5
Merge branch 'master' into export_modularize_pthread
kripken Feb 19, 2020
a37d7ff
remove out()
kripken Feb 20, 2020
6e3009e
Merge remote-tracking branch 'origin/master' into export_modularize_p…
kripken Feb 20, 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
26 changes: 19 additions & 7 deletions src/worker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,15 @@ var threadInfoStruct = 0; // Info area for this thread in Emscripten HEAP (share
var selfThreadId = 0; // The ID of this thread. 0 if not hosting a pthread.
var parentThreadId = 0; // The ID of the parent pthread that launched this thread.

// Cannot use console.log or console.error in a web worker, since that would risk a browser deadlock! https://bugzilla.mozilla.org/show_bug.cgi?id=1049091
// Therefore implement custom logging facility for threads running in a worker, which queue the messages to main thread to print.
var Module = {};

// If we use a custom export name, refer to Module from it as well, so that
// we connect properly to the modularized instance which is the only thing
// in the global scope.
#if MODULARIZE_INSTANCE && EXPORT_NAME != 'Module'
var {{{ EXPORT_NAME }}} = Module;
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it work instead of

var Module = {};


// If we use a custom export name, refer to Module from it as well, so that
// we connect properly to the modularized instance which is the only thing
// in the global scope.
#if MODULARIZE_INSTANCE && EXPORT_NAME != 'Module'
var {{{ EXPORT_NAME }}} = Module;
#endif

to do

var {{{ EXPORT_NAME }}} = {};

and in all places in the worker below, access {{{ EXPORT_NAME }}} instead of Module?

Copy link
Member Author

Choose a reason for hiding this comment

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

That might work too, but I think it makes the code a little less readable, and there's a risk of future changes to the file forgetting and using Module (and then working except for the corner case fixed here).


#if ASSERTIONS
function assert(condition, text) {
if (!condition) abort('Assertion failed: ' + text);
Expand All @@ -25,12 +30,19 @@ function assert(condition, text) {
function threadPrintErr() {
var text = Array.prototype.slice.call(arguments).join(' ');
console.error(text);
console.error(new Error().stack);
}
function threadAlert() {
var text = Array.prototype.slice.call(arguments).join(' ');
postMessage({cmd: 'alert', text: text, threadId: selfThreadId});
}
#if ASSERTIONS
// We don't need out() for now, but may need to add it if we want to use it
// here. Or, if this code all moves into the main JS, that problem will go
// away. (For now, adding it here increases code size for no benefit.)
var out = function() {
throw 'out() is not defined in worker.js.';
}
#endif
var err = threadPrintErr;
this.alert = threadAlert;

Expand Down Expand Up @@ -258,7 +270,7 @@ this.onmessage = function(e) {
#if ASSERTIONS
} else {
// else e == 'unwind', and we should fall through here and keep the pthread alive for asynchronous events.
out('Pthread 0x' + threadInfoStruct.toString(16) + ' completed its pthread main entry point with an unwind, keeping the pthread worker alive for asynchronous operation.');
err('Pthread 0x' + threadInfoStruct.toString(16) + ' completed its pthread main entry point with an unwind, keeping the pthread worker alive for asynchronous operation.');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
err('Pthread 0x' + threadInfoStruct.toString(16) + ' completed its pthread main entry point with an unwind, keeping the pthread worker alive for asynchronous operation.');
out('Pthread 0x' + threadInfoStruct.toString(16) + ' completed its pthread main entry point with an unwind, keeping the pthread worker alive for asynchronous operation.');

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since it looks like it's only used for debugging purposes. Correct me if I'm wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is for debugging, yeah. So it actually seems natural to use err instead of out to me? But I guess we don't have a very clear policy on such logging (which doesn't matter much in practice on the web console). I'm happy to do it either way people prefer.

Copy link
Collaborator

@kleisauke kleisauke Feb 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I see. The out / threadPrintOut functions however seems to be unused now in worker.js, so perhaps these can be removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess they can. I added them for completeness, as they may be needed in the future. But I believe we don't DCE this file, so probably best to remove, I'll do that.

#endif
}
}
Expand All @@ -274,11 +286,11 @@ this.onmessage = function(e) {
}
} else {
err('worker.js received unknown command ' + e.data.cmd);
console.error(e.data);
err(e.data);
}
} catch(ex) {
console.error('worker.js onmessage() captured an uncaught exception: ' + ex);
if (ex.stack) console.error(ex.stack);
err('worker.js onmessage() captured an uncaught exception: ' + ex);
if (ex.stack) err(ex.stack);
throw ex;
}
};
Expand Down
15 changes: 15 additions & 0 deletions tests/test_browser.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4961,3 +4961,18 @@ def test_wasm2js_fallback(self):
open('test.html', 'w').write(html)
os.remove('test.wasm') # Also delete the Wasm file to test that it is not attempted to be loaded.
self.run_browser('test.html', 'hello!', '/report_result?0')

# Test that basic thread creation works in combination with MODULARIZE_INSTANCE=1 and EXPORT_NAME=MyModule
@no_fastcomp('more work would be needed for this to work in deprecated fastcomp')
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the difference with fastcomp that causes this to fail? Iiuc MODULARIZE_INSTANCE is an identical feature in both backends?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fastcomp emits a bunch of code in the HTML to deal with the asm.js memory init file, and that has hardcoded Module in there (instead of the custom export name). From the stack trace I think that's what goes wrong, but I didn't investigate.

@requires_threads
def test_pthread_modularize_export_name(self):
create_test_file('shell.html', '''
<body>
{{{ SCRIPT }}}
</body>
''')
self.btest(path_from_root('tests', 'pthread', 'test_pthread_create.cpp'),
expected='0',
args=['-s', 'TOTAL_MEMORY=64MB', '-s', 'USE_PTHREADS=1', '-s',
'PTHREAD_POOL_SIZE=8', '-s', 'MODULARIZE_INSTANCE=1',
'-s', 'EXPORT_NAME=MyModule', '--shell-file', 'shell.html'])