Save and restore Thread.CurrentThread._synchronizationContext for synchronous runtime async calls
#117725
Conversation
Replace `GenTree::DefinesLocal` by a `GenTree::VisitLocalDefs` to prepare for being able to allow a single node to have multiple definitions. Update uses to use the new function.
github-actions
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.CoreCLR.cs
Show resolved
Hide resolved
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS This implements my last known piece of JIT-side compatibility/correctness for runtime async. With this I believe we should be fully compatible with async 1, but of course many pieces of optimizations are missing (and also there may be bugs). cc @VSadov @agocke too. @agocke this will probably regress your benchmark somewhat due to the added ceremony around |
|
/ba-g Infra timeout, succeeded earlier |
|
@VSadov Any other feedback here? |
| const AsyncCallInfo& callInfo = call->GetAsyncInfo(); | ||
| assert(callInfo.SaveAndRestoreSynchronizationContextField && | ||
| (callInfo.SynchronizationContextLclNum != BAD_VAR_NUM)); | ||
|
|
There was a problem hiding this comment.
Is it also a right observation that either ExecContextGCDataIndex or ContinuationContextGCDataIndex present, not both?
Since the sync context is only captured when awaiting Tasks and exec context in Task case is captured into a local (not captured into continuation) - thus we do not need to think about a scenario when both continuation indices are set.
There was a problem hiding this comment.
The structure of the code seems to allow that both contexts may be present in the continuation, but then it would raise questions on capturing exec context twice (before and after await), but I think it is not a real scenario.
There was a problem hiding this comment.
Maybe add an assert or a comment - so that whoever reads this does not need to worry about a combination that cannot happen.
There was a problem hiding this comment.
Is it also a right observation that either
ExecContextGCDataIndexorContinuationContextGCDataIndexpresent, not both?
Yes, that's correct.
The structure of the code seems to allow that both contexts may be present in the continuation, but then it would raise questions on capturing exec context twice (before and after await), but I think it is not a real scenario.
CaptureContinuationContext is orthogonal here and has nothing to do with ExecutionContext. It only deals with SynchronizationContext and TaskScheduler. So even if both were set we would not be capturing ExecutionContext twice.
We capture ExecutionContext into a local if ExecutionContextHandling == SaveAndRestore and we capture it into ExecContextGCDataIndex if ExecutionContextHandling == AsyncSaveAndRestore. Former case is used for task awaits and latter for custom awaitables. So for natural reasons we will never do both.
The breakdown of the handling is the following:
- For custom awaitables there is no special handling of
SynchronizationContextorTaskScheduler. All the handling that exists is custom implemented by the user. - For custom awaitables there is special handling of
ExecutionContext: when the custom awaitable suspends, the runtime/BCL ensure that theExecutionContextwill be captured and restored when the continuation is running. - For task awaits there is special handling of
SynchronizationContextandTaskSchedulerin multiple ways:- The runtime/BCL ensure that
Thread.CurrentThread._synchronizationContextis saved and restored around synchronously finishing calls - The runtime/BCL ensure that when the callee suspends, the caller will eventually be resumed on the
SynchronizationContext/TaskSchedulerpresent before the call started. This resumption can be inlined if theSynchronizationContextis current when the continuation is about to run, and otherwise will be posted to it.
Restoration ofThread.CurrentThread._synchronizationContextin this case is left up to the custom implementation of theSynchronizationContext, it is not handled by the runtime/BCL.
- The runtime/BCL ensure that
- For task awaits the runtime/BCL ensure that
Thread.CurrentThread._executionContextis captured before the call and restored after it. This happens consistently regardless of whether the callee finishes synchronously or not.
Does that make sense? I can add this comment inside AsyncCallInfo.
There was a problem hiding this comment.
This all matches my understanding.
I mostly wanted to confirm that a combination that would lead to both contexts stored to the continuation does not exist.
This implements the remaining handling: saving and restoring
Thread.CurrentThread._synchronizationContextaround runtime async calls that finish synchronously. Lots of trickiness in representing this in the JIT. We introduce a new "suspended indicator" local that is defined by async calls and that the front end uses when it expands IR that restores the field.Plenty of optimization opportunities remaining, including a simple jump threading optimization we should be able to do to make the suspension indicator variable disappear in common cases (all cases except when resuming with an exception).
Note: it might be beneficial to read my comment below to get a better understanding on what the IR being expanded by the JIT corresponds to.