Skip to content

[browser][MT] pool don't yield to event loop - #94600

Closed
pavelsavara wants to merge 4 commits into
dotnet:mainfrom
pavelsavara:browser_pool_dont_yield
Closed

[browser][MT] pool don't yield to event loop#94600
pavelsavara wants to merge 4 commits into
dotnet:mainfrom
pavelsavara:browser_pool_dont_yield

Conversation

@pavelsavara

@pavelsavara pavelsavara commented Nov 10, 2023

Copy link
Copy Markdown
Member

Context:
Because pthreads are re-used and user code could leave that web worker state in any shape.

  • I would like to prevent user code from interaction with JS state on managed thread pool thread.
  • I also don't need to deal with any async/promise on managed thread pool thread.

And so I could switch to standard unix thread pool. It will make lot of things simpler.

On this PR I also removed managed part of external loop handling around WebWorkerEventLoop.

We will need it again later for dedicated JSWebWorker, but I imagine different API.
Essentially from C# perspective it will be just something like:

var myResult  = await JSWebWorker.Create(async () => {
    var module = await JSHost.ImportAsync("my-module", "../my-module.js");
    return module.MyJsStuff()
})

And worker/thread will be kept alive only until the promise of the callback is not resolved/rejected.
Details of how to do the keep alive plumbing TBD.

@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-VM-threading-mono os-browser Browser variant of arch-wasm labels Nov 10, 2023
@pavelsavara pavelsavara added this to the 9.0.0 milestone Nov 10, 2023
@pavelsavara pavelsavara self-assigned this Nov 10, 2023
@ghost

ghost commented Nov 10, 2023

Copy link
Copy Markdown

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

Issue Details

testing ...

Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-VM-threading-mono, os-browser

Milestone: 9.0.0

@pavelsavara

Copy link
Copy Markdown
Member Author

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@pavelsavara

Copy link
Copy Markdown
Member Author

image

@pavelsavara pavelsavara changed the title [browser][MT] pool dont yield to event loop [browser][MT] pool don't yield to event loop Nov 10, 2023
@pavelsavara

Copy link
Copy Markdown
Member Author

/azp run runtime-wasm

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@lambdageek lambdageek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense

@pavelsavara

pavelsavara commented Nov 15, 2023

Copy link
Copy Markdown
Member Author

I still need ability of all managed threads to receive messages via emscripten_check_mailbox so that I could use emscripten_dispatch_to_thread_async to queue resolve/reject calls from promises of threads which do have JS interop.

The scenario is "use HTTP client from any managed thread".

I'm afraid _emscripten_thread_mailbox_await needs JS event loop :(

https://github.com/emscripten-core/emscripten/blob/94b36c04dd323af066f2c1ab6d13f26eb818eb0b/src/library_pthread.js#L1282

Alternative to this would be SynchronizationContext on each managed thread receiving resolve/reject calls. And that probably needs the same infrastructure.

@pavelsavara

Copy link
Copy Markdown
Member Author

If we can signal waitObject that PortableThreadPool.RegisterWaitForSingleObject() is sleeping on, and then call emscripten_check_mailbox from the managed thread pool thread ...

But we have that dream that UI thread would not be attached Mono thread ... just emscripten C.

Could we pin waitObject enough that it becomes pointer to some POSIX thing ? @lambdageek

@lambdageek

Copy link
Copy Markdown
Member

Could we pin waitObject enough that it becomes pointer to some POSIX thing ?

I need to chase down what it actually is.

I think it's a WaitSubsystem.WaitableObject which has a LowLevelLock in there, which has a LowLevelMonitor which has a native part that is defined in pal_threading.c as:

struct LowLevelMonitor
{
pthread_mutex_t Mutex;
pthread_cond_t Condition;
#ifdef DEBUG
bool IsLocked;
#endif
};

So at some point it does all boil down to posix.

Although the "fun" thing about LowLevelLock and LowLevelMonitor is that they both start out by doing some spin waiting before calling down to posix. so if you call down to the native stuff when the managed layers think the lock is not contested and are in the spin phase, you will break something.

@pavelsavara

Copy link
Copy Markdown
Member Author

in the spin phase, you will break something.

I wanted to simplify stuff, but this is not simpler.

I guess we just abandon this idea of threads without JS event loop.
And rather make all managed thread creation to use MONO_THREAD_CREATE_FLAGS_EXTERNAL_EVENTLOOP. I will open different PR for that.

@lambdageek

lambdageek commented Nov 15, 2023

Copy link
Copy Markdown
Member

in the spin phase, you will break something.

I wanted to simplify stuff, but this is not simpler.

I guess we just abandon this idea of threads without JS event loop. And rather make all managed thread creation to use MONO_THREAD_CREATE_FLAGS_EXTERNAL_EVENTLOOP. I will open different PR for that.

do you mean also for new Thread(ThreadStart) or just for threadpool threads? how will new Thread work - it takes a synchronous callback that never returns until the thread is done.

This waitAsync stuff in Emscripten is pretty new. Are we on a version that includes it already? I thought we were still doing the postMessage-through-main-thread thing. emscripten-core/emscripten#18861
I didn't look through that PR in enough detail to understand how it works - how does the async waiting stuff do anything if the pthread never exits ot the JS event loop? why isn't this wait.value.then(checkMailbox) just piling up unfulfilled promises?

@pavelsavara

pavelsavara commented Nov 15, 2023

Copy link
Copy Markdown
Member Author

do you mean also for new Thread(ThreadStart) it takes a synchronous callback that never returns

Yes, good point. Maybe it means that ThreadStart function can't be C# async right? In which case we don't have problem as it can't use await ?

This waitAsync stuff in Emscripten is pretty new. Are we on a version that includes it already?

I think we have version with em_task_queue already.

I thought we were still doing the postMessage-through-main-thread.

I'm not confident, in what i say now:

This problem of dispatching resolve/reject back to managed thread is not same problem as posting to UI thread.
"posting to UI thread" problem is problem because you need event loop (postMessage) to bootstrap new worker.

Messages via SharedArrayBuffer and wasm blocking wait instruction, should make it possible to receive messages while waiting on another wait handle ?

how does the async waiting stuff do anything if the pthread never exits to the JS event loop?

Is that relying on "wasm async proposal" ? Because Atomics.waitAsync is JS API and returns promise. You can't await it from C afaik.

why isn't this wait.value.then(checkMailbox) just piling up unfulfilled promises?

I think when you signal the SharedArrayBuffer address then Atomics.waitAsync would wake that JS only once and not create pile of it.

@pavelsavara

Copy link
Copy Markdown
Member Author

@lambdageek

Copy link
Copy Markdown
Member

Yes, good point. Maybe it means that ThreadStart function can't be C# async right? In which case we don't have problem as it can't use await ?

Yes, ThreadStart can't be an async function. and it cannot use await. But that was exactly my question: do you care only about threadpool threads?

Does emscripten POSIX wait for signals check em_task_queue periodically ?

I think only on the main thread. The code moved to emscripten_futex_wait emscripten-core/emscripten@646de91
on non-main threads they don't service the queue

@pavelsavara

Copy link
Copy Markdown
Member Author

Yes, good point. Maybe it means that ThreadStart function can't be C# async right? In which case we don't have problem as it can't use await ?

Yes, ThreadStart can't be an async function. and it cannot use await. But that was exactly my question: do you care only about threadpool threads?

No, I was wrong when I said "all", because I have not realize this. I guess we need to receive messages on thread pool and on JSWebWorker and on main/UI.

Does emscripten POSIX wait for signals check em_task_queue periodically ?

I think only on the main thread. The code moved to emscripten_futex_wait emscripten-core/emscripten@646de91 on non-main threads they don't service the queue

Thanks!

The emscripten_futex_wait() calls _emscripten_yield() which has condition only for main thread. I wonder why.

So, there is emscripten_current_thread_process_queued_calls which we could poll from idle thread pool thread.

But I guess it's not better than external event loop? What do you think ?

I also wonder if running arbitrary task from random futex could cause deadlock, if the something on pre-existing stack already held some lock. Perhaps they are re-entrant ?

@lambdageek

Copy link
Copy Markdown
Member

I also wonder if running arbitrary task from random futex could cause deadlock, if the something on pre-existing stack already held some lock. Perhaps they are re-entrant ?

They might be re-entrant, but probably there will be problems with higher level code that isn't re-entrant

But I guess it's not better than external event loop? What do you think ?

Probably both are fine, I wonder if webworkers that use an external event loop are better for battery on mobile.

@github-actions github-actions Bot locked and limited conversation to collaborators Dec 17, 2023
@pavelsavara
pavelsavara deleted the browser_pool_dont_yield branch September 2, 2024 15:34
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 os-browser Browser variant of arch-wasm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants