Use ExecutionContext.Restore rather than EC.Run callback#37942
Use ExecutionContext.Restore rather than EC.Run callback#37942kouvel merged 3 commits intodotnet:masterfrom
Conversation
|
Thanks, @benaadams. Any perf impact? |
.../System.Private.CoreLib/src/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs
Outdated
Show resolved
Hide resolved
.../System.Private.CoreLib/src/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs
Outdated
Show resolved
Hide resolved
.../System.Private.CoreLib/src/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs
Outdated
Show resolved
Hide resolved
11f08f2 to
a212991
Compare
|
Moved to using same api as would use externally.
Resolved |
For the common path (with no EC) can only see how it would get a little faster since the calls are inlined for that path. If |
4f96c49 to
b6b9436
Compare
|
Rebased |
.../System.Private.CoreLib/src/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs
Outdated
Show resolved
Hide resolved
.../System.Private.CoreLib/src/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs
Outdated
Show resolved
Hide resolved
36b7a5f to
2c0c15c
Compare
.../System.Private.CoreLib/src/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs
Outdated
Show resolved
Hide resolved
stephentoub
left a comment
There was a problem hiding this comment.
LGTM other than my one comment. @kouvel, any concerns?
.../System.Private.CoreLib/src/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs
Outdated
Show resolved
Hide resolved
.../System.Private.CoreLib/src/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Would it make sense to call this method Apply or something similar instead of Restore given how it's used?
There was a problem hiding this comment.
Why would the ASP.NET use case of the public API not use EC.Run() instead? In any case even for a public API (which would be a somewhat advanced API that may be misused), Apply seems more appropriate to me, maybe something to consider in the API review
There was a problem hiding this comment.
Why would the ASP.NET use case of the public API not use EC.Run() instead?
Because EC.Run forces you into a callback model, which is problematic if the code you're wrapping needs to be async.
There was a problem hiding this comment.
Hmm ok. Would a RunAsync instead be workable?
There was a problem hiding this comment.
It's not just the shape of the method: it's the overhead. A goal here is to avoid the extra machinery/allocation associated with another level of async method / state machine.
There was a problem hiding this comment.
Makes sense, but if that's why we're exposing the API (for perf), then would it make sense to prefix the API name with Unsafe or something else similarly since it feels like it could be easily misused?
There was a problem hiding this comment.
Hmm ok. Would a RunAsync instead be workable?
Well just putting it behind a regular async method gives that effect.
The ASP.NET case is inlining all the awaits into a top level request loop (as its allocated once for all requests on the connection); however the loop calls user code, if any of the methods are not async (Task.CompletedTask returning), but also change AsyncLocal<T>s then they are not undone and those changes leak into the next request (dotnet/aspnetcore#15384, dotnet/aspnetcore#13991)
The .Restore method allows the EC to be captured as a hoisted var outside the loop; and then reset back at the end of each loop iteration so each new request starts on a fresh ExecutionContext throwing away any changes from the last iteration dotnet/aspnetcore#23175 (kinda like the ThreadPool does)
There was a problem hiding this comment.
e.g.
var ec = ExecutionContext.Capture();
while (!cancelled)
{
await NextRequest();
await OnStartAsync(); // User code
await ProcessRequestAsync(); // User code
await OnCompleteAsync(); // User code
ExecutionContext.Restore(ec);
}
.../System.Private.CoreLib/src/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs
Outdated
Show resolved
Hide resolved
.../System.Private.CoreLib/src/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs
Outdated
Show resolved
Hide resolved
.../System.Private.CoreLib/src/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Threading/ExecutionContext.cs
Outdated
Show resolved
Hide resolved
.../System.Private.CoreLib/src/System/Threading/Tasks/Sources/ManualResetValueTaskSourceCore.cs
Outdated
Show resolved
Hide resolved
69e582d to
3895f53
Compare
|
Rebased to retrigger tests |
|
Good to merge? |
|
Thanks! |
Only running inline needs to do extra work and error handling for post run restore
@stephentoub this would be the change for
ManualResetValueTaskSourceCoreto useExecutionContext.Restorerather than a genericrefpassing callback basedExecutionContext.Run