Skip to content

Minimal runtime pthreads #10123

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 27 commits into from
Feb 18, 2020
Merged

Conversation

juj
Copy link
Collaborator

@juj juj commented Dec 29, 2019

This adds support for multithreading with MINIMAL_RUNTIME, in both regular build mode and MODULARIZEd build mode.

@juj juj force-pushed the minimal_runtime_pthreads branch 2 times, most recently from 12011bb to e832463 Compare January 13, 2020 19:51
@juj juj changed the base branch from incoming to master January 14, 2020 18:56
@juj juj force-pushed the minimal_runtime_pthreads branch from e832463 to 428287a Compare January 18, 2020 12:06
@juj juj force-pushed the minimal_runtime_pthreads branch 5 times, most recently from 870d380 to ea02fab Compare February 11, 2020 14:18
@juj
Copy link
Collaborator Author

juj commented Feb 12, 2020

Brought this PR up to latest and all tests pass. This would be great to see a review and get landed soon, it'll bump up MINIMAL_RUNTIME features with threading support!

STACK_MAX = stackMax;
},
#endif

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This deletion is not strictly related to MINIMAL_RUNTIME multithreading support, but while doing this PR I realized this whole function is redundant, so simplified the code.

if shared.Settings.USE_PTHREADS:
funcs.append('''
function establishStackSpace(stackBase, stackMax) {
function asmJsEstablishStackFrame(stackBase, stackMax) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renamed this to highlight the fact that this functionality is only needed for old fastcomp threading support.

'DYNAMIC_BASE': DYNAMIC_BASE,
#endif
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In MINIMAL_RUNTIME the DYNAMIC_BASE variable is not a thing.

'DYNAMICTOP_PTR': DYNAMICTOP_PTR
});
},

// Creates a new web Worker and places it in the unused worker pool to wait for its use.
allocateUnusedWorker: function() {
#if MINIMAL_RUNTIME
var pthreadMainJs = Module['worker'];
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This use of Module['worker'] is probably temporary/internal, and will go away later as the src/worker.js refactor happens.

#if !ALLOW_BLOCKING_ON_MAIN_THREAD
abort('Blocking on the main thread is not allowed by default. See https://emscripten.org/docs/porting/pthreads.html#blocking-on-the-main-browser-thread');
#endif

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In MINIMAL_RUNTIME we do not want to pay code size for emscripten_check_blocking_allowed type of checks, so this is not generated there.

@@ -597,3 +599,5 @@ var emscriptenMemoryProfiler = {
function memoryprofiler_add_hooks() { emscriptenMemoryProfiler.initialize(); }

if (typeof Module !== 'undefined' && typeof document !== 'undefined' && typeof window !== 'undefined' && typeof process === 'undefined') emscriptenMemoryProfiler.initialize();

#endif
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moving this #ifdef was done to be more idiomatic with include guards (the file itself guards, rather than the includer of that file)

@@ -3,7 +3,7 @@ mergeInto(LibraryManager.library, {
report_result__proxy: 'async',
report_result__sig: 'viii',
report_result: function(param1, param2, param3) {
if (ENVIRONMENT_IS_WORKER) {
if (typeof ENVIRONMENT_IS_WORKER !== 'undefined' && ENVIRONMENT_IS_WORKER) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This test change and all the test changes below are to make pthreads tests pass in MINIMAL_RUNTIME. It enables a workflow where I locally edit src/settings.js directly to change var MINIMAL_RUNTIME = 1; by default to run the whole browser.test_pthread* test suite in MINIMAL_RUNTIME, even though we don't duplicate all browser tests twice for MINIMAL_RUNTIME and non-MINIMAL_RUNTIME cases (only select tests are duplicated)

@@ -3427,8 +3434,7 @@ def module_export_name_substitution():
src = src.replace(shared.JS.module_export_name_substitution_pattern, replacement)
# For Node.js and other shell environments, create an unminified Module object so that
# loading external .asm.js file that assigns to Module['asm'] works even when Closure is used.
if shared.Settings.MINIMAL_RUNTIME and (shared.Settings.target_environment_may_be('node') or
shared.Settings.target_environment_may_be('shell')):
if shared.Settings.MINIMAL_RUNTIME and not shared.Settings.MODULARIZE_INSTANCE and (shared.Settings.target_environment_may_be('node') or shared.Settings.target_environment_may_be('shell')):
Copy link
Member

Choose a reason for hiding this comment

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

why does modularize_instance matter here? (and, why not modularize?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

MODULARIZE_INSTANCE creates the Module on the next line, emitting

if(typeof Module==="undefined"){var Module={};}
var Module = (

would be redundant for code size.

#if ASSERTIONS
assert(ENVIRONMENT_IS_WEB);
warnOnce('Blocking on the main thread is very dangerous, see https://emscripten.org/docs/porting/pthreads.html#blocking-on-the-main-browser-thread');
#if ASSERTIONS || IN_TEST_HARNESS || !MINIMAL_RUNTIME || !ALLOW_BLOCKING_ON_MAIN_THREAD
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand why !MINIMAL_RUNTIME is here? This code is already behind ASSERTIONS, so it won't be in an optimized build with the minimal runtime or not? I think it's useful in assertion builds to have maximal checks, regardless of runtime.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh, that is a || !MINIMAL_RUNTIME statement and not a && !MINIMAL_RUNTIME statement. The || !MINIMAL_RUNTIME statement is here so that regular runtime will get the non-assertion release builds also to contain the allow blocking checks.

@@ -86,6 +86,10 @@ var LibraryManager = {
libraries.push('library_browser.js');
}

if (USE_PTHREADS) { // TODO: Currently WebGL proxying makes pthreads library depend on WebGL.
libraries.push('library_webgl.js');
}
Copy link
Member

Choose a reason for hiding this comment

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

is this a fix unrelated to this PR?

(we don't deduplicate this list, so this is extra work, and potentially buggy if we have global state - we should deduplicate it before doing this, probably)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This fix is not unrelated - in MINIMAL_RUNTIME and STRICT we don't autolink libraries, and that would cause pthreads builds to fail at missing GL libraries. Added deduplication.

#if MODULARIZE_INSTANCE
var imports = Module;
#else
var imports = {};
Copy link
Member

Choose a reason for hiding this comment

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

how does this empty imports object work? is it filled later? (seems not?) do we not need imports somehow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The imports are filled from makeAsmImportsAccessInPthread() function.

char name[7] = "resize";
getDomElementParentInnerHTML(name, dst, sizeof(dst));
char name[7] = "body";
const char *dst = getDomElementContents(name);
memset(name, 0, sizeof(name)); // Try to uncover if there might be a race condition and above line was not synchronously processed, and we could take name string away.
Copy link
Member

Choose a reason for hiding this comment

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

are the changes to this file necessary? (comment above said they were, but I don't see how?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The old test asserted the existence of an inner HTML text node with text contents "resize" in it (that was the [ ] resize canvas checkbox in default shell). In MINIMAL_RUNTIME shell such checkbox does not exist, so I replaced it with a string snippet <canvas that can be found in both shells.

@@ -107,7 +108,7 @@ int main()
for(int i = 0; i < NUM_THREADS; ++i)
CreateThread(i, threadNum++);

emscripten_set_main_loop(WaitToJoin, 0, 0);
emscripten_set_timeout_loop(WaitToJoin, 100, 0);
} else {
Copy link
Member

Choose a reason for hiding this comment

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

same question for this file - why doesn't it work in minimal_runtime without these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

MINIMAL_RUNTIME does not support library_browser.js because the $Browser object does not DCE well if linked in. This test was not looking to exercise emscripten_set_main_loop, so changed to emscripten_set_timeout_loop for the same effect.

@juj juj force-pushed the minimal_runtime_pthreads branch from 2d1e36e to 2290619 Compare February 17, 2020 12:51
@juj
Copy link
Collaborator Author

juj commented Feb 17, 2020

Ping, any chance this could get a second review pass?

@juj juj force-pushed the minimal_runtime_pthreads branch from 2290619 to 61d0ea0 Compare February 17, 2020 12:56
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.

lgtm, and good plan for the followup!

@juj juj merged commit 673915c into emscripten-core:master Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants