csharplang icon indicating copy to clipboard operation
csharplang copied to clipboard

Proposal: Allow [AsyncMethodBuilder(...)] on methods

Open stephentoub opened this issue 7 years ago • 36 comments

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.

public struct AsyncCoolTypeMethodBuilder
{
    public static AsyncCoolTypeMethodBuilder Create();
    ...
}

[AsyncMethodBuilder(typeof(AsyncCoolTypeMethodBuilder))]
public struct CoolType { ... }

public async CoolType SomeMethodAsync() { ... } // will implicitly use AsyncCoolTypeMethodBuilder.Create()

This, however, means that:

  • every use of a given return type requires the same builder.
  • there's no context available to be passed to the builder.
  • if you don't own the type and it's not already attributed, you can't use it as an async return type.

Proposal

Two parts:

  1. Allow AsyncMethodBuilderAttribute to be applied to methods. When applied to an async method, it would be used as the builder for that method, overriding any AsyncMethodBuilderAttribute on the return type.
  2. Allow the builder's Create method to have arguments. Specifically, it can have arguments that match the parameters to the method to which it's applied, including the implicit 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>, and ValueTask<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 returning ValueTask<T> won’t have any additional allocations, and an asynchronously completing async method returning ValueTask<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 and IValueTaskSource<T>. Previously ValueTask<T> could be constructed from a T or a Task<T>; now it can also be constructed from an IValueTaskSource<T>. That means a ValueTask<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 an async ValueTask<T> method to utilize a custom IValueTaskSource<T> to back it, because the builder can only be assigned by the developer of the ValueTask<T> type; thus it can’t be customized for other uses.

If a developer could write:

[AsyncMethodBuilder(UseMyCustomValueTaskSourceMethodBuilder)]
public async ValueTask<T> SomeMethodAsync() { … }

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’s ValueTask<…> ReceiveAsync(…) method utilizing this feature would end up hitting a pool shared by all WebSocket instances for this particular WebSocket-derived type, but given WebSocket’s constraints that only one receive can be in flight at a time, WebSocket can have a very efficient “pool” of a single IValueTaskSource<T> that’s reused over and over. To enable that, the compiler could pass the this into UseMyCustomValueTaskSourceMethodBuilder’s Create method, e.g.

internal sealed class ManagedWebSocket : WebSocket
{
    private struct WebSocketReceiveMethodBuilder
    {
        private ManagedWebSocket _webSocket;

        public static WebSocketReceiveMethodBuilder Create(ManagedWebSocket thisRef, Memory<byte> buffer, CancellationToken cancellationToken) =>
            new WebSocketReceiveMethodBuilder { _webSocket = thisRef };

        …
    }

    private IValueTaskSource<ValueWebSocketReceiveResult> _receiveVtsSingleton;

    [AsyncMethodBuilder(typeof(WebSocketReceiveMethodBuilder)]
    public override async ValueTask<ValueWebSocketReceiveResult> ReceiveAsync(Memory<byte> buffer, CancellationToken cancellationToken) { … }
}

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 an IValueTaskSource<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 https://github.com/dotnet/coreclr/pull/16618 https://github.com/dotnet/corefx/pull/27497

LDM Notes

  • https://github.com/dotnet/csharplang/blob/main/meetings/2021/LDM-2021-03-29.md
  • https://github.com/dotnet/csharplang/blob/main/meetings/2020/LDM-2020-11-11.md

stephentoub avatar Mar 22 '18 13:03 stephentoub

I'm not familiar with IValueTaskSource and it has very few Google search results, but how do you pool a struct? 😕

yaakov-h avatar Mar 22 '18 13:03 yaakov-h

IValueTaskSource<T> is an interface, not a struct.

stephentoub avatar Mar 22 '18 13:03 stephentoub

I'm not familiar with IValueTaskSource and it has very few Google search results,

Available in .NET Core 2.1 (preview2) https://github.com/dotnet/corefx/issues/27445

benaadams avatar Mar 22 '18 13:03 benaadams

Ah, I see. You’re pooling the IValueTaskSource, not the ValueTask.

Interesting - both that feature and this proposal.

yaakov-h avatar Mar 22 '18 13:03 yaakov-h

there should be some default builder implementation for dev easy usage.

juepiezhongren avatar Dec 29 '18 10:12 juepiezhongren

The proposal is the is the missing link, how you can let a custom (i.e. not yield using) IAsyncEnumerable implementation benefit from IValueTaskSource. I like that. What worries me a little is that you are forced to create your own builder for every class or at least for every assembly if you do not want to leak internals. Hence wouldn't it be better to explicitly name the DI property/field? So that you could use a async method builder written once by experts. Like:

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 AsyncMethodBuilderAttribute wouldn't be sealed one could also define their own shortcut:

class ManualResetValueTaskAttribute : AsyncMethodBuilderAttribute
{
    public ManualResetValueTaskAttribute() : base(typeof(ManualResetValueTaskSourceMethodBuilder), nameof(_valueTaskSource)) {}
}

quinmars avatar Dec 31 '18 10:12 quinmars

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<T> (it has a direct conversion to ValueTask-T that is free and retains semantics)

mgravell avatar Jul 06 '19 11:07 mgravell

zero-alloc (not even the state machine)

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 avatar Jul 06 '19 11:07 stephentoub

@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.

mgravell avatar Jul 06 '19 12:07 mgravell

@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

mgravell avatar Jul 06 '19 12:07 mgravell

an implementation of Yield that doesn't allocate

Task.Yield you mean? In .NET Core 3.0 it doesn't allocate either, at least not in the common case.

stephentoub avatar Jul 06 '19 12:07 stephentoub

@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?

mgravell avatar Jul 06 '19 12:07 mgravell

@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.

mgravell avatar Jul 06 '19 12:07 mgravell

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 avatar Jul 06 '19 13:07 stephentoub

@stephentoub test results

(removed; out of date)

Interestingly it works great when using Task/ValueTask methods; maybe it is only allocating differently in my experimental (TaskLike) scenario. I'll dig some more!

mgravell avatar Jul 06 '19 16:07 mgravell

maybe it is only allocating differently in my experimental (TaskLike) scenari

Yes. The optimizations employed depends on the built-in async method builder.

Thanks for the clarification.

stephentoub avatar Jul 06 '19 16:07 stephentoub

ooh, I'll have to dig and see if I can exploit it too :)

mgravell avatar Jul 06 '19 16:07 mgravell

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

Name	Total (Allocations)	Self (Allocations)	Total Size (Bytes)	Self Size (Bytes)
 - System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1	10002	
 - FunWithAwaitables.Awaitable.<<ViaTask>g__Inner|8_0>d	10000	
 - System.SByte[]	99	
 - ...

ValueTask + Task.Yield

Name	Total (Allocations)	Self (Allocations)	Total Size (Bytes)	Self Size (Bytes)
 - System.Runtime.CompilerServices.AsyncTaskMethodBuilder`1.AsyncStateMachineBox`1	10002	
 - FunWithAwaitables.Awaitable.<<ViaValueTask>g__Inner|9_0>d	10000	
 - System.SByte[]	102	
 - ...

TaskLike + TaskLike.Yield

Name	Total (Allocations)	Self (Allocations)	Total Size (Bytes)	Self Size (Bytes)
 - System.SByte[]	109	
 - ...	

mgravell avatar Jul 06 '19 16:07 mgravell

@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 SomeCompilerServicesAttribute subclass, a designated SomethingAttribute that maps the method's return type to a custom builder type".

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.

mgravell avatar Jul 07 '19 18:07 mgravell

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 |      - |     - |     - |         - |

mgravell avatar Jul 08 '19 10:07 mgravell

@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 SomeCompilerServicesAttribute subclass, a designated SomethingAttribute that maps the method's return type to a custom builder type".

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.

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 avatar Jul 09 '19 07:07 YairHalberstadt

@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(){}
}

mgravell avatar Jul 10 '19 10:07 mgravell

Related - general enhancements to AsyncMethodBuilder machinery: https://github.com/dotnet/csharplang/discussions/3403

YairHalberstadt avatar Oct 14 '20 06:10 YairHalberstadt

I'm a bit confused about this part of the proposal:

The syntax for applying such an attribute to a method is verbose. This is an advanced feature, but a developer using it frequently could create their own attribute derived from the one they care about, and then use that derived attribute to simplify, e.g.

class Pool<T> : AsyncMethodBuilderAttribute<T>
{
    public Pool() : base(typeof(AsyncValueTaskMethodBuilder<T>)) { }
}
...
[Pool]
internal async ValueTask<int> ExampleAsync() { ... }

I don't know exactly how this could be made to work with the createArguments support, though. The derived attribute could also accept a params array and pass it down to the base, but the compiler would need to be able to recognize it. Maybe a heuristic of special-casing any params object[] at the end of an AsyncMethodBuilder-derived attribute.

First of all, AsyncMethodBuilderAttribute is not generic and the Pool attribute is not used generically, so I'm assuming those two types were not supposed to be generic.

But more importantly: how would the compiler figure out what's inside of the Pool constructor? Wouldn't that effectively require disassembling the IL of a library (if Pool was not in the same assembly)? And how could that even be possible with reference assemblies?

svick avatar Nov 08 '20 20:11 svick

so I'm assuming those two types were not supposed to be generic

The base wasn't supposed to be. That's a typo I'll fix.

how would the compiler figure out what's inside of the Pool constructor?

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.

stephentoub avatar Nov 08 '20 20:11 stephentoub

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.

roji avatar Feb 18 '21 16:02 roji

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<T> will be used a lot on a single command.

Wraith2 avatar Mar 02 '21 11:03 Wraith2

I'm providing custom async type(UniTask, UniTask<T>, UniTaskVoid) and builder for Unity game engine. https://github.com/Cysharp/UniTask This has already been used in many games, this is probably the largest example of a custom Task.

I am providing async UniTaskVoid to use instead of async void. However, this is more difficult to use than async void(requires Forget() to prevent compiler warning, can't be used in Action lambda expressions, etc.) I want to override void by AsyncMethodBuilderOverride.

[AsyncMethodBuilderOverride(typeof(UniTaskVoidMethodBuilder))]
async void FooAsync() { }

// more better syntax
[UniTaskVoid]
async void FooAsync() { }

There have also been proposals at the module level. https://github.com/dotnet/csharplang/pull/4512 I would like to write it this way.

[module: AsyncMethodBuilderOverride(typeof(UniTaskVoidMethodBuilder), typeof(void)]

neuecc avatar Mar 10 '21 09:03 neuecc

I'm providing custom async type(UniTask, UniTask<T>, UniTaskVoid) and builder for Unity game engine. https://github.com/Cysharp/UniTask This has already been used in many games, this is probably the largest example of a custom Task.

I am providing async UniTaskVoid to use instead of async void. However, this is more difficult to use than async void(requires Forget() to prevent compiler warning, can't be used in Action lambda expressions, etc.) I want to override void by AsyncMethodBuilderOverride.

[AsyncMethodBuilderOverride(typeof(UniTaskVoidMethodBuilder))]
async void FooAsync() { }

// more better syntax
[UniTaskVoid]
async void FooAsync() { }

There have also been proposals at the module level. #4512 I would like to write it this way.

[module: AsyncMethodBuilderOverride(typeof(UniTaskVoidMethodBuilder), typeof(void)]

I think AsyncMethodBuilderAttribute should be unsealed so we can inherit it for a cleaner attribute like that:

public class UniTaskVoidAttribute : AsyncMethodBuilderAttribute
{
    public UniTaskVoidAttribute() : base(typeof(UniTaskVoidMethodBuilder)) { }
}
``

timcassell avatar Nov 06 '21 10:11 timcassell

I think AsyncMethodBuilderAttribute should be unsealed so we can inherit it for a cleaner attribute like that:

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.

stephentoub avatar Nov 06 '21 10:11 stephentoub