Shrink AsyncValueTaskMethodBuilder #24431
Conversation
| #endif | ||
|
|
||
| /// <summary>The generic builder object to which this non-generic instance delegates.</summary> | ||
| private AsyncTaskMethodBuilder<VoidTaskResult> m_builder; // mutable struct: must not be readonly. Debugger depends on the exact name of this field. |
There was a problem hiding this comment.
This is commented Debugger depends on the exact name of this field.
Not sure if removing the wrapping and changing it to m_task will cause issue?
There was a problem hiding this comment.
Not sure if removing the wrapping and changing it to m_task will cause issue?
It might. You would need to test it. Does stepping out of an async method that yielded continue to work correctly? Does stepping over a yielding await continue to work correctly?
|
As the Task is now unwrapped and we test and can set it directly, using the utility methods, rather than going via the wrapped builder; Stream.ReadAsync's local function: G_M40602_IG27:
mov rax, bword ptr [rbp+10H]
mov dword ptr [rax+16], -2
mov rdi, bword ptr [rbp+10H]
add rdi, 24
- cmp byte ptr [rdi+5], 0
- je SHORT G_M40602_IG31
- add rdi, 8
- cmp gword ptr [rdi], 0
- jne SHORT G_M40602_IG30
- mov ebx, esi
- cmp ebx, 9
- jge SHORT G_M40602_IG28
- cmp esi, -1
- jl SHORT G_M40602_IG28
- call [CORINFO_HELP_READYTORUN_STATIC_BASE]
- mov rax, gword ptr [rax+0950H]
- lea edx, [rbx+1]
- cmp edx, dword ptr [rax+8]
- jae SHORT G_M40602_IG33
- inc ebx
- movsxd rdx, ebx
- mov rdx, gword ptr [rax+8*rdx+16]
+ mov rdx, gword ptr [rcx]
+ test rdx, rdx
+ jne SHORT G_M40595_IG28
+ mov dword ptr [rcx+8], esi
+ mov byte ptr [rcx+12], 1
jmp SHORT G_M40602_IG29
-G_M40602_IG28:
- call [CORINFO_HELP_READYTORUN_NEW]
- mov rdx, rax
- mov dword ptr [rdx+52], 0xD1FFAB1E
- mov dword ptr [rdx+56], esi
-
-G_M40602_IG29:
- mov rcx, rdi
- call [CORINFO_HELP_CHECKED_ASSIGN_REF]
- jmp SHORT G_M40602_IG32
-G_M40602_IG30:
+G_M40595_IG28:
- mov rcx, rdi
+ mov rcx, gword ptr [rcx]
mov edx, esi
- call [AsyncTaskMethodBuilder`1:SetExistingTaskResult(int):this]
+ call [AsyncMethodBuilderCore:SetExistingTaskResult(ref,int)]
- jmp SHORT G_M40602_IG32
-G_M40602_IG31:
+G_M40595_IG29:
- mov dword ptr [rdi], esi
- mov byte ptr [rdi+4], 1
+ nop
-G_M40602_IG32:
+G_M40595_IG30:
lea rsp, [rbp-38H]
pop rbx |
src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/AsyncIteratorMethodBuilder.cs
Outdated
Show resolved
Hide resolved
| /// <summary>The builder this void builder wraps.</summary> | ||
| private AsyncTaskMethodBuilder _builder; // mutable struct: must not be readonly | ||
| /// <summary>The lazily-initialized built task.</summary> | ||
| private Task<VoidTaskResult> m_task; // lazily-initialized: must not be readonly. Debugger depends on the exact name of this field. |
There was a problem hiding this comment.
Have you verified that stepping over yielding awaits in the debugger in VS continues to work correctly?
src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/AsyncMethodBuilder.cs
Outdated
Show resolved
Hide resolved
| @@ -29,24 +29,24 @@ public struct AsyncVoidMethodBuilder | |||
| { | |||
| /// <summary>The synchronization context associated with this operation.</summary> | |||
| private SynchronizationContext? _synchronizationContext; | |||
| /// <summary>The builder this void builder wraps.</summary> | |||
| private AsyncTaskMethodBuilder _builder; // mutable struct: must not be readonly | |||
| /// <summary>The lazily-initialized built task.</summary> | |||
There was a problem hiding this comment.
Are all these changes to AsyncVoidMethodBuilder really necessary? Devs that care about perf are using "async void"? And the changes result in a measurable and meaningful throughput improvement?
There was a problem hiding this comment.
The methods are there now, so its just swapping from forwarding to AsyncTaskMethodBuilder to AsyncTaskMethodBuilderCore
src/System.Private.CoreLib/shared/System/Runtime/CompilerServices/AsyncMethodBuilder.cs
Show resolved
Hide resolved
...System.Private.CoreLib/shared/System/Runtime/CompilerServices/AsyncValueTaskMethodBuilder.cs
Outdated
Show resolved
Hide resolved
...System.Private.CoreLib/shared/System/Runtime/CompilerServices/AsyncValueTaskMethodBuilder.cs
Outdated
Show resolved
Hide resolved
...System.Private.CoreLib/shared/System/Runtime/CompilerServices/AsyncValueTaskMethodBuilder.cs
Outdated
Show resolved
Hide resolved
...System.Private.CoreLib/shared/System/Runtime/CompilerServices/AsyncValueTaskMethodBuilder.cs
Outdated
Show resolved
Hide resolved
...System.Private.CoreLib/shared/System/Runtime/CompilerServices/AsyncValueTaskMethodBuilder.cs
Show resolved
Hide resolved
...System.Private.CoreLib/shared/System/Runtime/CompilerServices/AsyncValueTaskMethodBuilder.cs
Outdated
Show resolved
Hide resolved
|
Thanks, @benaadams, for working on this. I'm not sure how much benefit there is to shrinking the size of |
In order to drop the two fields For The cleanest approach is to remove the extra 2 layers of struct wrapping and put the Task directly into the AsyncValueTaskMethodBuilder. However, that's problematic as they then no longer have access to the instance methods from So the next step is to move the logic from At that point, none of the MethodBuilders need to do any wrapping, can have the Task themselves and can just call into So for So the call chain is
And that's repeated for the various types ( This changes the 3 levels of struct wrapping over a Task; to everything becoming a single struct over a Task and going to the same direct methods in
I understand the concern on the churn; but hopefully its easier to follow as all the calls are now direct to their centralised implementation rather than going through multiple layers? And it may be easier for the Jit (or might not make much difference) |
Will look into the specifics tomorrow |
Ignoring the churn in the method builder files; the other impacted methods in coreclr are: The Jit does does unnecessary work with the 3 wrappers; as for ; Assembly listing for method AsyncIteratorMethodBuilder:Create():struct
-; Lcl frame size = 8
+; Lcl frame size = 0
G_M16816_IG01:
- push rax
- xor rax, rax
- mov qword ptr [rsp], rax
+ nop
G_M16816_IG02:
xor rax, rax
- mov gword ptr [rsp], rax
- mov rax, gword ptr [rsp]
G_M16816_IG03:
- add rsp, 8
ret
-; Total bytes of code 22, prolog size 7 for method AsyncIteratorMethodBuilder:Create():struct
+; Total bytes of code 8, prolog size 5 for method AsyncIteratorMethodBuilder:Create():struct |
|
Improvements from
Regression from
Same regression in
|
|
However, it then also has a gain from
So not sure if the AggressiveInline it or leave as is? |
|
From the Jit side of things for
Which I assume is a good thing :) |
|
Thanks. Have you run any perf tests? |
|
Rebased
Will do and test the debugger. Though I think the bigger gain will be from the copy improvements @AndyAyersMS and @mikedn are looking at in #24915 (comment) |
| private Task Task => _builder.Task; | ||
| public Task Task | ||
| { | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] |
There was a problem hiding this comment.
This code is repeated in each variant of the builder, is there a better way to express it as a short-circuit inside AsyncMethodBuilderCore.InitializeTaskAsPromise with aggressive inlining there?
I'm probably too shallow in the code here, I'm honestly not a fan of gettors that have side-effects either.
There was a problem hiding this comment.
This is AggressivelyInlined but InitializeTaskAsPromise is NoInlined
not a fan of gettors that have side-effects either.
Its a Lazy initialize, rather than full side effect
There was a problem hiding this comment.
Would it be better to have InitializeTaskAsPromise inlined too?
There was a problem hiding this comment.
No; as isn't always called; the backing task can be set elsewhere (and I think usually is)
| // ProjectN's AsyncTaskMethodBuilder<VoidTaskResult>.Create() currently does additional debugger-related | ||
| // work, so we need to delegate to it. | ||
| new AsyncTaskMethodBuilder { m_builder = AsyncTaskMethodBuilder<VoidTaskResult>.Create() }; | ||
| var result = new AsyncTaskMethodBuilder(); |
There was a problem hiding this comment.
Should we move this InitializeTaskIfDebugging call into the (possibly new internal) constructor for AsyncTaskMethodBuilder now to ensure it doesn't get in future code-creep?
Also, since this behavior is needed for each variant of the *Builder, how can we ensure this constructor behavior unless we have a abstract base class?
There was a problem hiding this comment.
unless we have a abstract base class
No inheritence as they are all structs which do not support it
There was a problem hiding this comment.
Yeah, duh. Sorry for that... wondering still about the fragility of this duplicate code.
|
|
||
| /// <summary>Gets the <see cref="System.Threading.Tasks.Task"/> for this builder.</summary> | ||
| /// <returns>The <see cref="System.Threading.Tasks.Task"/> representing the builder's asynchronous operation.</returns> | ||
| /// <exception cref="System.InvalidOperationException">The builder is not initialized.</exception> | ||
| public Task Task | ||
| { | ||
| [MethodImpl(MethodImplOptions.AggressiveInlining)] | ||
| get => m_builder.Task; | ||
| get => m_task ?? AsyncMethodBuilderCore.InitializeTaskAsPromise(ref m_task!); // TODO-NULLABLE: Remove ! when nullable attributes are respected |
There was a problem hiding this comment.
| result.InitializeTaskAsStateMachineBox(); | ||
| } | ||
| return result; | ||
| return AsyncMethodBuilderCore.InitalizeTaskIfDebugging(ref result, ref result.m_task!); // TODO-NULLABLE: Remove ! when nullable attributes are respected |
There was a problem hiding this comment.
Parallel comment to the AsyncMethodBuilder.Create above.
| CreateDebugFinalizableAsyncStateMachineBox<IAsyncStateMachine>() : | ||
| new AsyncStateMachineBox<IAsyncStateMachine>()); | ||
| #endif | ||
| get => m_task ?? AsyncMethodBuilderCore.InitializeTaskAsPromise(ref m_task!); // TODO-NULLABLE: Remove ! when nullable attributes are respected |
There was a problem hiding this comment.
| // ProjectN's AsyncTaskMethodBuilder.Create() currently does additional debugger-related | ||
| // work, so we need to delegate to it. | ||
| new AsyncValueTaskMethodBuilder() { _methodBuilder = AsyncTaskMethodBuilder.Create() }; | ||
| var result = new AsyncValueTaskMethodBuilder(); |
There was a problem hiding this comment.
| } | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| private static ValueTask CreateValueTask(ref Task<VoidTaskResult> task) => new ValueTask(task ?? AsyncMethodBuilderCore.InitializeTaskAsPromise(ref task!)); // TODO-NULLABLE: Remove ! when nullable attributes are respected |
There was a problem hiding this comment.
This one is subtly different from code in https://github.com/dotnet/coreclr/pull/24431/files#diff-290cb2024350a19884ed781417721d54R170 in that it wraps the m_task by getting it by ref in it's only caller. We should likely follow this pattern or the other one as best we can.
| // work, so we need to delegate to it. | ||
| new AsyncValueTaskMethodBuilder<TResult>() { _methodBuilder = AsyncTaskMethodBuilder<TResult>.Create() }; | ||
| var result = new AsyncValueTaskMethodBuilder<TResult>(); | ||
| return AsyncMethodBuilderCore.InitalizeTaskIfDebugging(ref result, ref result.m_task!); // TODO-NULLABLE: Remove ! when nullable attributes are respected |
There was a problem hiding this comment.
Once again a parallel to code in https://github.com/dotnet/coreclr/pull/24431/files#diff-290cb2024350a19884ed781417721d54R202
| } | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining)] | ||
| private static ValueTask<TResult> CreateValueTask(ref Task<TResult> task) => new ValueTask<TResult>(task ?? AsyncMethodBuilderCore.InitializeTaskAsPromise(ref task!)); // TODO-NULLABLE: Remove ! when nullable attributes are respected |
There was a problem hiding this comment.
Same comment as Same as code in https://github.com/dotnet/coreclr/pull/24431/files#diff-b2f7a19e7ee56b16cfe87d2b115e0059R114
|
Lot of bit rot here. |
|
I'm experimenting with your approach in my pooling PR as well. I'm hopeful as a next step it'll let me actually make the pooling opt-in via a feature flag. |
😄 |
Remove the struct wrappings over Task in the AsyncMethodBuilders by moving the implementations to static helper members on AsyncMethodBuilderCore.
Shrink
AsyncValueTaskMethodBuilderto a single pointer size.Remove
bool _useBuilderfield fromAsyncValueTaskMethodBuilder<TResult>as we can check ifm_taskis set instead./cc @stephentoub