[wasm-mt] Support async JS interop on threadpool threads #84494

lambdageek · 2023-04-07T19:58:40Z

This is Part 4 of #84489 - initial runtime support for async JS interop on threadpool threads in multi-threaded WebAssembly.

This depends on Part1, Part2, Part3 and the Emscripten Bump PR.

This is equivalent to the abandoned #83838.

Conceptually there are a few things here:

A mechanism for the native runtime to start some threads as "exitable" so that we don't clean up when they return from their thread start function. (we will clean up when the pthreads TLS dtor runs)
A change to make JSHostImplementation.s_csOwnedObjects a thread-static in multithreaded builds. This is because the map keys on a small integer handle that is allocated per-thread on the JS side as a handle for JS objects that are to be kept alive because managed code depends on them. (This is needed in general, but also makes the new smoke test work)
A version of the PortableThreadPool.WorkerThread that starts as an exitable thread and uses asynchronous callbacks to wait for available threadpool work items and returns to the JS event loop periodically in order to allow JS promises to settle.
A smoke test that does a background JS fetch and some async callbacks on it.

ghost · 2023-04-19T19:08:19Z

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

Issue Details

This is Part 4 of #84489 - initial runtime support for async JS interop on threadpool threads in multi-threaded WebAssembly.

This depends on Part1, Part2, Part3 and the Emscripten Bump PR. It's not worth reviewing in detail until those other pieces land.

This is equivalent to the abandoned #83838.

Conceptually there are three things here:

A mechanism for the native runtime to start some threads as "exitable" so that we don't clean up when they return from their thread start function. (we will clean up when the pthreads TLS dtor runs)
A version of the PortableThreadPool.WorkerThread that starts as an exitable thread and uses asynchronous callbacks to wait for available threadpool work items and returns to the JS event loop periodically in order to allow JS promises to settle.
A smoke test that does a background JS fetch and some async callbacks on it.

Author:	lambdageek
Assignees:	lambdageek
Labels:	`arch-wasm`, `area-Threading-mono`
Milestone:	-

lambdageek · 2023-04-19T19:29:58Z

@pavelsavara @kg this is probably ok to look at once Part 2 and Part 3 go in.

@vargaz or @lateralusX: could you take a look at the changes in threads.c around create_thread and start_wrapper_internal for the MONO_THREAD_CREATE_FLAGS_RETURNS_TO_JS_EVENT_LOOP stuff

src/mono/wasm/runtime/pthreads/worker/index.ts

…l-worker-4-async-threadpool-worker

lambdageek · 2023-04-27T15:06:55Z

@lateralusX @vargaz When you have time, could please you take a look at the thread creation changes in this PR (threads.c)?

lambdageek · 2023-04-27T15:21:49Z

From Zoltan:

I'd suggest reducing the amount of ifdefs.
adding flags and fields to StartInfo doesn't need an ifdef imho.

From myself:

kind of unhappy with how much Emscripten stuff leaked into threads.c. gonna move some of it out to mono-threads-wasm.c and make some "cross platform" sounding names for it. "has_extern_eventloop" or something

pavelsavara

System.Runtime.InteropServices.JavaScript and TypeScript changes look good to me.
I don't love the messy Wasm.Browser.Threads.Minimal.Sample, but I'm happy to deal with that in the future.

It only does something on WASM, but in principle if other platforms allow us to run some code after returning from a thread start function, we could do it there, too.

kg · 2023-04-27T21:06:55Z

src/mono/mono/metadata/threads.c

-	if (returns_to_js_event_loop) {
-		/* if the thread wants to stay alive, don't clean up after it */
-		if (emscripten_runtime_keepalive_check())
+	if (G_UNLIKELY (external_eventloop)) {


Is the G_UNLIKELY here important or necessary? Just curious

Neither - this code isn't particularly hot (I think), so it's more for documentation than anything else.

It is also a hint to compilers that supports it to put the branch out of line assisting the branch predictors heuristics, so it should then end up with a conditional jmp to an out of line code block when true, but branch predictor will predict it to not be taken (normal default heuristics for forward branches)., potentially improving execution speed around that block.

kg · 2023-04-27T21:07:51Z

src/mono/mono/metadata/threads.c

@@ -4947,7 +4935,7 @@ ves_icall_System_Threading_Thread_StartInternal (MonoThreadObjectHandle thread_h
 	// HACK: threadpool threads can return to the JS event loop
 	// WISH: support this for other threads, too


Maybe this should be TODO or FIXME instead of wish so it's easier to find in codebase audits. I assume we're going to get to it eventually either way though

I should probably just add a managed internal flag on MonoThreadObject and get rid of this hack entirely. I've more or less convinced myself that we will eventually need a wasm-specific API to set something like this on threads created with var t = new Thread(...); t.Start(), anyway

kg

Looks right to me based on my understanding of the codebase

...ropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSHostImplementation.cs

lateralusX · 2023-04-28T07:48:05Z

src/mono/mono/metadata/threads.c

 	/* Run the actual main function of the thread */
 	res = start_wrapper_internal (start_info, (gsize*)info->stack_end);

+	if (G_UNLIKELY (external_eventloop)) {


How will this thread be used after it has returned from its thread function? Will we use it to run other code (JS event loop), "impersonating" the thread, like having events that should run managed code, referencing this thread, so when picked up in JS even loop, it will still be able to complete those events, even if the original thread execution has stopped? Is that the background why we need keeping the managed thread alive as done in this PR? If so, will the thread move over to safe mode since it will still be attached to runtime, but not able to react to any suspend requests etc?

Yes, exactly right.

In order for JS promises to resolve, the JS eventloop must get a chance to run. So if we create a worker thread with Task.Run and await on an imported JS promise (which gets exposed as a Task<T> in C#) the worker thread must be able to run the JS eventloop and then invoke the managed code once the JS promise is finished.

The earlier PRs (see the PR description) refactored the managed threadpool worker code to use callbacks that are registered with the JS event loop in order to signal a threadpool worker when it has work available. The worker stays alive as long as there are unsettled JS promises (that are awaited by managed code) and then either exits (normal pthread_exit shuts down these "external eventloop" pthreads same as normal pthreads) or picks up additional work - depending on what the threadpool algorithm says.

If so, will the thread move over to safe mode since it will still be attached to runtime, but not able to react to any suspend requests etc?

yes it should be in GC Safe when it's in the JS loop (I'll do a checked build this afternoon to verify. but I'm pretty sure that actually happens as part of the normal thread state management for the managed thread start function invocation. when it returns we will go to gc safe)

In order for JS promises to resolve, the JS eventloop must get a chance to run. So if we create a worker thread with Task.Run and await on an imported JS promise (which gets exposed as a Task<T> in C#) the worker thread must be able to run the JS eventloop and then invoke the managed code once the JS promise is finished.

So then we keep a reference count or similar making sure the managed thread gets detached when there are no more outstanding references to the thread, correct? Sounded like we depend on TLS destructors to do that, so does code reacting to callback from JS eventloop, invoke managed code and then check refcount and if that is 0, close native thread, that in turn will trigger TLS destructors, detaching thread from runtime?

Added a gc safe assertion when we're exiting to the external event loop

lateralusX

Changes to native code LGTM!

Set it from WebWorkerEventLoop.StartExitable. In native code, use it to set the `MONO_THREAD_CREATE_FLAGS_EXTERNAL_EVENTLOOP` flag when starting the thread.

(used to be CsOwnedObjects) Rename to make it clear that it's objects owned by the current thread, not the runtime globally

dotnet-issue-labeler bot added the area-Threading-mono label Apr 7, 2023

ghost assigned lambdageek Apr 7, 2023

This was referenced Apr 7, 2023

[wasm-mt] Land the webworker JSImport support #84489

Closed

[wasm-mt] Allow JSImport promises to run on managed threadpool worker threads #83838

Closed

lambdageek force-pushed the pieces-wasm-threadpool-worker-4-async-threadpool-worker branch 3 times, most recently from 2cd0abf to 15f5aa9 Compare April 8, 2023 05:47

runfoapp bot mentioned this pull request Apr 10, 2023

Long Running Test: Interop/MonoAPI/MonoMono/PInvokeDetach/PInvokeDetach.sh #73040

Closed

lambdageek force-pushed the pieces-wasm-threadpool-worker-4-async-threadpool-worker branch 2 times, most recently from 7f03148 to 5aec84b Compare April 11, 2023 14:40

lambdageek force-pushed the pieces-wasm-threadpool-worker-4-async-threadpool-worker branch 2 times, most recently from 5af30a0 to 4ee3c94 Compare April 18, 2023 18:34

build-analysis bot mentioned this pull request Apr 19, 2023

WasmTestOnBrowser-System.Text.Json.Tests.WorkItemExecution timing out #84434

Closed

lambdageek mentioned this pull request Apr 19, 2023

[browser][MT] Keep track of additional JS keepalive sources for worker threads #85052

Closed

lambdageek force-pushed the pieces-wasm-threadpool-worker-4-async-threadpool-worker branch from 4ee3c94 to c919bbe Compare April 19, 2023 19:01

lambdageek marked this pull request as ready for review April 19, 2023 19:07

lambdageek requested review from lewing, pavelsavara, kg, vargaz, lateralusX, thaystg, marek-safar and MichalStrehovsky as code owners April 19, 2023 19:07

lambdageek added the arch-wasm WebAssembly architecture label Apr 19, 2023

pavelsavara reviewed Apr 20, 2023

View reviewed changes

src/mono/wasm/runtime/pthreads/worker/index.ts Outdated Show resolved Hide resolved

build-analysis bot mentioned this pull request Apr 20, 2023

[wasm] interpreter timeouts when WebSocket closes unexpectedly #84101

Closed

lambdageek force-pushed the pieces-wasm-threadpool-worker-4-async-threadpool-worker branch from 8c07676 to 53329df Compare April 20, 2023 15:23

Merge remote-tracking branch 'origin/main' into pieces-wasm-threadpoo…

c813701

…l-worker-4-async-threadpool-worker

This was referenced Apr 24, 2023

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

IOException running NuGet-Migrations during tests in dotnet CLI first run #80619

Closed

Various WASM timeouts on CI #85304

Closed

Merge remote-tracking branch 'origin/main' into pieces-wasm-threadpoo…

0a4dd8c

…l-worker-4-async-threadpool-worker

pavelsavara approved these changes Apr 27, 2023

View reviewed changes

This was referenced Apr 27, 2023

Tracking issue for CI build timeouts #76454

Closed

[Wasm.Build.Tests] Could not restore Microsoft.NET.Workload.Emscripten.Current.Manifest #85456

Closed

[threads] make the "external eventloop" platform independent

77a54ff

It only does something on WASM, but in principle if other platforms allow us to run some code after returning from a thread start function, we could do it there, too.

kg reviewed Apr 27, 2023

View reviewed changes

kg approved these changes Apr 27, 2023

View reviewed changes

kg reviewed Apr 27, 2023

View reviewed changes

...ropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSHostImplementation.cs Outdated Show resolved Hide resolved

fix wasi and singlethreaded browser-wasm

fd99953

lambdageek force-pushed the pieces-wasm-threadpool-worker-4-async-threadpool-worker branch from 2a3d7af to 17a3d73 Compare April 28, 2023 01:31

lateralusX reviewed Apr 28, 2023

View reviewed changes

lateralusX approved these changes Apr 28, 2023

View reviewed changes

lambdageek added 2 commits April 28, 2023 08:05

Add a Thread.HasExternalEventLoop managed property

4671e03

Set it from WebWorkerEventLoop.StartExitable. In native code, use it to set the `MONO_THREAD_CREATE_FLAGS_EXTERNAL_EVENTLOOP` flag when starting the thread.

rename JSHostImplementation.ThreadCsOwnedObjects

e22ca46

(used to be CsOwnedObjects) Rename to make it clear that it's objects owned by the current thread, not the runtime globally

lambdageek force-pushed the pieces-wasm-threadpool-worker-4-async-threadpool-worker branch 2 times, most recently from 5f523f6 to e22ca46 Compare April 28, 2023 12:05

This was referenced Apr 28, 2023

Roslyn source generator crash on mono/linux/arm64 #81123

Closed

System.Security.Cryptography.Tests CertTests.RaceDisposeAndKeyAccess failing #85073

Closed

[checked] assert GC Safe mode, when returning to external eventloop

c591501

lambdageek merged commit 7ddc45a into dotnet:main Apr 28, 2023

lambdageek mentioned this pull request May 1, 2023

[browser][MT] Multithreading and JavaScript async interop in .NET 9 #85592

Closed

31 tasks

ghost locked as resolved and limited conversation to collaborators May 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wasm-mt] Support async JS interop on threadpool threads #84494

[wasm-mt] Support async JS interop on threadpool threads #84494

lambdageek commented Apr 7, 2023 •

edited

Loading

ghost commented Apr 19, 2023

lambdageek commented Apr 19, 2023

lambdageek commented Apr 27, 2023

lambdageek commented Apr 27, 2023

pavelsavara left a comment

kg Apr 27, 2023

lambdageek Apr 27, 2023 •

edited

Loading

lateralusX Apr 28, 2023 •

edited

Loading

kg Apr 27, 2023

lambdageek Apr 27, 2023

kg left a comment

lateralusX Apr 28, 2023 •

edited

Loading

lambdageek Apr 28, 2023

lambdageek Apr 28, 2023

lateralusX Apr 28, 2023

lambdageek Apr 28, 2023

lambdageek Apr 28, 2023

lateralusX left a comment

		@@ -4947,7 +4935,7 @@ ves_icall_System_Threading_Thread_StartInternal (MonoThreadObjectHandle thread_h
		// HACK: threadpool threads can return to the JS event loop
		// WISH: support this for other threads, too

[wasm-mt] Support async JS interop on threadpool threads #84494

[wasm-mt] Support async JS interop on threadpool threads #84494

Conversation

lambdageek commented Apr 7, 2023 • edited Loading

ghost commented Apr 19, 2023

lambdageek commented Apr 19, 2023

lambdageek commented Apr 27, 2023

lambdageek commented Apr 27, 2023

pavelsavara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdageek Apr 27, 2023 • edited Loading

Choose a reason for hiding this comment

lateralusX Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kg left a comment

Choose a reason for hiding this comment

lateralusX Apr 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lateralusX left a comment

Choose a reason for hiding this comment

lambdageek commented Apr 7, 2023 •

edited

Loading

lambdageek Apr 27, 2023 •

edited

Loading

lateralusX Apr 28, 2023 •

edited

Loading

lateralusX Apr 28, 2023 •

edited

Loading