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
[wasm-mt] Support async JS interop on threadpool threads #84494
[wasm-mt] Support async JS interop on threadpool threads #84494
Changes from 14 commits
8c3fef3
a3db886
847ecd5
dd69c8f
451d21b
2889a1f
92d80a5
5f4acc8
806b770
d2811f7
48d22ae
c813701
0a4dd8c
77a54ff
fd99953
4671e03
e22ca46
c591501
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.
Is the G_UNLIKELY here important or necessary? Just curious
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.
Neither - this code isn't particularly hot (I think), so it's more for documentation than anything else.
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.
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.
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.
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?
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, 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
andawait
on an imported JS promise (which gets exposed as aTask<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.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 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)
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.
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?
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.
Correct
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.
Added a gc safe assertion when we're exiting to the external event loop
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.
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
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 should probably just add a managed
internal
flag onMonoThreadObject
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 withvar t = new Thread(...); t.Start()
, anyway