-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proposal: Allow [AsyncMethodBuilder(...)] on methods #1407
Comments
I'm not familiar with |
|
Available in .NET Core 2.1 (preview2) https://github.com/dotnet/corefx/issues/27445 |
Ah, I see. You’re pooling the IValueTaskSource, not the ValueTask. Interesting - both that feature and this proposal. |
there should be some default builder implementation for dev easy usage. |
The proposal is the is the missing link, how you can let a custom (i.e. not class ZipAsyncEnumerator : IAsyncEnumerator
{
IValueTaskSource _valueTaskSource;
readonly IAsyncEnumerator _e1;
readonly IAsyncEnumerator _e2;
[AsyncMethodBuilder(typeof(ManualResetValueTaskSourceMethodBuilder), nameof(_valueTaskSource))]
public ValueTask<bool> MoveNextAsync()
{
return await _e1.MoveNextAsync() && await _e2.MoveNextAsync();
}
...
} If the class ManualResetValueTaskAttribute : AsyncMethodBuilderAttribute
{
public ManualResetValueTaskAttribute() : base(typeof(ManualResetValueTaskSourceMethodBuilder), nameof(_valueTaskSource)) {}
} |
I have a working implementation of zero-alloc (not even the state machine) value task with full support for everything that value-task does, because it is value task. If we had this, it could be applied to tons of places without an API change. (The gotcha is you can't call GetResult() twice, which is technically a semantic change,but fine as an opt-in basis) So: please please. This would be amazing. Citation: https://github.com/mgravell/FunWithAwaitables Right now you need to return a different type, but that type effectively is a ValueTask (it has a direct conversion to ValueTask-T that is free and retains semantics) |
Presumably ammortized, because you're pooling the objects? I've had an implementation of that as well, for async ValueTask in general (no attribute), but in addition to the semantic change you mention (undocumented but potentially relied on), there's the perf cost of potentially fine-grained synchronization on the pool, policy around how much to store, etc. It's on my list to revisit for .NET 5. This attribute feature would be a way to both opt-in to such behavior (as you mention) and potentially do better in specific situations (as in the example highlighted in my description). |
@stephentoub exactly; POC is linked above (I edited it in). Amazing speed and memory improvements. Probably going to make that a proper lib and use it in a ton of places. |
@stephentoub actually there's something else in the above you might like - an implementation of Yield that doesn't allocate. Not quite complete yet - I need to implement a non-generic version of the POC so that I can respect execution/sync context - right now it always uses the thread-pool |
Task.Yield you mean? In .NET Core 3.0 it doesn't allocate either, at least not in the common case. |
@stephentoub another related observation: a huge number of times, the value-task is awaited exactly once, in the same assembly (caller/callee). With some escape analysis, if the compiler could prove that an awaitable is only awaited once, it could opt in to a pooled implementation, with no exposure of the semantic change. That would impact tons of use-cases for free. Thoughts? |
@stephentoub huh, that's odd; it did for me, and IIRC I was on .NET Core 3. I'll recheck. It allocated - I found the allocations in the profiler, that's why I added the other version. Specifically, it was the thread-pool queue item "thing" that was allocating. |
You're still seeing it? Can you share the profiler result? It should just be queuing the existing state machine object. |
@stephentoub test results (removed; out of date) Interestingly it works great when using |
Yes. The optimizations employed depends on the built-in async method builder. Thanks for the clarification. |
ooh, I'll have to dig and see if I can exploit it too :) |
dammit, it looks like the speed measure is also invalid - I think benchmarkdotnet might not be awaiting custom awaitables correctly; revised numbers put it in the same ballpark, just with much reduced allocations. Sad now 😢 Allocations, based on x10000 inner-ops: Task + Task.Yield
ValueTask + Task.Yield
TaskLike + TaskLike.Yield
|
@stephentoub I've been thinking about this a lot over the weekend, and if this did become a feature, it feels like a level of abstraction is warranted; there's a difference between intent and implementation, and it is really awkward to express that right now since each shape (Task, TaskT, ValueTask, ValueTaskT, custom whatever) requires a different builder. Mad idea - the caller should be able to express just the intent: [Magic]
public async ValueTask<int> SomeMethod() { ...} with the details delegated via a layer that maps that intent to specific awaitable types: [Something(typeof(Task), typeof(MagicTaskBuilder))]
[Something(typeof(Task<>), typeof(MagicTaskBuilder<>))]
[Something(typeof(ValueTask), typeof(MagicValueTaskBuilder))]
[Something(typeof(ValueTask<>), typeof(MagicValueTaskBuilder<>))]
public sealed class MagicAttribute : SomeCompilerServicesAttribute {} i.e. "look at the method; is it awaitable? does it have, via some The alternative is ... kinda ugly by comparison: [AsyncMethodBuilder(typeof(MagicTaskBuilder<>)]
public async ValueTask<int> SomeMethod() { ...} That's workable, but... or, maybe I'm over-thinking it. |
Finally got all the benchmarkdotnet things figured out; here's my "why I would love this feature", in a single table: | Method | Categories | Mean | Error | StdDev | Gen 0 | Gen 1 | Gen 2 | Allocated |
|------- |------------- |---------:|----------:|----------:|-------:|------:|------:|----------:|
| .NET | Task<T> | 1.738 us | 0.1332 us | 0.0073 us | 0.0176 | - | - | 120 B |
| Pooled | Task<T> | 1.615 us | 0.1809 us | 0.0099 us | 0.0098 | - | - | 72 B |
| | | | | | | | | |
| .NET | Task | 1.693 us | 0.2390 us | 0.0131 us | 0.0176 | - | - | 112 B |
| Pooled | Task | 1.611 us | 0.1460 us | 0.0080 us | 0.0098 | - | - | 72 B |
| | | | | | | | | |
| .NET | ValueTask<T> | 1.710 us | 0.0786 us | 0.0043 us | 0.0195 | - | - | 128 B |
| Pooled | ValueTask<T> | 1.635 us | 0.0677 us | 0.0037 us | - | - | - | - |
| | | | | | | | | |
| .NET | ValueTask | 1.701 us | 0.1759 us | 0.0096 us | 0.0176 | - | - | 120 B |
| Pooled | ValueTask | 1.658 us | 0.1115 us | 0.0061 us | - | - | - | - | |
I think an easy way to do this would be by unsealing AsyncMethodBuilderAttribute: using System.Runtime.CompilerServices;
using System.Threading.Tasks;
namespace System.Runtime.CompilerServices
{
public class AsyncMethodBuilderAttribute : Attribute
{
public Type BuilderType { get; }
public AsyncMethodBuilderAttribute(Type builderType)
{
BuilderType = builderType;
}
}
public class MyMethodBuilderAttribute : AsyncMethodBuilderAttribute
{
public MyMethodBuilderAttribute() : base(typeof(MyMethodBuilder))
{
}
}
}
public static class C
{
[MyMethodBuilder]
public static async Task M(){}
} |
@YairHalberstadt the problem with that is that it strictly ties one result-type to one attribute; maybe that's fine, but it means that you'd need to do: public static class C
{
[MyMethodBuilder]
public static async Task M(){}
[MyDifferentMethodBuilder]
public static async Task<int> Foo(){}
[YetAnotherMethodBuilder]
public static async ValueTask Bar(){}
} etc. I'm not saying that this is insurmountable - just that I'm trying to thing from the consumer's perspective - they are usually interested in expressing intent, not implementation. At that point, it is perhaps just as convenient/inconvenient to use: public static class C
{
[AsyncMethodBuilder(typeof(Magic.TaskMethodBuilder))]
public static async Task M(){}
[AsyncMethodBuilder(typeof(Magic.GenericTaskMethodBuilder))]
public static async Task<int> Foo(){}
[AsyncMethodBuilder(typeof(Magic.ValueTaskMethodBuilder))]
public static async ValueTask Bar(){}
} What I'm trying to propose is that consumers don't care, and would rather have: public static class C
{
[Magic]
public static async Task M(){}
[Magic]
public static async Task<int> Foo(){}
[Magic]
public static async ValueTask Bar(){}
} |
Related - general enhancements to AsyncMethodBuilder machinery: #3403 |
The base wasn't supposed to be. That's a typo I'll fix.
It wouldn't. The derived attribute would need to expose a ctor that accepted such arguments as well, and then the cited heuristic would apply. Or potentially instead of arguments it'd be done with a settable property on the base and which could be specified on usage of the derived. Ah, but the type argument itself is problematic. I'll just delete that section. |
Just voicing a thought here... I'm seeing many database-related codepaths where concurrent invocation of an async method is not supported, much like read and write operations on sockets. For these cases, it would be ideal to be able to somehow store async state in an explicit field on the CLR type where the async method is being invoked; that would eliminate any allocations while at the same time avoiding all the perf overheads of pooling. If I understand this proposal correctly as it is, it would allow building custom pooling implementations for async state, but not this. |
In SQLClient I reuse state objects because I know that only one async operation is permitted at any time and that calls like ReadAsync and GetFieldValueAsync will be used a lot on a single command. |
I'm providing custom async type( I am providing [AsyncMethodBuilderOverride(typeof(UniTaskVoidMethodBuilder))]
async void FooAsync() { }
// more better syntax
[UniTaskVoid]
async void FooAsync() { } There have also been proposals at the module level. #4512 [module: AsyncMethodBuilderOverride(typeof(UniTaskVoidMethodBuilder), typeof(void)] |
I think public class UniTaskVoidAttribute : AsyncMethodBuilderAttribute
{
public UniTaskVoidAttribute() : base(typeof(UniTaskVoidMethodBuilder)) { }
}
`` |
The C# compiler needs to be able to see the type of the builder in the attribute use. Just unsealing the attribute will not help. |
Hm... how about when we get generic attributes? We could have |
With the proposal that made it into C# 10 there seems to be no way to pass state to the method builder and reuse it to avoid extra allocations (i.e. IValueTaskSource in non-concurrent scenarios like #1407 (comment) and the aforementioned allocation free reading from websockets). Since this issue is still open, are there plans to extend AsyncMethodBuilder to support such scenarios in the future? |
The issue should have been closed. I will do so now.
There's no support for the compiler generating code to pass in arguments or |
This proposal needs to remain open until it's incorporated into the ECMA specification. |
@stephentoub Now that generic attributes are shipped, could this be made to work? public class UniTaskVoidAttribute : AsyncMethodBuilderAttribute<UniTaskVoidMethodBuilder>
{
} [UniTaskVoid]
async void FooAsync() { } |
I assume so, since the compiler would be able to see the type argument in the metadata. |
EDIT: Proposal added 11/5/2020:
https://github.com/dotnet/csharplang/blob/master/proposals/csharp-10.0/async-method-builders.md
Background
AsyncMethodBuilderAttribute can be put on a type to be used as the return type of an async method, e.g.
This, however, means that:
Proposal
Two parts:
this
for instance methods. The compiler will then forward the arguments to the method's invocation into the builder's Create.Ammortized allocation-free async methods
There are four types in .NET with built-in builders:
Task
,ValueTask
,Task<T>
, andValueTask<T>
. In .NET Core, significant work has gone in to optimizing async methods with these types, and thus in the majority of uses, each async method invocation will incur at most one allocation of overhead, e.g. a synchronously completing async method returningValueTask<T>
won’t have any additional allocations, and an asynchronously completing async method returningValueTask<T>
will incur one allocation for the underlying state machine object.However, with this feature, it would be possible to avoid even that allocation, for developers/scenarios where it really mattered. .NET Core 2.1 sees the introduction of
IValueTaskSource
andIValueTaskSource<T>
. PreviouslyValueTask<T>
could be constructed from aT
or aTask<T>
; now it can also be constructed from anIValueTaskSource<T>
. That means aValueTask<T>
can be wrapped around an arbitrary backing implementation, and that backing implementation can be reused or pooled (.NET Core takes advantage of this in a variety of places, for example enabling allocation-free async sends and receives on sockets). However, there is no way currently for anasync ValueTask<T>
method to utilize a customIValueTaskSource<T>
to back it, because the builder can only be assigned by the developer of theValueTask<T>
type; thus it can’t be customized for other uses.If a developer could write:
then the developer could write their own builder that used their own
IValueTaskSource<T>
under the covers, enabling them to plug in arbitrary logic and even to pool.However, such a pool would end up being shared across all uses of SomeMethodAsync. This could be a significant scalability bottleneck. If a pool for this were being managed manually, a developer would be able to make the pool specific to a particular instance rather than shared globally. For example,
WebSocket
’sValueTask<…> ReceiveAsync(…)
method utilizing this feature would end up hitting a pool shared by allWebSocket
instances for this particular WebSocket-derived type, but givenWebSocket
’s constraints that only one receive can be in flight at a time,WebSocket
can have a very efficient “pool” of a singleIValueTaskSource<T>
that’s reused over and over. To enable that, the compiler could pass thethis
intoUseMyCustomValueTaskSourceMethodBuilder
’sCreate
method, e.g.The ReceiveAsync implementation can then be written using awaits, and the builder can use
_webSocket._receiveVtsSingleton
as the cache for the single instance it creates. As is now done in .NET Core for task’s builder, it can create anIValueTaskSource<ValueWebSocketReceiveResult>
that stores the state machine onto it as a strongly-typed property, avoiding a separate boxing allocation for it. Thus all state for the async operation becomes reusable across multiple ReceiveAsync invocations, resulting in amortized allocation-free calls.Related
https://github.com/dotnet/corefx/issues/27445
dotnet/coreclr#16618
dotnet/corefx#27497
LDM Notes
The text was updated successfully, but these errors were encountered: