Combine ExecutionContext/CapturedContext fields in ManualResetValueTaskSourceCore #82181
Conversation
…skSourceCore There's currently an _executionContext field and a _capturedContext field. Both of these are for non-fast-path. This not only means the struct is larger than is necessary for the common case, it also means when we reset the instance between operations we need to clear an extra field, and it means we have an extra branch on some paths to check both fields. We can instead combine them into a single field. We allocate an extra tuple object if there's both an ExecutionContext and a scheduler, but this is exceedingly rare: when used as part of awaits, there will never be an ExecutionContext, so this will only happen in situations where someone is directly using the awaiter's OnCompleted (not UnsafeOnCompleted) method and there's a scheduler and ConfigureAwait(false) wasn't used. Such a situation not only is rare, it also already has additional overheads. This also cleans up a bit about how exceptions are handled and moves more logic out of the generic type to avoid code bloat with generic instantiations.
stephentoub
added
area-System.Threading.Tasks
tenet-performance
ghost
assigned 
|
Tagging subscribers to this area: @dotnet/area-system-threading-tasks Issue DetailsThere's currently an _executionContext field and a _capturedContext field. Both of these are for non-fast-path. This not only means the struct is larger than is necessary for the common case, it also means when we reset the instance between operations we need to clear an extra field, and it means we have an extra branch on some paths to check both fields. We can instead combine them into a single field. We allocate an extra tuple object if there's both an ExecutionContext and a scheduler, but this is exceedingly rare: when used as part of awaits, there will never be an ExecutionContext, so this will only happen in situations where someone is directly using the awaiter's OnCompleted (not UnsafeOnCompleted) method and there's a scheduler and ConfigureAwait(false) wasn't used. Such a situation not only is rare, it also already has additional overheads. This also cleans up a bit about how exceptions are handled and moves more logic out of the generic type to avoid code bloat with generic instantiations.
|
|
Very similar to the change I made here. Nice! |
|
|
||
| if (continuation is null) | ||
| { | ||
| ThrowHelper.ThrowArgumentNullException(ExceptionArgument.continuation); |
There was a problem hiding this comment.
This is because of the generic context, right?
Should we add a comment to such helpers so future refactorings don't accidentally regress things?
There was a problem hiding this comment.
We use these ThrowHelpers throughout corelib in literally thousands of places. I wouldn't want to comment each usage as to why it's being used. And the ThrowHelper class itself has a large comment about how it's there to reduce code size.
There was a problem hiding this comment.
I meant specifically this vs ArgumentNullException.ThrowIfNull(continuation)
That is, using throw helpers is fine/expected. But using this manual check + throw helper is "unexpected" vs using the new public API we expose that specifically does it. Not everyone may be aware its done due to the generic context and a general refactoring or analyzer might flag it as something to fixup.
There was a problem hiding this comment.
Sure, but that goes for 456 occurrences of ThrowHelper.ThrowArgumentNullException(ExceptionArgument.whatever) elsewhere in src\libraries\System.Private.CoreLib, too.
|
Thanks for reviewing. |
There's currently an _executionContext field and a _capturedContext field. Both of these are for non-fast-path. This not only means the struct is larger than is necessary for the common case, it also means when we reset the instance between operations we need to clear an extra field, and it means we have an extra branch on some paths to check both fields.
We can instead combine them into a single field. We allocate an extra tuple object if there's both an ExecutionContext and a scheduler, but this is exceedingly rare: when used as part of awaits, there will never be an ExecutionContext, so this will only happen in situations where someone is directly using the awaiter's OnCompleted (not UnsafeOnCompleted) method and there's a scheduler and ConfigureAwait(false) wasn't used. Such a situation not only is rare, it also already has additional overheads.
This also cleans up a bit about how exceptions are handled and moves more logic out of the generic type to avoid code bloat with generic instantiations.