Enable pthread_create with ASYNCIFY#26933
Conversation
pthread_create with ASYNCIFY
|
View with "hide whitespace" to see how simple this change really is. Its a testamant to all the work we have done over the years with ASYNCIFY/JSPI that this patch is so small. |
c59d635 to
08bf02e
Compare
This means that users of ASYNCIFY/JSPI no longer need to worry about setting `PTHREAD_POOL_SIZE`. We can do the same for pthread_join in a followup I think. See: emscripten-core#9910
08bf02e to
af21449
Compare
| // This is needed in browsers where syncronous worker creation is still not | ||
| // possible: <BUG_LINK> |
There was a problem hiding this comment.
| // This is needed in browsers where syncronous worker creation is still not | |
| // possible: <BUG_LINK> | |
| // This is needed in browsers where synchronous worker creation is still not | |
| // possible: <BUG_LINK> |
Do you have a bug to link to?
There was a problem hiding this comment.
I'm hoping @juj might have a link to this?
We don't need/want threads to be `unref` until the worker is up and running. This is important for emscripten-core#26933 since otherwise the node runtime can shut down while a thread is starting up. This change also simplifies the code by limiting the number of places we called `unref` to just one, which also slightly reduces codesize. Split out from emscripten-core#26933
We don't need/want threads to be `unref` until the worker is up and running. This is important for emscripten-core#26933 since otherwise the node runtime can shut down while a thread is starting up. This change also simplifies the code by limiting the number of places we called `unref` to just one, which also slightly reduces codesize. Also, remove unused `worker.loaded` assignment. Split out from emscripten-core#26933
We don't need/want threads to be `unref` until the worker is up and running. This is important for emscripten-core#26933 since otherwise the node runtime can shut down while a thread is starting up. This change also simplifies the code by limiting the number of places we called `unref` to just one, which also slightly reduces codesize. Also, remove unused `worker.loaded` assignment. Split out from emscripten-core#26933
|
Thanks for the PR! This is exactly what I had in mind for Unity's use. However, I feel a bit cautious about this behavior to occur unconditionally for Emscripten in general. Codebases need to be carefully aware where JSPI exists in their codebases. The following aspects come to mind:
(it seems like as a small optimization, we would never want to JSPI pause if a ready Worker does exist in the pool?)
A bit related concern, although not about JSPIfying
Ultimately I wonder if this JSPIfied Maybe we'd have functions This way app developers could optimize certain call sites to explicitly say that "this callsite will not synchronously need to join with the thread, so I can |
|
Thanks for the feedback. I mostly agree with everything you are saying. I do hope we can place some restrictions on the design space here though, and perhaps add settings/refinements in response to realworld user experience. Regarding adding "fast-path" to avoid returning to the event loop in just some cases. We originally had this in the design of JSPI itself, but the web standards folks pushed back on this as a bad design in general and so JSPI entry points will not always go back to event loop. So I think we are limited by this in that for a given JS import it must either be JSPI or not-JSPI. We don't have a sometimes-JSPI option, and I think thats fine. We could still do the seperate API design (e.g. Regarding the cost of going back to the event loop: For thread creation, which is not normally a super high-frequency activity the cost of hitting the event loop seems pretty small to me. JS microtask queue is very fast. The concerns about caller needing to handler Promise-returning export is totally valid, but isn't this a general concern about all JSPI usage? There is nothing unique about pthread_create in this argument right? If your program uses JSPI then you exports will likely be promise-returning. Isn't this just something that the use of JSPI is going to have to deal with in any case? @brendandahl @tlively how realistic is it to have a JSPI-using program what has useful non-promising exports? It seems like as we enable more JSPI usage pretty much all exports are going to end up returning promises. Do we have any kind of mechanism to limit or control this? |
|
Since we already have the Right now JS libraries functions that are marked as |
|
The problem of viral I agree it would be great to make of this behaviour opt-in (or opt-out). (Note: |
You can get this sometimes-JSPI behavior by not using JSPI for the import, having it sometimes return a Promise, then if it does return a Promise, passing that Promise to a JSPI-wrapped identity function, e.g.
Right now I believe this is realistic. We've had large applications tell us that rewriting the entire Wasm/JS boundary to be async is prohibitively expensive, so they would only be able to use JSPI in a targeted way where they can reason about everything reachable from a specific async export. That being said, for new applications it would be much better to conservatively assume that any Wasm can suspend by default. Perhaps we want one mode where JSPI is enabled, but only used for APIs that explicitly mention it (e.g. |
Presumably this means we want to some way to opt into JSPI on a per-function basis? Something like My feeling is that these should all be on-by-default. i.e. if you just add |
|
I wonder if we could try to use some the code from the original asyncify that tries to work backwards from |
We don't need/want threads to be `unref` until the worker is up and running. This is important for #26933 since otherwise the node runtime can shut down while a thread is starting up. This change also simplifies the code by limiting the number of places we called `unref` to just one, which also slightly reduces codesize. Also, remove unused `worker.loaded` assignment. Split out from #26933
This means that users of ASYNCIFY/JSPI no longer need to worry about setting
PTHREAD_POOL_SIZE. We can do the same for pthread_join in a followup I think.See: #9910