-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[browser][MT] use regular POSIX portable threadpool #99836
[browser][MT] use regular POSIX portable threadpool #99836
Conversation
I filed #99888 for timeouts. I think it's not related to this PR, but I will explore further. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's worth keeping the async semaphore around. But I suppose we can always revert that part, if there is a need in theb future.
Threadpool changes lgtm.
Part of the work here originally was slicing up the event loop into multiple functions and files. @kouvel , would you prefer everything back into a single giant function, or no?
It would be nice to clean up some things if they're not necessary anymore (from a4e2609, dcb34de, 042f593). Eg:
Anyway, the thread pool changes LGTM aside from some cleanup that could be done. |
Moved code back, no functional change. (apart from the first commit here which changes Browser TP) |
src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.cs
Outdated
Show resolved
Hide resolved
Thanks! Just left one minor suggestion but otherwise the moves LGTM. |
Because we dispatch all interaction with JS to the thread which owns the JS objects,
we don't need to yield to browser event loop in the managed thread pool any more.
WebWorkerEventLoop
LowLevelLifoAsyncWaitSemaphore
mono_thread_platform_external_eventloop_keepalive_check
LowLevelLifoSemaphoreBase
and_LifoSemaphoreBase
This is fine piece of machinery which I'm removing, but we don't need it for browser and it would be wasting time to maintain it.
I wonder if we will need to resurrect some of this for WASI, once preview 3 brings futures/promises. But I hope not and I hope that there would not be as strong thread affinity either.
Fixes #85052