Skip to content
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

[wasm-mt] Publish dotnet.worker.js #68509

Closed
lambdageek opened this issue Apr 25, 2022 · 13 comments · Fixed by #73697
Closed

[wasm-mt] Publish dotnet.worker.js #68509

lambdageek opened this issue Apr 25, 2022 · 13 comments · Fixed by #73697
Assignees
Labels
arch-wasm WebAssembly architecture area-Build-mono
Milestone

Comments

@lambdageek
Copy link
Member

lambdageek commented Apr 25, 2022

The WasmApp.targets should place dotnet.worker.js next to dotnet.js

There may also be an issue with Blazor dotnet publish doing likewise.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 25, 2022
@lambdageek lambdageek changed the title Publish dotnet.worker.js [wasm-mt] Publish dotnet.worker.js Apr 25, 2022
@lambdageek lambdageek added the arch-wasm WebAssembly architecture label Apr 25, 2022
@ghost
Copy link

ghost commented Apr 25, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

The WasmApp.targets should place dotnet.worker.js next to dotnet.js

There may also be an issue with Blazor dotnet publish doing likewise.

Author: lambdageek
Assignees: -
Labels:

arch-wasm, untriaged

Milestone: -

@lewing lewing added this to the 7.0.0 milestone Apr 29, 2022
@lewing lewing removed the untriaged New issue has not been triaged by the area owner label Apr 29, 2022
@pavelsavara
Copy link
Member

We probably need a way how to customize it.

  • so that it's compatible with ES6 when loading the dotnet.js
  • so that it's compatible our module exports, which are not Module object
  • so that it's CSP compliant, no eval
  • so that it doesn't use require in NodeJs + ES6

@lewing
Copy link
Member

lewing commented Jul 26, 2022

I'd love to have consistent naming pattern for all the workers and we need to make sure that the blazor publish step includes them in the output.

cc @radical @javiercn

@radical
Copy link
Member

radical commented Jul 27, 2022

@javiercn if we populate a msbuild item with the files to deploy (not exhaustive), then would you be able to use that to ensure that they get deployed wherever needed?

@javiercn
Copy link
Member

@radical yes. that would help.

@lewing I set up some time with Ankit to go through the details.

@lewing
Copy link
Member

lewing commented Jul 27, 2022

It is worth reading through #48016 as well

@lambdageek
Copy link
Member Author

I'd love to have consistent naming pattern for all the workers and we need to make sure that the blazor publish step includes them in the output.

renaming dotnet.worker.js is possible, if that's what we want to do - it looks like Emscripten will go through locateFile to get the URL to load. (going the other way - when dotnet.worker.js imports dotnet.js - it goes through the mainUrlOrBlob property on the module - which we do set up : anyModule.mainScriptUrlOrBlob = replacements.scriptUrl; // this is needed by worker threads

tl;dr if we want the published app to use different/consistent naming for worker JS files, we can do that.

@lambdageek
Copy link
Member Author

lambdageek commented Aug 1, 2022

We're going to need to install a replacement for PThread.allocateUnusedWorker (here's the default one):

 allocateUnusedWorker: function() {
  if (!Module["locateFile"]) {
   PThread.unusedWorkers.push(new Worker(new URL("dotnet.worker.js", import.meta.url)));
   return;
  }
  var pthreadMainJs = locateFile("dotnet.worker.js");
  PThread.unusedWorkers.push(new Worker(pthreadMainJs));
 },

We should use the new js-module-threads asset type and callresolve_module_url to get the worker URL. see #73073 (comment)


Note that emscripten pre-allocates at startup by calling

PThread.init ();

in its giant global initialization (before it calls the Module["preInit"] callbacks. So we would need to also replace PThread.init and make it async in order to wait until the asset machinery is setup.

@lambdageek
Copy link
Member Author

lambdageek commented Aug 10, 2022

Replacing Module["PThread"]["init"] is not really feasible - emscripten calls PThread.init very early - before our replacement code runs.

On the bright side PThread.init just calls PThread.initMainThread or PThread.initWorker. initWorker is fine as is. initMainThread doesn't do anything other than call allocateUnusedWorker based on the size of PTHREAD_POOL_SIZE.

So we could:

  1. set -s PTHREAD_POOL_SIZE=0 in wasm.proj
  2. make our own pthreadPreallocatePool function that we can call during startup, based on some MONO_PTHREAD_POOL_SIZE property that we define.
  3. replace allocateUnusedWorker with one that uses resolve_asset_path (from assets.ts). the replacement is too late for PThread.init, but that doesn't matter since it won't be allocating any workers.

I'm going to give it a go

@lambdageek lambdageek self-assigned this Aug 10, 2022
@pavelsavara
Copy link
Member

The way how to resolve the asset is now merged

        const asset = resolve_asset_path("js-module-threads");
        mono_assert(asset && asset.resolvedUrl, "Can't find js-module-threads");
        const worker = new Worker(asset.resolvedUrl);

You will also need to add it to the mono-config.json via src\tasks\WasmAppBuilder\WasmAppBuilder.cs

config.Assets.Add(new ThreadsWorkerEntry ("dotnet.worker.js") );

@pavelsavara
Copy link
Member

This is what SDK needs to do https://github.com/dotnet/sdk/pull/26966/files

@lambdageek
Copy link
Member Author

yep, I have it basically working. PR soon

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 10, 2022
@radical radical removed their assignment Aug 10, 2022
lambdageek added a commit that referenced this issue Aug 14, 2022
…er (#73697)

Enables using asset loading to get the `dotnet.worker.js` file that provides the emscripten pthread worker code.
Also allows specifying the number of pre-allocated workers that will be created at startup using MSBuild properties.

Fixes #68509 and fixes #68397 and fixes #72606

- Override Emscripten `PThread.allocateUnusedWorker`
   We want to use our own allocateUnusedWorker because we want to load
   `dotnet.worker.js` using our asset loading machinery.

   Unfortunately, Emscripten first calls allocateUnusedWorker very early (from
   `PThread.init`) to pre-allocate the pthread worker pool.

   So we set Emscripten's own pthread worker pool to size 0 and make our own.  This requires calling `loadWasmModuleToWorker` during our startup because Emscripten deletes their code that normally does it (in "receiveInstance" in "createWasm" in "emscripten/src/preamble.js") when the pthread pool size is 0.

   Also added a pthreadPoolSize field to MonoConfig to allow specifying the initial pthread pool size in mono-config.json

- Add `IncludeThreadsWorker` and `PThreadPoolSize` props to WasmAppBuilder

   `IncludeThreadsWorker` adds the `"js-module-threads"` asset to the `mono-config.json`

   `PThreadPoolSize` can be -1 or >=0 to specify the number of workers that will be pre-allocated at startup for the pthread worker pool.  -1 means use the default compiled into `dotnet.js`

- Reorganize the pthreads TS code by moving `Internals` (access API that digs through Emscripten's pthreads implementation) to its own module. And add types.

- Replace emscripten's `allocateUnusedWorker` function with our own that goes through the asset loading API.

- Update samples

- Set up console proxying for the workers.
   This is done by sending a message from the main thread to the pthread workers with the current `MonoConfig` on our
    dedicated channel.  (This means the proxying is setup asynchronously, so if the worker is busy before it receives the message, it may not start redirecting messages right away).

---

* [wasm-mt] Override Emscripten PThread.allocateUnusedWorker

We want to use our own allocateUnusedWorker because we want to load `dotnet.worker.js` using our asset loading machinery.

Unfortunately, Emscripten first calls allocateUnusedWorker very early (from `PThread.init`) to pre-allocate the pthread worker pool.

So we set Emscripten's own pthread worker pool to size 0 and make our own.  This requires calling `loadWasmModuleToWorker` during our startup because Emscripten deletes their code that normally does
it (in "receiveInstance" in "createWasm" in "emscripten/src/preamble.js") when the pthread pool size is 0.

Also added a pthreadPoolSize field to MonoConfig to allow specifying the initial pthread pool size in mono-config.json

* Add IncludeThreadsWorker and PThreadPoolSize props to WasmAppBuilder

IncludeThreadsWorker adds the js-module-threads asset to the mono-config

PThreadPoolSize can be -1 or >=0 to specify the number of workers that will be pre-allocated at startup for the pthread worker pool.  -1 means use the default compiled into dotnet.js

* Move emscripten PThread internals access to a separate module

   and add types

* Load js-module-threads asset in replacement allocateUnusedWorker

* Update samples to explicitly enable threading / perftracing

   Makes the WasmAppBuilder include the threads worker module

* tighten up Internals types

* apply review feedback

* fix import

* Apply suggestions from code review

* proxy pthread worker messages to websocket, if enabled

use a new MonoThreadMessageApplyMonoConfig message to send the MonoConfig from the main thread to each worker when the workers set up the communication channel to the main thread.

then if the diagnosticTracing property is true, redirect the worker console logging to a websocket.

Fixes #72606

Co-authored-by: Marek Fišera <[email protected]>
Co-authored-by: Ankit Jain <[email protected]>
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 14, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants