Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] WASM deputy worker - multi-threading proposal #91696
[browser] WASM deputy worker - multi-threading proposal #91696
Changes from 5 commits
739f633
f8e7c8c
16eda73
25c91ad
9974831
e3e76d2
923b6c4
831eb06
455f3ab
69e49b9
14a3394
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
what happens if we can't pause the GC because the deputy worker is stuck? I guess the browser thread hangs, right?
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.
GCs can take hundreds of MS in the real world (I'm not sure why, but our GC under WASM seems to be much slower than it would be on native) so we should assume that users on lower spec devices will see hangs if we have to do this. It might still be the right choice
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.
The other alternative is to rewrite
renderBatch
to streaming bytes.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.
Why does this dispatch need to be done in C#? why can't it be done in JS?
More generally I don't understand why running emscripten off the main thread is a non a workable solution. It has a much more easily understandable execution model, in my opinion - it's just like talking to a server.
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.
JS interop is creating JS objects and that needs to happen on the right JS worker, because of affinity.
The JS side of the marshaller is calling static C# methods, to create C# instances of
Task
.To the second question, that's alternative 10. The main reason is that JS interop and also the JS embedding APIs actually needs to make those short-lived synchronous calls to Mono/C#. Otherwise even JS code of marshaling individual arguments would have to be async, because it would send messages to a "server".
The Blazor
renderBatch
is also touching memory and convertingMonoString*
etc. That could not become async, perf would just die.It could be done, but it would not be less trouble than having C# on the UI thread internally.
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.
There is more developed version of this "why not aternative 10." lower in the conversation. I will create separate PR/proposal for that.
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.
See #91731
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.
@lambdageek here is more detailed answer #91731 (comment)
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.
Short answer: because marshaling JSImport/JSExport parameters requires lot on Mono infrastructure. Like allocating
MonoString
,TaskCompletionSource
and many others. Those are mostly synchronous calls and need to be fast. Trying to dispatch those calls from UI to sidecar thread would lead to terrible latency, per parameter. Alternatively we could have double proxy for some of those data types, which is not great either, mostly for GC.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.
Is this just a well-known "JSWebWorker with JSInterop" thread that the ui-thread JS interop knows about? If not, are the different capabilities of this special thread and other JSWebWorker with JS Interop threads required?
Wait, I read more and I thikn I can answer myself. It's just like a JSWebWorker with JSInterop thread except instead of owning "real" JS Interop it owns proxies that represent JS Interop objects of the UI thread instead. Is that right?
So we basically have special "JS event loop" threads and they come in two flavors: 1. they interact with their own js objects, or 2. they just pass messages to some other delegated js object realm?
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.
Yes, deputy is similar to
JSWebWorker
except it talks to UI thread JS space, not it's own (on logical level).The implementation of how it works is not JS event loop, but rather C#
SynchronizationContext
.Passing those messages could be also done via emscripten's
emscripten_dispatch_to_thread_async
or by JSpostMessage
.Doing it in C# allows us to interleave it with the code generated by Roslyn codegen.
Let's do more thinking about this, how to actually do it.
One of the open questions for me is selection of target thread based on affinity of the
JSObject
proxy passed as argument. Note we are talking about static methods which could have more or also none such object.This question is relevant for "call HTTP from any C# thread" scenario.
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.
finalizers already run on a separate thread, not the main thread, AFAIK. the finalizer thread is owned by the GC, I think?
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 think we don't run finalizers in signe-threaded WASM right now.
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.
in single-threaded wasm finalizers run in
mono_runtime_do_background_work
which is scheduled as a background job whenmono_wasm_gc_finalize_notify
is called. In other words after a GC is finished we queue up an idle task to run finalizers.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.
Can we somehow guarantee that GC will never happen on the browser thread? The browser thread will potentially have to stall and wait for GC at the very least, if we ever allow C# to run on it.
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.
There's
GC.TryStartNoGCRegion()
(no-op on Mono, I think). It's a weird global mode (so if you have background threads that are quickly allocating, your critical region can still run out of memory), but it might be good enough for some scenarios.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.
FYI since you mentioned performance, be aware that (at present) having any kind of custom synchronization context installed causes some of the Task/Await machinery to go through a slow-path. see #69409 (comment) (though I hope this won't actually matter)
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.
we should assume it can and will happen
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 want the answer to be "yes"
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 hope so too. Over night I realized that shape of this API could be 2 methods on the
JSHost
rather than exposing theSynchronizationContext
, because there could more complex logic on which of the should be used.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.
This seems hard
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.
We could allow it but tell users not to use it. This is similar ot guidance in other UI framworks not to block UI threads. They don't prohibit you from running code but then you're on the hook for your UI freezing up (or in this case draining the battery and getting killed by the mobile browser)
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.
We could have MSBuild property for it, which would be opt-in. Right now, I think if we are doing all of this complex stuff to prevent it, we should gain the benefits of it. Otherwise we will have to support crazy things.
I though that we could get away with spin block. But MT unit tests on CI are hanging in too many cases, and probable for this reason. We just need to get over it, me included.