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] Support async JS interop on threadpool threads #84494

Conversation

lambdageek
Copy link
Member

@lambdageek lambdageek commented Apr 7, 2023

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:

  1. 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)
  2. 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)
  3. 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.
  4. A smoke test that does a background JS fetch and some async callbacks on it.

@ghost ghost assigned lambdageek Apr 7, 2023
@lambdageek 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
@lambdageek 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 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
@lambdageek lambdageek force-pushed the pieces-wasm-threadpool-worker-4-async-threadpool-worker branch from 4ee3c94 to c919bbe Compare April 19, 2023 19:01
@lambdageek lambdageek marked this pull request as ready for review April 19, 2023 19:07
@lambdageek lambdageek added the arch-wasm WebAssembly architecture label Apr 19, 2023
@ghost
Copy link

ghost commented Apr 19, 2023

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:

  1. 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)
  2. 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.
  3. 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
Copy link
Member Author

@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

@lambdageek
Copy link
Member Author

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

@lambdageek
Copy link
Member Author

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

Copy link
Member

@pavelsavara pavelsavara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the G_UNLIKELY here important or necessary? Just curious

Copy link
Member Author

@lambdageek lambdageek Apr 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@lateralusX lateralusX Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@kg kg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks right to me based on my understanding of the codebase

@lambdageek lambdageek force-pushed the pieces-wasm-threadpool-worker-4-async-threadpool-worker branch from 2a3d7af to 17a3d73 Compare April 28, 2023 01:31
/* Run the actual main function of the thread */
res = start_wrapper_internal (start_info, (gsize*)info->stack_end);

if (G_UNLIKELY (external_eventloop)) {
Copy link
Member

@lateralusX lateralusX Apr 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@lateralusX lateralusX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
@lambdageek lambdageek merged commit 7ddc45a into dotnet:main Apr 28, 2023
@ghost ghost locked as resolved and limited conversation to collaborators May 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants