Faster path default EC #11100
Faster path default EC #11100
Conversation
|
Most of the EC calls for Kestrel on libuv take this path Before (call count underlined) After (call count underlined) The 18 of the other 22 are timer callbacks; which work off timer thread or do an Unsafe queue to threadpool and other 4 are thread starts. (didn't want to mess in this area yet) The Socket version and IO completion doesn't seem to be on the default context (or it would take a different path and doesn't call Run) so there are a higher percentage that are in the regular |
|
Socket version Upstream for the |
|
What impact does this have on throughout? If code calls TP.UQUWI and then changes the EC (e.g. changes an AsyncLocal), that'll poison the TP thread and knock subsequent work items off the fast path? And the slow path is now slower than the fast path today? |
Slow path is slightly faster than current as using the local Default in Restore means one less call to
Off the faster-path and only for that Dispatch quantum; will still use the previous default context fast path; albeit with one extra call. Slow path is unchanged. However have fixed. One speed bump is as there are more explicit default context threads I'd like to move more threads onto the default context, like the IO and explicitly call Preliminary local testing is faster; my threadpool/task tests are slower but with CPU time in kernel greatly increased - which paradoxically means its faster shown through higher SpinWait contention - need to improve how they measure 😆 Will do further testing. |
|
Feel a bit foolish, is easy to test by setting Then all contention measurement drops out |
|
Full tests using single threadpool thread https://gist.github.com/benaadams/bf56dc4424ca6888aba178074cdae2b6 Highlights Pre Post Pre Post Pre Post Pre Post Generally looks better |
|
@dotnet-bot test Windows_NT x64 corefx_baseline |
| { | ||
| // Current thread passed as parameter to avoid extern current native thread lookup call | ||
| Debug.Assert(currentThread == Thread.CurrentThread); | ||
| // Default context passed as parameter to avoid static inializer checks in NGen'd code |
|
|
||
| if (currentThread.ExecutionContext != defaultContext) | ||
| { | ||
| currentThread.ExecutionContext = defaultContext; |
There was a problem hiding this comment.
Should this call ExecutionContext.RestoreDefault as in UndoToDefault to check for AsyncLocal changes and raise events?
If so, then perhaps UndoToDefault could be renamed to SetDefaults and this overload with no parameters can call the one with parameters.
There was a problem hiding this comment.
Is only used for after running an Unsafe workitem and Dispatch when no user code is running
There was a problem hiding this comment.
Not sure about this, the unsafe work item could change async locals and it seems this is the only place where we don't notify when restoring the context. For the Dispatch case where no user code is running, if the EC needs to be initialized to Default, could probably do that directly and even avoid checking the SynchronizationContext which should be null anyway at that point.
There was a problem hiding this comment.
Currently if Unsafe changed async locals you'd wouldn't get notification at the end of the Unsafe call but you'd get them at the start of every regular threadpool item execution and the values would live in a kinda limbo land.
Also you could never move back onto the DefaultContext as even if you clear the values it won't move you back onto Default #8227 (comment)
There was a problem hiding this comment.
Its the poisoning @stephentoub referred to in #11100 (comment) that I resolved with f358e02
Can revert it to allow poisoning by Unsafe which would permanently move it onto slower path but maintain current behaviour?
There was a problem hiding this comment.
Yea that is broken. @stephentoub, thoughts on whether we should restore the context, and whether we should notify of async local changes upon restoring here? Not sure how important the Unsafe version is, but the same path applies to suppressed-flow contexts as well.
There was a problem hiding this comment.
Yea that is broken
I was referring to your previous comment:
Currently if Unsafe changed async locals you'd wouldn't get notification at the end of the Unsafe call but you'd get them at the start of every regular threadpool item execution and the values would live in a kinda limbo land.
| currentThread.ExecutionContext = defaultContext; | ||
|
|
||
| // For the purposes of dealing with context change, null counts as the default EC | ||
| if (previous != null) |
There was a problem hiding this comment.
It looks like this would call OnContextChanged when previous == defaultContext. Maybe this should be previous != defaultContext && previous != null.
There was a problem hiding this comment.
Is called by UndoToDefault when previous != default so its already implied that previous isn't default. Probably should add an additional asset at top.
There was a problem hiding this comment.
Yea an assert would be good, as it is exposed to future calls from elsewhere
| internal static bool Dispatch() | ||
| { | ||
| // Put threadpool thread on default context | ||
| ExecutionContextSwitcher.SetDefaults(); |
There was a problem hiding this comment.
Why is this necessary? Shouldn't each work item restore the context especially after these changes, or is that still missing somewhere?
There was a problem hiding this comment.
Threadpool threads don't start on Default context as the VM creates them rather than the managed code; starts as null
There was a problem hiding this comment.
Though it should only need ever to be called once (on thread create); so maybe there is a better way of doing it?
There was a problem hiding this comment.
Shouldn't the context be null to start with? Maybe ExecutionContextSwitcher.CurrentIsDefault should also check for null.
There was a problem hiding this comment.
If its null then it needs to do a memory barrier assign to Default; so its once per Dispatch or once per loop in Dispatch (as it would get restored to null).
There was a problem hiding this comment.
I kinda agree; but the EC mix with default and flow suppressed is more complicated so it would need a greater set of changes.
e.g. For Capture for CurrentThread.ExecutionContext
EC == null => Default
EC != null && FlowSupressed => null
EC != null =>EC
So null means flow suppressed, unless you recapture when it means Default
There was a problem hiding this comment.
i.e. could have a FlowSupressed context with null being Default instead; but that's probably a more dangerous change?
There was a problem hiding this comment.
So null means flow suppressed, unless you recapture when it means Default
I didn't follow that. I think Capture could stay as is, if it returns null it means EC != null and FlowSuppressed == true. If Capture returns Default it means EC == null. A current flow-suppressed context means EC != null.
i.e. could have a FlowSupressed context with null being Default instead; but that's probably a more dangerous change?
I didn't follow that either, could you please elaborate?
There was a problem hiding this comment.
Will try using null for default internally
|
Hi @benaadams do you expect to take it further? |
|
@danmosemsft yes, I need to invert what its doing as per @kouvel's suggestion (i.e. what |
|
@benaadams interested in continuing?... |
|
@benaadams if you don't have time to work on it now, let's close the PR, until you have the time again ;) |
|
Very interested; but swamped. Will close and revisit. |
7 participants
Trying again, follow up to #11095
Puts the Threadpool threads on the default context.
Less churn when moving between Default contexts (
ExecutionContext.RunDefaultContext); with lighter context switching.Also allows the opportunity to benefit from Finally Cloning by the Jit in
RunDefaultContext.Resolves https://github.com/dotnet/coreclr/issues/11126