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] Add MessageChannel between browser thread and new pthreads #70908

Merged
merged 35 commits into from
Jul 1, 2022

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented Jun 17, 2022

Add a mechanism for creating a communication channel between the browser thread and new pthreads (running in webworkers - note, Emscripten may recycle workers after a pthread exits).

This is done by adding our own event listener to the webworker (in the main thread) and to globalThis (in the worker). This conflicts with emscripten's message handlers (although it's considered a bug by Emscripten upstream that their event handler doesn't ignore unrecognized messages).

One potential problem here is that for any communication to happen, the worker must service its event loop. If it's just busy running a loop in wasm, it might never handle the messages. On the other hand, posting messages back to main should work.

Once we have our message handlers in place, the rest is straightforward, the worker creates a MessageChannel and transfers one of the ports to the browser thread.

With the browser-to-pthread channel established, we can build up cross-thread channels by asking the main thread to transfer ports on our behalf. This part isn't done yet.

@ghost
Copy link

ghost commented Jun 17, 2022

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

Issue Details

Add a mechanism for creating a communication channel between the browser thread and new pthreads (running in webworkers - note, Emscripten may recycle workers after a pthread exits).

This is done by adding our own event listener to the webworker (in the main thread) and to globalThis (in the worker). This conflicts with emscripten's message handlers (although it's considered a bug by Emscripten upstream that their event handler doesn't ignore unrecognized messages).

The downside of the current approach is that we need to run some async work on the main thread (because otherwise there's no good way to get notified when Emscripten spins up a new worker - the mechanism isn't extensible). So if the main thread isn't servicing Emscripten's async work queue, we might block.

Once we have our message handlers in place, the rest is straightforward, the worker creates a MessageChannel and transfers one of the ports to the browser thread.

Right now the messagePort is just dropped on the floor, but the right thing is to keep a dictionary of them in the main thread. This isn't done yet.

With the browser-to-pthread channel established, we can build up cross-thread channels by asking the main thread to transfer ports on our behalf. This part isn't done yet, either.

Author: lambdageek
Assignees: lambdageek
Labels:

arch-wasm, area-VM-meta-mono, area-Threading-mono

Milestone: -

@lambdageek lambdageek added this to the 7.0.0 milestone Jun 17, 2022
@lambdageek

This comment was marked as resolved.

@lambdageek lambdageek force-pushed the wasm-mt-channels branch 2 times, most recently from 6705c84 to 662e0f6 Compare June 19, 2022 19:04
@lambdageek lambdageek marked this pull request as ready for review June 20, 2022 17:57
@lambdageek lambdageek requested review from pavelsavara and kg June 20, 2022 17:57
@lambdageek lambdageek force-pushed the wasm-mt-channels branch 2 times, most recently from 989c721 to a87e135 Compare June 21, 2022 02:20
lambdageek and others added 4 commits June 24, 2022 09:24
Use it to call mono_wasm_pthread_on_pthread_created

Rename the previous callback to mono_wasm_pthread_on_pthread_attached
- it gets called but it's unused.

This is enough to get the diagnostic sever worker started up and
pinging.
@lambdageek
Copy link
Member Author

Cleaned up a little bit: I now have a "on created" and "on attached" calls - the first is called from Emscripten's PThread["threadInit"] function which initializes a worker once a pthread is assgned to it; the latter "on attached" is called when the thread beings to interact with the Mono runtime. This means we can now notice when third party libraries call pthread_create (which might turn out ot be useful). Also the diagnostic server thread can now be started before the native mono runtime is initialized sufficiently to create threads.

@pavelsavara
Copy link
Member

pavelsavara commented Jun 29, 2022

could we perhaps have something like pattern EventTarget named onCurrentThreadCreated.

And subscribers would call onCurrentThreadCreated.addEventListener(event=>{ mysomething(event.pthread_ptr) })
and the emscripten would call onCurrentThreadCreated.dispatchEvent()

@lambdageek
Copy link
Member Author

done.

subsystems in the runtime can now do:

import { currentWorkerThreadEvents, dotnetPthreadCreated, WorkerThreadEvent } from './pthreads/worker';

currentWorkerThreadEvents.addEventListener(dotnetPthreadCreated, (ev: WorkerThreadEvent) => {
  /* do stuff with ev.pthread_ptr */
});

this lets pthread lifecycle event handlers post messages and setup
listeners on the message port back to the main browser thread.

Also update the README to describe the current design
@radical
Copy link
Member

radical commented Jun 29, 2022

Trimming tests failing:

  console.error: failed to load the dotnet.js file.
  ReferenceError: CustomEvent is not defined
      at /__w/1/s/artifacts/bin/trimmingTests/projects/System.Data.Common.TrimmingTests/CreateSqlXmlReader/browser-wasm/bin/Release/net7.0/browser-wasm/AppBundle/dotnet.js:3:75595
      at /__w/1/s/artifacts/bin/trimmingTests/projects/System.Data.Common.TrimmingTests/CreateSqlXmlReader/browser-wasm/bin/Release/net7.0/browser-wasm/AppBundle/dotnet.js:3:112494

@lambdageek
Copy link
Member Author

Trimming tests failing:

  console.error: failed to load the dotnet.js file.
  ReferenceError: CustomEvent is not defined
      at /__w/1/s/artifacts/bin/trimmingTests/projects/System.Data.Common.TrimmingTests/CreateSqlXmlReader/browser-wasm/bin/Release/net7.0/browser-wasm/AppBundle/dotnet.js:3:75595
      at /__w/1/s/artifacts/bin/trimmingTests/projects/System.Data.Common.TrimmingTests/CreateSqlXmlReader/browser-wasm/bin/Release/net7.0/browser-wasm/AppBundle/dotnet.js:3:112494

Yea Im' working on it.

There's two issues:

  1. WorkerThreadEvent isn't getting removed from builds without threads. I'm working on making rollup remove it.
  2. Node doesn't have CustomEvent. Hopefully (1) will fix it, but I might just switch to Event instead which should be in recent Nodes

@pavelsavara
Copy link
Member

LGTM

lambdageek and others added 2 commits June 29, 2022 16:45
rollup doesn't understand fallthru.  In the following (which is what
`mono_assert` ammounts to) it retains `C`:

```
if (condition)
  throw new Error (...);
// fallthru
return new C();
```

Solution is to use an if-then-else

```
if (condition)
  throw new Error (...);
else
  return new C(); // C is not retained if 'condition' is false
```
@lambdageek
Copy link
Member Author

lambdageek commented Jun 29, 2022

Update filed an issue about it rollup/rollup#4542

weird, apparently rollup doesn't understand that if condition is false then in the following it doesn't need to retain C

import condition from "consts:condition";

class C {
  constructor () {}
  ...
}

export function makeC () {
  if (!condition)
    throw new Error ("condition is false");
  // fallthru
  return new C(); // rollup incorrectly retains C
}

It rewrites the function to always throw, but still keeps the definition of C:

class C {
  constructor() { }
  ...
}
export function makeC() {
   throw new Error ("condition is false");
}

However this works:

import condition from "consts:condition";

class C {
  constructor () {}
  ...
}

export function makeC () {
  if (!condition)
    throw new Error ("condition is false");
  else
     return new C(); // rollup doesn't retain C
}

@lambdageek
Copy link
Member Author

v8 shell doesn't have Event and EventTarget as far as I can tell. Not sure how much we care about using threads with v8, but I added some poor polyfills in case we need them.

@lambdageek lambdageek merged commit 0577227 into dotnet:main Jul 1, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jul 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-VM-threading-mono
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants