Skip to content

Enable using -s EXIT_RUNTIME=0/1 when paired with -s PROXY_TO_PTHREAD=1 option.#10126

Merged
juj merged 1 commit intoemscripten-core:incomingfrom
juj:remove_noExitRuntime_EM_ASM
Jan 8, 2020
Merged

Enable using -s EXIT_RUNTIME=0/1 when paired with -s PROXY_TO_PTHREAD=1 option.#10126
juj merged 1 commit intoemscripten-core:incomingfrom
juj:remove_noExitRuntime_EM_ASM

Conversation

@juj
Copy link
Copy Markdown
Collaborator

@juj juj commented Dec 29, 2019

Enable using -s EXIT_RUNTIME=1 when paired with -s PROXY_TO_PTHREAD=1. Previously PROXY_TO_PTHREAD=1 always implied EXIT_RUNTIME=0, and in this build mode, EXIT_RUNTIME=0 would specify the behavior of what the main browser thread would do after calling __proxy_main(). I.e. the main browser thread should not exit when the internal __proxy_main() function has been called. Users did not have a possibility to do -s PROXY_TO_PTHREAD=1 -s EXIT_RUNTIME=1 builds at all.

After this change, -s EXIT_RUNTIME=0/1 now applies to the main() function instead, like one would expect: specifying -s EXIT_RUNTIME=1 means that the application will quit immediately when the proxied main() returns. Specifying -s EXIT_RUNTIME=0 means that the application will continue on executing after the proxied main() returns.

@juj juj force-pushed the remove_noExitRuntime_EM_ASM branch from 854b7fc to 7d4b9fd Compare January 7, 2020 09:28
@juj
Copy link
Copy Markdown
Collaborator Author

juj commented Jan 7, 2020

Hmm, the CI Failed in lsan tests, but oddly when I run locally, the same lsan tests fail in current incoming as well even without this PR. Rebased on top of latest incoming to check if the failure repeats...

…. Previously PROXY_TO_PTHREAD=1 always implied EXIT_RUNTIME=0, and in this build mode, EXIT_RUNTIME=0 would specify the behavior of what the main browser thread would do after calling __proxy_main(). I.e. the main browser thread should not exit when the internal __proxy_main() function has been called. Users did not have a possibility to do -s PROXY_TO_PTHREAD=1 -s EXIT_RUNTIME=1 builds at all.

After this change, -s EXIT_RUNTIME=0/1 now applies to the main() function instead, like one would expect: specifying -s EXIT_RUNTIME=1 means that the application will quit immediately when the proxied main() returns. Specifying -s EXIT_RUNTIME=0 means that the application will continue on executing after the proxied main() returns.
@juj juj force-pushed the remove_noExitRuntime_EM_ASM branch from 7d4b9fd to c11a736 Compare January 7, 2020 12:41
@juj juj changed the title Remove unnecessary noExitRuntime = true EM_ASM with PROXY_TO_PTHREAD. Enable using -s EXIT_RUNTIME=0/1 when paired with -s PROXY_TO_PTHREAD=1 option. Jan 7, 2020
@juj
Copy link
Copy Markdown
Collaborator Author

juj commented Jan 7, 2020

Oh, hmm, while debugging this, I realized a better way to do this altogether.

Changed this PR to actually add support for -s EXIT_RUNTIME=0/1 in pthreads. Updated the description in this PR accordingly. @kripken: want to take another look?

// Proceed by running main() on the main browser thread as a fallback.
return __call_main(_main_arguments.argc, _main_arguments.argv);
}
EM_ASM(noExitRuntime = true);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how is this change related to PROXY_TO_PTHREAD?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This EM_ASM line is what used to keep the runtime alive in PROXY_TO_PTHREAD build mode.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see, thanks!

Comment thread src/library_pthread.js
if (detached) {
PThread.returnWorkerToPool(worker);
}
#if EXIT_RUNTIME // If building with -s EXIT_RUNTIME=0, no thread will post this message, so don't even compile it in.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What about if the application actually calls "exit()"? Or does -s EXIT_RUNTIME=0 keep the program alive in that case too?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

If you look at the implementation of exit(), you can see that it is a no-op in -s EXIT_RUNTIME=0 builds. (Also calling exit() never posted a exitProcess message)

@kripken
Copy link
Copy Markdown
Member

kripken commented Jan 8, 2020

Btw, it might be useful to do less force-pushes in PRs, because it would allow seeing the changes over time (and we squash them at the end anyhow). I'm not sure how the code changed here in the new version?

@juj
Copy link
Copy Markdown
Collaborator Author

juj commented Jan 8, 2020

Ah sorry - yeah, out of habit I tend to force push since we used to have a workflow to preserve commit history from PRs.

The original version of the code just force-enabled EXIT_RUNTIME=0 when PROXY_TO_PTHREAD=1 was in effect. (i.e. just did from command line the same thing that the EM_ASM() block achieved), but this new version instead changes the meaning of EXIT_RUNTIME to not apply to the main browser thread, but to the proxied main() thread, so it has same functional meaning as when used in single-threaded builds.

@sbc100
Copy link
Copy Markdown
Collaborator

sbc100 commented Jan 8, 2020

Btw, it might be useful to do less force-pushes in PRs, because it would allow seeing the changes over time (and we squash them at the end anyhow). I'm not sure how the code changed here in the new version?

BTW, for my changes I follow a rebase workflow locally so I almost always have to force push. I can preserve the original commit if you prefer but I also sometime like to squash locally too.. I'll try not to do that with this repo if you prefer.

@juj juj merged commit 52ffe71 into emscripten-core:incoming Jan 8, 2020
@juj juj deleted the remove_noExitRuntime_EM_ASM branch January 9, 2020 15:57
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
…. Previously PROXY_TO_PTHREAD=1 always implied EXIT_RUNTIME=0, and in this build mode, EXIT_RUNTIME=0 would specify the behavior of what the main browser thread would do after calling __proxy_main(). I.e. the main browser thread should not exit when the internal __proxy_main() function has been called. Users did not have a possibility to do -s PROXY_TO_PTHREAD=1 -s EXIT_RUNTIME=1 builds at all. (emscripten-core#10126)

After this change, -s EXIT_RUNTIME=0/1 now applies to the main() function instead, like one would expect: specifying -s EXIT_RUNTIME=1 means that the application will quit immediately when the proxied main() returns. Specifying -s EXIT_RUNTIME=0 means that the application will continue on executing after the proxied main() returns.
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.

3 participants