Module.pthreadPoolSize by kripken · Pull Request #10263 · emscripten-core/emscripten

kripken · 2020-01-22T18:09:24Z

This does the same as PTHREAD_POOL_SIZE but can be specified
at runtime. While the compile-time setting is great for testing, in
practice I think the Module option is probably more useful, since lots
of projects want to use a pool equal to the number of actual cores
on the user's machine.

fixes #10231

juj · 2020-01-22T18:49:17Z

Hmm, this is tricky, because it adds un-DCEable code that cannot be optimized away.

How about instead:

use -s PTHREAD_POOL_SIZE=-1 to mean a compile time decision "give me navigator.hardwareConcurrency threads in the pool" (or "give me Module['pthreadPoolSize'] || navigator.hardwareConcurrency` threads in the pool?), and/or
use -s PTHREAD_POOL_SIZE=2*#+1 to mean a compile time decision "give me 2*navigator.hardwareConcurrency + 1 threads in the pool, and/or
document a code snippet that let's users create an appropriate preRun/--pre-js code that initializes a pool?

That way we would not build more unoptimizable runtime bits. I know the increase in this PR does not add much, but it feels to me that we should strongly prefer to add only DCEable runtime elements.

Or I suppose if the references to Module['pthreadPoolSize'] are gated behind -s PTHREAD_POOL_SIZE setting, then the added size is not bad.

Basically the pthread pool is intended to be an emulation setting for use cases where sync pthread_create() is absolutely required, so I would not want to see all users have to pay bytes for that emulation setting.

kripken · 2020-01-22T21:45:42Z

Fair point about code size. I feel it's a little clunky to need to set an option in order to get Module.pthreadsPoolSize to be noticed, but I guess it's not that bad. Changed it that way.

I did consider having special values for "number of cores" or such, but there are going to be more general cases that that doesn't handle, so the Module option seems best.

juj · 2020-01-23T09:11:59Z

+  addOnPreRun(function() {
+    if (typeof SharedArrayBuffer !== 'undefined') {
+      addRunDependency('pthreads');
+      PThread.allocateUnusedWorkers(pthreadPoolSize, function() {


Uh hmm we have now both PThread.allocateUnusedWorkers and PThread.createNewWorkers(pthreadPoolSize);, that seems very wrong. To me this looks like we are allocating 2x the amount of Workers that are needed? Or perhaps I am very confused here. The intent of #9394 where this came into existence does not make sense to me. hmm...

I believe the idea is that creating and instantiating the workers is a separate thing. One can create the Worker objects without blocking startup on being able to instantiate them with the info they require etc. But we can discuss in your comments over there.

However, all of that is separate from this PR, and should not block it, unless I'm missing something?

Perhaps this PR can be relayered on top of https://github.com/emscripten-core/emscripten/pull/10269/files#diff-db41bea94577c2dd9b0eef0308b06cf9R36 ?

Maybe we can relayer on top of that, but that one seems to need discussion (so we figure out the history there) so unclear how long it will take to land, while this one is ready to go, isn't it? 😄 Or is there some reason you'd prefer to delay landing this?

juj · 2020-01-23T09:12:45Z

-if (!ENVIRONMENT_IS_PTHREAD) addOnPreRun(function() { if (typeof SharedArrayBuffer !== 'undefined') { addRunDependency('pthreads'); PThread.allocateUnusedWorkers({{{PTHREAD_POOL_SIZE}}}, function() { removeRunDependency('pthreads'); }); }});
+#if USE_PTHREADS && PTHREAD_POOL_SIZE
+
+var pthreadPoolSize = Module['pthreadPoolSize'] || {{{ PTHREAD_POOL_SIZE }}};


If one wanted to set Module['pthreadPoolSize'] = 0; to disable pthread pool, this has the effect of using {{{PTHREAD_POOL_SIZE}}} instead?

Oh, good point. Perhaps this should be more careful then.

Another thought I had meanwhile, if we're considering delaying this PR anyhow as discussed above - perhaps PTHREAD_POOL_SIZE could be a string - then the user could just set it to navigator.hardwareConcurrency directly, what do you think?

I like that idea - they could then set it to Module['pthreadPoolSize'] as well to customize.

juj · 2020-01-23T09:14:01Z

 #endif
+        PThread.preallocatedWorkers = PThread.createNewWorkers(pthreadPoolSize);
+      }
+#endif // PTHREAD_POOL_SIZE


I am not sure why this code block even exists.. this seems to double allocate a new pool of Workers that has already been created in the code below in src/preamble.js?

kripken · 2020-02-04T16:59:48Z

Ok, rewritten to simply make PTHREAD_POOL_SIZE a string,
PTAL @juj

This makes PTHREAD_POOL_SIZE a string, which allows stuff
like -s PTHREAD_POOL_SIZE=navigator.hardwareConcurrency
in addition to the old -s PTHREAD_POOL_SIZE=8 etc. (This works
because our parser is smart and doesn't require unnecessary
quotes for string values.) The value the user provided is simply
emitted in the JS, and used, so it could be anything that is valid
JS.

juj

lgtm!

Without the linker option `PTHREAD_POOL_SIZE` set, the wasm backend does not create any webworkers and spins forever waiting for them to be created (crashing the browser). This PR sets `PTHREAD_POOL_SIZE` to `Math.min(4, Math.max(1, (navigator.hardwareConcurrency || 1) / 2))`, meaning it will use half of the logical cores available up to a max of 4 (or only 1 if `navigator.hardwareConcurrency` is not available). See [this comment](emscripten-core/emscripten#10263 (comment)) for details on why this works. This option apparently supersedes the value passed to [`pthreadpool_create` in `backend.cc`](https://github.com/tensorflow/tfjs/blob/c9dfebddfa34f531d2f0c363b6ea574a9fe9745d/tfjs-backend-wasm/src/cc/backend.cc#L65-L66), but its current setting should match the value computed in the `.cc` file. Fixes #4932 This PR also fixes a bug where webworkers created by the wasm backend were not removed when `dispose` is called. Fixes #4796. Fixes #4934 as well.

kripken requested a review from juj January 22, 2020 18:09

juj mentioned this pull request Jan 23, 2020

Remove PThread.preallocatedWorkers #10269

Merged

juj reviewed Jan 23, 2020

View reviewed changes

kripken force-pushed the pool branch from 7c87edb to a44349a Compare February 4, 2020 00:04

kripken and others added 4 commits February 4, 2020 06:02

NEW [ci skip]

3182290

nicer

39ed1a9

fix typo [ci skip]

8494cc3

typo

6012fc5

kripken force-pushed the pool branch from f6f4513 to 6012fc5 Compare February 4, 2020 14:04

juj approved these changes Feb 5, 2020

View reviewed changes

kripken merged commit dfc667c into master Feb 5, 2020

kripken deleted the pool branch February 5, 2020 22:33

RReverser mentioned this pull request Sep 22, 2020

Document that PTHREAD_POOL_SIZE accepts JS expressions #12302

Closed

mattsoulanille mentioned this pull request Apr 19, 2021

[wasm] Fix wasm backend not starting any threads tensorflow/tfjs#4957

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Module.pthreadPoolSize#10263

Module.pthreadPoolSize#10263
kripken merged 4 commits intomasterfrom
pool

kripken commented Jan 22, 2020

Uh oh!

juj commented Jan 22, 2020

Uh oh!

kripken commented Jan 22, 2020

Uh oh!

juj Jan 23, 2020

Uh oh!

kripken Jan 23, 2020

Uh oh!

juj Jan 23, 2020

Uh oh!

kripken Jan 23, 2020

Uh oh!

juj Jan 23, 2020

Uh oh!

kripken Jan 23, 2020

Uh oh!

juj Jan 24, 2020

Uh oh!

juj Jan 23, 2020

Uh oh!

kripken commented Feb 4, 2020

Uh oh!

juj left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

kripken commented Jan 22, 2020

Uh oh!

juj commented Jan 22, 2020

Uh oh!

kripken commented Jan 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken commented Feb 4, 2020

Uh oh!

juj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants