[wasm-mt] A version of LowLevelLifoSemaphore that uses callbacks on the browser #84491

lambdageek · 2023-04-07T19:45:27Z

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

This PR adds two things:

A new unmanaged LifoSemaphoreAsyncWait semaphore that can use Emscripten's ability to push C calls from one thread to another in order to implement a callback-based semaphore - when a thread wants to wait, it sets up a success callback and a timeout callback, and then can return to the JS event loop. When the semaphore is released, Emscripten will trigger the callback to run on the waiting thread. If the wait times out, the timeout callback will run.
A new flavor of the managed LowLevelLifoSemaphore that is not allowed to use the normal Wait() function, and instead needs to use the callback-based PrepareAsyncWait() function.

ghost · 2023-04-07T19:46:15Z

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

Issue Details

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

This PR adds two things:

A new unmanaged LifoSemaphoreAsyncWait semaphore that can use Emscripten's ability to push C calls from one thread to another in order to implement a callback-based semaphore - when a thread wants to wait, it sets up a success callback and a timeout callback, and then can return to the JS event loop. When the semaphore is released, Emscripten will trigger the callback to run on the waiting thread. If the wait times out, the timeout callback will run.
A new flavor of the managed LowLevelLifoSemaphore that is not allowed to use the normal Wait() function, and instead needs to use the callback-based PrepareAsyncWait() function.

Author:	lambdageek
Assignees:	lambdageek
Labels:	`arch-wasm`, `area-VM-threading-mono`, `needs-area-label`
Milestone:	-

src/libraries/System.Private.CoreLib/src/System/Threading/LowLevelLifoSemaphore.cs

lambdageek · 2023-04-12T14:30:52Z

Not sure who should look at the native C bits here.

@jkoritzinsky is it something you could take a look at? @kg, @pavelsavara, @lateralusX maybe you also?

jkoritzinsky · 2023-04-12T14:47:11Z

I don't have a lot of familiarity with this portion of the Mono code base. I can take a look, but someone from Mono that works on WASM should definitely be the main reviewer for that component.

kouvel

Just one question, otherwise LGTM. I only did a quick scan over the native parts, probably would be good to have someone familiar with WASM review as well.

src/mono/mono/utils/lifo-semaphore.c

....Private.CoreLib/src/System/Threading/LowLevelLifoAsyncWaitSemaphore.Browser.Threads.Mono.cs

kg · 2023-04-12T23:08:50Z

....Private.CoreLib/src/System/Threading/LowLevelLifoAsyncWaitSemaphore.Browser.Threads.Mono.cs

+    {
+        GCHandle gchandle = GCHandle.FromIntPtr(userData);
+        WaitEntry internalWaitEntry = (WaitEntry)gchandle.Target!;
+        gchandle.Free();


According to the sample in https://learn.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.gchandle.fromintptr?view=net-8.0 I'm not sure it's correct to free the handle here. I think it might be a double-free, unless the caller isn't freeing their handle?

Yea this is different from the example in the docs. In this case the ownership of the GC handle passes from PrepareAsyncWaitCore - which creates the GCHandle, to the underlying native wait structure, to the SuccessCallback or TimeoutCallback. At that point, the callback is the sole owner of the GC Handle (the underlying native wait entry was already freed). So the callback is responsible for freeing the GCHandle.

In the example in the docs, the call to EnumWindows never owns the GCHandle - it is always owned by the calling function, which is responsible for freeing it.

src/mono/System.Private.CoreLib/src/System/Threading/LowLevelLifoSemaphore.Unix.Mono.cs

kg · 2023-04-12T23:33:58Z

src/mono/mono/utils/lifo-semaphore.c

@@ -16,7 +24,7 @@ void
 mono_lifo_semaphore_delete (LifoSemaphore *semaphore)
 {
 	g_assert (semaphore->head == NULL);


does this need to occur inside the lock?

I don't think so. This gets called from DeleteInternal which is, in turn, called from IDisposable.Dispose for the managed object. So at that point we don't expect contention on the semaphore because there should be nothing in managed code that keeps the semaphore alive.

(The reality is I think it's probably difficult to call Dispose correctly in managed - except maybe from some finalizer. For the threadpool in particular, i'm pretty sure it's never actually called at all. The semaphore lives for the lifetime of the runtime)

src/libraries/System.Private.CoreLib/src/System/Threading/LowLevelLifoSemaphore.cs

lambdageek · 2023-04-19T16:20:30Z

WBT failures are due to #85010

....Private.CoreLib/src/System/Threading/LowLevelLifoAsyncWaitSemaphore.Browser.Threads.Mono.cs

kouvel

LGTM, thanks!

lambdageek · 2023-04-20T13:32:34Z

@kg @kouvel could I get a sign-off

Add a new kind of LifoSemaphore that has a callback-based wait function, instead of a blocking wait using Emscripten's ability to send work from one webworker to another in C. This will allow us to wait for a semaphore from the JS event loop in a web worker.

A normal LowLevelLifoSemaphore that can do a synchronous wait and another that can do a callback-based wait from the JS event loop

Move the counts to the base class Move Release to the base class, make ReleaseCore abstract

instead of magic constants that are otherwise not needed in managed

…areExchange When a thread wakes after waiting for a semaphore to be released, if it raced with another thread that is also trying to update the semaphore counts and loses, it has to go back to waiting again. In that case, decrement the remaining timeout by the elapsed wait time so that the next wait is shorter.

make PrepareAsyncWaitCore static and remove a redundant argument

lambdageek · 2023-04-21T14:04:01Z

Got an ok from @kg offline.

lambdageek requested review from vargaz, lateralusX, thaystg and marek-safar as code owners April 7, 2023 19:45

dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Apr 7, 2023

ghost assigned lambdageek Apr 7, 2023

lambdageek mentioned this pull request Apr 7, 2023

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

Closed

lambdageek added arch-wasm WebAssembly architecture area-VM-threading-mono and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 7, 2023

lambdageek mentioned this pull request Apr 7, 2023

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

Merged

lambdageek requested review from kg, pavelsavara and kouvel April 7, 2023 20:15

jkoritzinsky reviewed Apr 7, 2023

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Threading/LowLevelLifoSemaphore.cs Outdated Show resolved Hide resolved

lambdageek requested review from lewing and MichalStrehovsky as code owners April 8, 2023 03:22

lambdageek force-pushed the pieces-wasm-threadpool-worker-2-async-lifo-semaphore branch 2 times, most recently from 98bd2ec to 40fb41c Compare April 8, 2023 05:17

This comment was marked as resolved.

Sign in to view

kouvel reviewed Apr 12, 2023

View reviewed changes

src/mono/mono/utils/lifo-semaphore.c Outdated Show resolved Hide resolved