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.

Sign up for GitHub

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

Merged
merged 18 commits into from
Apr 28, 2023
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
8c3fef3
[wasm-mt] Full JSInterop on threadpool workers
lambdageek Apr 7, 2023
a3db886
[wasm-mt] Add background interop to smoketest
lambdageek Apr 7, 2023
847ecd5
update to use the LowLevelLifoAsyncWaitSemaphore
lambdageek Apr 8, 2023
dd69c8f
adjust to renamed PortableThreadPool helper methods
lambdageek Apr 18, 2023
451d21b
adjust to renamed WebWorkerEventLoop.HasJavaScriptInteropDependents
lambdageek Apr 19, 2023
2889a1f
extend and rationalize the smoke test a bit
lambdageek Apr 19, 2023
92d80a5
remove old-Emscripten workaround hack
lambdageek Apr 19, 2023
5f4acc8
hide some debug output
lambdageek Apr 20, 2023
806b770
smoke test: dispose of the ImportAsync result after the task is done
lambdageek Apr 20, 2023
d2811f7
[wasm-mt] make JSHostImplementation.s_csOwnedObjects ThreadStatic
lambdageek Apr 20, 2023
48d22ae
remove locking on JSHostImplementation.CsOwnedObjects
lambdageek Apr 21, 2023
c813701
Merge remote-tracking branch 'origin/main' into pieces-wasm-threadpoo…
lambdageek Apr 24, 2023
0a4dd8c
Merge remote-tracking branch 'origin/main' into pieces-wasm-threadpoo…
lambdageek Apr 27, 2023
77a54ff
[threads] make the "external eventloop" platform independent
lambdageek Apr 27, 2023
fd99953
fix wasi and singlethreaded browser-wasm
lambdageek Apr 27, 2023
4671e03
Add a Thread.HasExternalEventLoop managed property
lambdageek Apr 27, 2023
e22ca46
rename JSHostImplementation.ThreadCsOwnedObjects
lambdageek Apr 27, 2023
c591501
[checked] assert GC Safe mode, when returning to external eventloop
lambdageek Apr 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/mono/mono/metadata/threads-types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,10 @@ typedef enum {
MONO_THREAD_CREATE_FLAGS_DEBUGGER = 0x02,
MONO_THREAD_CREATE_FLAGS_FORCE_CREATE = 0x04,
MONO_THREAD_CREATE_FLAGS_SMALL_STACK = 0x08,
#if defined(HOST_BROWSER) && !defined(DISABLE_THREADS)
MONO_THREAD_CREATE_FLAGS_RETURNS_TO_JS_EVENT_LOOP = 0x10,
#endif
// "external eventloop" means the thread main function can return without killing the thread
// and the thread will continue to be attached to the runtime and may invoke embedding APIs
// and managed calls. There is usually some platform-specific way to shut down the thread.
MONO_THREAD_CREATE_FLAGS_EXTERNAL_EVENTLOOP = 0x10,
} MonoThreadCreateFlags;

MONO_COMPONENT_API MonoInternalThread*
Expand Down
32 changes: 10 additions & 22 deletions src/mono/mono/metadata/threads.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1088,9 +1088,7 @@ typedef struct {
MonoThreadStart start_func;
gpointer start_func_arg;
gboolean force_attach;
#if defined(HOST_BROWSER) && !defined(DISABLE_THREADS)
gboolean returns_to_js_event_loop;
#endif
gboolean external_eventloop;
gboolean failed;
MonoCoopSem registered;
} StartInfo;
Expand Down Expand Up @@ -1176,9 +1174,7 @@ start_wrapper_internal (StartInfo *start_info, gsize *stack_ptr)
/* Let the thread that called Start() know we're ready */
mono_coop_sem_post (&start_info->registered);

#if defined(HOST_BROWSER) && !defined(DISABLE_THREADS)
gboolean returns_to_js_event_loop = start_info->returns_to_js_event_loop;
#endif
gboolean external_eventloop = start_info->external_eventloop;

if (mono_atomic_dec_i32 (&start_info->ref) == 0) {
mono_coop_sem_destroy (&start_info->registered);
Expand Down Expand Up @@ -1247,13 +1243,11 @@ start_wrapper_internal (StartInfo *start_info, gsize *stack_ptr)

THREAD_DEBUG (g_message ("%s: (%" G_GSIZE_FORMAT ") Start wrapper terminating", __func__, mono_native_thread_id_get ()));

#if defined(HOST_BROWSER) && !defined(DISABLE_THREADS)
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 •
edited
Loading

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 •
edited
Loading

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.

/* if the thread wants to stay alive in an external eventloop, don't clean up after it */
if (mono_thread_platform_external_eventloop_keepalive_check ())
return 0;
}
#endif

/* Do any cleanup needed for apartment state. This
* cannot be done in mono_thread_detach_internal since
Expand Down Expand Up @@ -1281,19 +1275,15 @@ start_wrapper (gpointer data)
info = mono_thread_info_attach ();
info->runtime_thread = TRUE;

#if defined(HOST_BROWSER) && !defined(DISABLE_THREADS)
gboolean returns_to_js_event_loop = start_info->returns_to_js_event_loop;
#endif
gboolean external_eventloop = start_info->external_eventloop;
/* Run the actual main function of the thread */
res = start_wrapper_internal (start_info, (gsize*)info->stack_end);

#if defined(HOST_BROWSER) && !defined(DISABLE_THREADS)
if (returns_to_js_event_loop) {
if (G_UNLIKELY (external_eventloop)) {
Copy link
Member

@lateralusX lateralusX Apr 28, 2023 •
edited
Loading

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

/* if the thread wants to stay alive, don't clean up after it */
if (emscripten_runtime_keepalive_check())
if (mono_thread_platform_external_eventloop_keepalive_check ())
return 0;
}
#endif

mono_thread_info_exit (res);

Expand Down Expand Up @@ -1381,9 +1371,7 @@ create_thread (MonoThread *thread, MonoInternalThread *internal, MonoThreadStart
start_info->start_func_arg = start_func_arg;
start_info->force_attach = flags & MONO_THREAD_CREATE_FLAGS_FORCE_CREATE;
start_info->failed = FALSE;
#if defined(HOST_BROWSER) && !defined(DISABLE_THREADS)
start_info->returns_to_js_event_loop = (flags & MONO_THREAD_CREATE_FLAGS_RETURNS_TO_JS_EVENT_LOOP) != 0;
#endif
start_info->external_eventloop = (flags & MONO_THREAD_CREATE_FLAGS_EXTERNAL_EVENTLOOP) != 0;
mono_coop_sem_init (&start_info->registered, 0);

if (flags != MONO_THREAD_CREATE_FLAGS_SMALL_STACK)
Expand Down Expand Up @@ -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

if (internal->threadpool_thread)
create_flags |= MONO_THREAD_CREATE_FLAGS_RETURNS_TO_JS_EVENT_LOOP;
create_flags |= MONO_THREAD_CREATE_FLAGS_EXTERNAL_EVENTLOOP;
#endif

res = create_thread (internal, internal, NULL, NULL, stack_size, create_flags, error);
Expand Down
9 changes: 9 additions & 0 deletions src/mono/mono/utils/mono-threads-posix.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,15 @@ mono_threads_platform_exit (gsize exit_code)
pthread_exit ((gpointer) exit_code);
}

gboolean
mono_thread_platform_external_eventloop_keepalive_check (void)
{
/* vanilla POSIX thread creation doesn't support an external eventloop: when the thread main
function returns, the thread is done.
*/
return FALSE;
}

#if HOST_FUCHSIA
int
mono_thread_info_get_system_max_stack_size (void)
Expand Down
10 changes: 10 additions & 0 deletions src/mono/mono/utils/mono-threads-wasm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,6 +301,16 @@ mono_thread_platform_create_thread (MonoThreadStart thread_fn, gpointer thread_d
#endif
}

gboolean
mono_thread_platform_external_eventloop_keepalive_check (void)
{
/* if someone called emscripten_runtime_keepalive_push (), the
* thread will stay alive in the JS event loop after returning
* from the thread's main function.
*/
return emscripten_runtime_keepalive_check ();
}

void mono_threads_platform_init (void)
{
}
Expand Down
9 changes: 9 additions & 0 deletions src/mono/mono/utils/mono-threads-windows.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,6 +501,15 @@ typedef BOOL (WINAPI *LPFN_ISWOW64PROCESS) (HANDLE, PBOOL);
static gboolean is_wow64 = FALSE;
#endif

gboolean
mono_thread_platform_external_eventloop_keepalive_check (void)
{
/* We don't support thread creation with an external eventloop on WIN32: when the thread start
function returns, the thread is done.
*/
return FALSE;
}

/* We do this at init time to avoid potential races with module opening */
void
mono_threads_platform_init (void)
Expand Down
3 changes: 3 additions & 0 deletions src/mono/mono/utils/mono-threads.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -632,6 +632,9 @@ gboolean mono_threads_platform_in_critical_region (THREAD_INFO_TYPE *info);
gboolean mono_threads_platform_yield (void);
void mono_threads_platform_exit (gsize exit_code);

gboolean
mono_thread_platform_external_eventloop_keepalive_check (void);

void mono_threads_coop_begin_global_suspend (void);
void mono_threads_coop_end_global_suspend (void);

Expand Down