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

Allocation-free awaitable async operations with ValueTask<T> and ValueTask #27445

Closed
stephentoub opened this Issue Feb 25, 2018 · 116 comments

Comments

@stephentoub
Member

stephentoub commented Feb 25, 2018

Background

ValueTask<T> is currently a discriminated union of a T and a Task<T>. This lets APIs that are likely to complete synchronously and return a value do so without allocating a Task<T> object to carry the result value. However, operations that complete asynchronously still need to allocate a Task<T>. There is no non-generic ValueTask counterpart today because if you have an operation that completes synchronously and successfully, you can just return Task.CompletedTask, no allocation.

That addresses the 80% case where synchronously completing operations no longer allocate. But for cases where you want to strive to address the 20% case of operations completing asynchronously and still not allocating, you’re forced to play tricks with custom awaitables, which are one-offs, don’t compose well, and generally aren’t appropriate for public surface area. Task and Task<T>, by design, never go from a completed to incomplete state, meaning you can’t reuse the same object; this has many usability benefits, but for APIs that really care about that last pound of performance, in particular around allocations, it can get in the way.

We have a bunch of new APIs in .NET Core 2.1 that return ValueTask<T>s, e.g. Stream.ReadAsync, ChannelReader.ReadAsync, PipeReader.ReadAsync, etc. In many of these cases, we’ve simply accepted that they might allocate; in others, custom APIs have been introduced specific to that method. Neither of these is a good place to be.

Proposal

I have implemented a new feature in ValueTask<T> and a counterpart non-generic ValueTask that lets these not only wrap a T result or a Task<T>, but also another arbitrary object that implements the IValueTaskSource<T> interface (or IValueTaskSource for the non-generic ValueTask). An implementation of that interface can be reused, pooled, etc., allowing for an implementation that returns a ValueTask<T> or ValueTask to have amortized non-allocating operations, both synchronously completing and asynchronously completing.

The enabling APIs

First, we need to add these interfaces:

namespace System.Threading.Tasks
{
    public interface IValueTaskSource
    {
        bool IsCompleted { get; }
        bool IsCompletedSuccessfully { get; }
        void OnCompleted(Action<object> continuation, object state, ValueTaskSourceOnCompletedFlags flags);
        void GetResult();
    }

    public interface IValueTaskSource<out TResult>
    {
        bool IsCompleted { get; }
        bool IsCompletedSuccessfully { get; }
        void OnCompleted(Action<object> continuation, object state, ValueTaskSourceOnCompletedFlags flags);
        TResult GetResult();
    }

    [Flags]
    public enum ValueTaskSourceOnCompletedFlags
    {
        None = 0x0,
        UseSchedulingContext = 0x1,
        FlowExecutionContext = 0x2,
    }
}

An object implements IValueTaskSource to be wrappable by ValueTask, and IValueTaskSource<TResult> to be wrappable by ValueTask<TResult>.

Then we add this ctor to ValueTask<TResult>:

namespace System.Threading.Tasks
{
    public struct ValueTask<TResult>
    {
        public ValueTask(IValueTaskSource<TResult> source);
        ...
    }
}

Then we add a non-generic ValueTask counterpart to ValueTask<TResult>. This mirrors the ValueTask<TResult> surface area, except that it doesn’t have a Result property, doesn’t have a ctor that takes a TResult, uses Task in places where Task<TResult> was used, etc.

namespace System.Threading.Tasks
{
    [AsyncMethodBuilder(typeof(AsyncValueTaskMethodBuilder))]
    public readonly partial struct ValueTask : IEquatable<ValueTask>
    {
        public ValueTask(Task task);
        public ValueTask(IValueTaskSource source);
        public bool IsCanceled { get; }
        public bool IsCompleted { get; }
        public bool IsCompletedSuccessfully { get; }
        public bool IsFaulted { get; }
        public Task AsTask();
        public ConfiguredValueTaskAwaitable ConfigureAwait(bool continueOnCapturedContext);
        public override bool Equals(object obj);
        public bool Equals(ValueTask other);
        public ValueTaskAwaiter GetAwaiter();
        public override int GetHashCode();
        public static bool operator ==(ValueTask left, ValueTask right);
        public static bool operator !=(ValueTask left, ValueTask right);
    }
}

And finally we add the System.Runtime.CompilerServices goo that allows ValueTask to be awaited and used as the return type of an async method:

namespace System.Runtime.CompilerServices
{
    public struct AsyncValueTaskMethodBuilder
    {
        public static AsyncValueTaskMethodBuilder Create();

        public void Start<TStateMachine>(ref TStateMachine stateMachine) where TStateMachine : IAsyncStateMachine;
        public void SetStateMachine(IAsyncStateMachine stateMachine);
        public void AwaitOnCompleted<TAwaiter, TStateMachine>(ref TAwaiter awaiter, ref TStateMachine stateMachine) where TAwaiter : INotifyCompletion where TStateMachine : IAsyncStateMachine;
        public void AwaitUnsafeOnCompleted<TAwaiter, TStateMachine>(ref TAwaiter awaiter, ref TStateMachine stateMachine) where TAwaiter : ICriticalNotifyCompletion where TStateMachine : IAsyncStateMachine;

        public void SetResult();
        public void SetException(Exception exception);
        public ValueTask Task;
    }

    public readonly struct ValueTaskAwaiter : ICriticalNotifyCompletion
    {
        public bool IsCompleted { get; }
        public void GetResult() { }
        public void OnCompleted(Action continuation);
        public void UnsafeOnCompleted(Action continuation);
   }

    public readonly struct ConfiguredValueTaskAwaitable
    {
        public ConfiguredValueTaskAwaiter GetAwaiter();
        public readonly struct ConfiguredValueTaskAwaiter : ICriticalNotifyCompletion
        {
            public bool IsCompleted { get; }
            public void GetResult();
            public void OnCompleted(Action continuation);
            public void UnsafeOnCompleted(Action continuation);
        }
    }
}

Changes to Previously Accepted APIs

At the very least, we would use the ValueTask and ValueTask<T> types in the following previously accepted/implemented APIs that are shipping in 2.1:

  • Pipelines. Instead of pipelines having a custom PipeAwaiter<T> type, it will return ValueTask<T> from the ReadAsync and FlushAsync methods that currently return PipeAwaiter. PipeAwaiter<T> will be deleted. Pipe uses this to reuse the same pipe object over and over so that reads and flushes are allocation-free.
  • Channels. The WaitToReadAsync and WaitToWriteAsync methods will return ValueTask<bool> instead of Task<bool>. The WriteAsync method will return ValueTask instead of Task. At least some of the channel implementations, if not all, will pool and reuse objects backing these value tasks.
  • Streams. The new WriteAsync(ReadOnlyMemory<byte>, CancellationToken) overload will return ValueTask instead of Task. Socket’s new ReceiveAsync/SendAsync methods that are already defined to return ValueTask<int> will take advantage of this support, making sending and receiving on a socket allocation free. NetworkStream will then expose that functionality via ReadAsync/WriteAsync. FileStream will potentially also pool so as to make synchronous and asynchronous reads/writes allocation-free.
  • WebSockets. The new SendAsync(ReadOnlyMemory<byte>, …) overload will return ValueTask instead of Task. Many SendAsync calls just pass back the result from the underlying NetworkStream, so this will incur the benefits mentioned above.

There are likely to be other opportunities in the future as well. And we could re-review some of the other newly added APIs in .NET Core 2.1, e.g. TextWriter.WriteLineAsync(ReadOnlyMemory<char>, ...), to determine if we want to change those from returning Task to ValueTask. The tradeoff is one of Task's usability vs the future potential for additional optimization.

Limitations

Task is powerful, in large part due to its “once completed, never go back” design. As a result, a ValueTask<T> that wraps either a T or a Task<T> has similar power. A ValueTask<T> that wraps an IValueTaskSource<T> can be used only in much more limited ways:

  • The 99.9% use case: either directly await the operation (e.g. await SomethingAsync();), await it with configuration (e.g. await SomethingAsync().ConfigureAwait(false);), or get a Task out (e.g. Task t = SomethingAsync().AsTask();). Using AsTask() incurs allocation if the ValueTask/ValueTask<T> wraps something other than a Task/Task<T>.
  • Once you’ve either awaited the ValueTask/ValueTask<T> or called AsTask, you must never touch it again.
  • With a ValueTask<T> that wraps a Task<T>, today you can call GetAwaiter().GetResult(), and if it hasn’t completed yet, it will block. That is unsupported for a ValueTask<T> wrapping an IValueTaskSource<T>, and thus should be generally discouraged unless you're sure of what it's wrapping. GetResult must only be used once the operation has completed, as is guaranteed by the await pattern.
  • With a ValueTask<T> that wraps a Task<T>, you can await it an unlimited number of times, both serially and in parallel. That is unsupported for a ValueTask<T> wrapping an IValueTaskSource<T>; it can be awaited/AsTask'd once and only once.
  • With a ValueTask<T> that wraps a Task<T>, you can call any other operations in the interim and then await the ValueTask<T>. That is unsupported for a ValueTask<T> wrapping an IValueTaskSource<T>; it should be awaited/AsTask’d immediately, as the underlying implementation may be used for other operation, subject to whatever the library author chose to do.
  • You can choose to explicitly call IsCompletedSuccessfully and then use Result or GetAwaiter().GetResult(), but that is the only coding pattern outside of await/AsTask that’s supported.
    We will need to document that ValueTask/ValueTask<T> should only be used in these limited patterns unless you know for sure what it wraps and that the wrapped object supports what's being done. And APIs that return a ValueTask/ValueTask<T> will need to be clear on the limitations, in hopes of preserving our ability to change the backing store behind ValueTask<T> in the future, e.g. an API that we ship in 2.1 that returns ValueTask<T> around a Task<T> then in the future instead wrapping an IValueTaskSource<T>.

Finally, note that as with any solution that involves object reuse and pooling, usability/diagnostics/debuggability are impacted. If an object is used after it's already been effectively freed, strange/bad behaviors can result.

Why now?

If we don’t ship this in 2.1, we will be unable to do so as effectively in the future:

  • Some methods (e.g. the new Stream.WriteAsync overload) are currently defined to return Task but should be changed to return ValueTask.
  • Some methods return ValueTask<T>, but if we’re not explicit about the limitations of how it should be used, it’ll be a breaking change to modify what it backs in the future.
  • Various types (e.g. PipeAwaiter<T>) will be instant legacy.
  • Prior to .NET Core 2.1, ValueTask<T> was just OOB. It’s now also in System.Private.CoreLib, with core types like Stream depending on it.

Implementation Status

With the exception of pipelines, I have these changes implemented across coreclr and corefx. I can respond to any changes from API review, clean things up, and get it submitted as PRs across coreclr and corefx. Due to the breaking changes in existing APIs, it will require some coordination across the repos.

(EDIT stephentoub 2/25: Renamed IValueTaskObject to IValueTaskSource.)
(EDIT stephentoub 2/25: Changed OnCompleted to accept object state.)

@stephentoub

This comment has been minimized.

@benaadams

This comment has been minimized.

Collaborator

benaadams commented Feb 25, 2018

IValueTaskObject =>IValueTaskAwaitable? e.g.

public interface IValueTaskAwaitable
{
    bool IsCompleted { get; }
    bool IsCompletedSuccessfully { get; }
    void OnCompleted(Action continuation, ValueTaskAwaitableOnCompletedFlags flags);
    void GetResult();
}

public interface IValueTaskAwaitable<out TResult>
{
    bool IsCompleted { get; }
    bool IsCompletedSuccessfully { get; }
    void OnCompleted(Action continuation, ValueTaskAwaitableOnCompletedFlags flags);
    TResult GetResult();
}

[Flags]
public enum ValueTaskAwaitableOnCompletedFlags
{
    None = 0x0,
    UseSchedulingContext = 0x1,
    FlowExecutionContext = 0x2,
}

Similar to the ConfiguredValueTaskAwaitable in the proposal

How are

bool IsCanceled { get; }
bool IsFaulted { get; }
AggregateException Exception { get; }

communicated from the IValueTaskObject/IValueTaskAwaitable as they aren't on the interface?

Do they come via throwing Cancelled vs OtherException on GetResult?

Ripple effects; would go via the statemachine and SetResult()/SetException(Exception exception); but I'm wondering how an exception/cancellation in the IValueTaskObject/IValueTaskAwaitable itself is communicated?

i.e Should they throw from GetResult and the state machine catch it?

e.g.

try
    SetResult(GetResult);
catch Exception
    SetException(ex)
@davidfowl

This comment has been minimized.

Contributor

davidfowl commented Feb 25, 2018

@stephentoub does all of this work on .NET Standard 2.0? I'm assuming yes?

@geoffkizer

This comment has been minimized.

Member

geoffkizer commented Feb 25, 2018

Can you give some details on what the ValueTaskAwaitableOnCompletedFlags do? I assume this is at least partly related to ConfigureAwait?

@omariom

This comment has been minimized.

Contributor

omariom commented Feb 25, 2018

With a ValueTask that wraps a Task, you can await it an unlimited number of times, both serially and in parallel. That is unsupported for a ValueTask wrapping an IValueTaskObject; it can be awaited/AsTask'd once and only once.

Will it make impossible/complicated to use AsTask and then WhenAll/WhenAny?

With a ValueTask that wraps a Task, you can call any other operations in the interim and then await the ValueTask. That is unsupported for a ValueTask wrapping an IValueTaskObject; it should be awaited/AsTask’d immediately, as the underlying implementation may be used for other operation, subject to whatever the library author chose to do.

So the following will be unsupported?

var read = pipe.Reader.ReadAsync();
var write = pipe.Writer.WriteAsync();
await read;
await write;
// or
var write = pipe.Writer.WriteAsync();
var flush= pipe.Writer.FlushAsync();
await write;
await flush;
@svick

This comment has been minimized.

Contributor

svick commented Feb 25, 2018

On one hand, I've been wondering if the new interfaces could be made more general, so that they can serve as the solution to "make Task<T> covariant" (dotnet/roslyn#2981) and "have a generic IAwaitable" (#15469).

On the other hand, all those limitations make using such APIs error-prone and unpleasant, so I've thought if it would make sense to have a separate type for this (e.g. ReusableValueTask/<T>). But then you can't just change the implementation to start using reusable tasks, you have to change the API.

Maybe there is a way to avoid or diminish the limitations? Some options I considered:

  1. ValueTask keeps a version of the IValueTaskObject. If you reuse IValueTaskObject, its version changes. If you then attempt to access the value from ValueTask, you get an exception. (Though this would probably increase the size of ValueTask, which is not good.)
  2. ValueTask has a way to tell IValueTaskObject that it can be reused and ValueTask itself can be Disposed. If you don't Dispose the ValueTask, the IValueTaskObject can't be reused. This will make using ValueTask in a way that reuses IValueTaskObjects harder, but maybe that's acceptable, since it's only for users that need the "last pound of performance"?
  3. Provide a Roslyn analyzer that ensures you're not using ValueTask incorrectly.
@stephentoub

This comment has been minimized.

Member

stephentoub commented Feb 25, 2018

IValueTaskObject =>IValueTaskAwaitable

But it's not awaitable (by design). From a technical perspective, for it to be awaitable, it would need to expose slightly different surface area, and it would need to implement ICriticalNotifyCompletion, which then makes it difficult or impossible to have the same object implement both IValueTaskObject<X> and IValueTaskObject<Y>, which for example pipelines wants/needs to do. From a purity perspective, awaitables are things that expose GetAwaiter, and awaiters provide the IsCompleted/OnCompleted/GetResult surface area. And from an ideological/perf perspective, I really don't want APIs returning an instance of this interface directly. The vast number of calls to the vast number of async APIs actually do complete synchronously, and for such operations it would be a terrible shame if accessing the result incurred a minimum of three interface calls: GetAwaiter(), IsCompleted, GetResult().

How are IsCanceled / Faulted / Exception communicated from the IValueTaskObject/IValueTaskAwaitable as they aren't on the interface?

Exception isn't on ValueTask<T>, it's on Task<T> (or Task rather), so that's not an issue.

I've implemented IsFaulted as IsCompleted && !IsCompletedSuccessfully.

I've made IsCanceled always return false for an IValueTaskObject/IValueTaskObject<T>. That's not how I started this effort, though. I started with there being an IsCanceled on the interface. The problem though is one of performance. It became clear very quickly as I implemented this in various places that calling GetResult is the signal that the object is no longer in use and can be reused, and that means the holder of the ValueTask/ValueTask<T> must not touch the ValueTask again after GetResult has been called. If we need to then interpret an exception that emerges from GetResult as being for either cancellation or failure, as we do for example in ValueTask.AsTask, then we can't call IsCanceled after calling GetResult, which means we need to call it before, which means we need to call it even for a synchronously completed operation, which introduces another interface call on that faster path. Instead, I implemented the same semantics in AsTask that async methods have: exc is OperationCanceledException means the Task is canceled, everything else means faulted.

To me, the design I have seemed like the lesser of two evils. It does lead to an inconsistency, in that with code like:

ValueTask<int> vt = SomeAsync();
bool faulted = vt.IsFaulted;
Task<int> t = vt.AsTask();
bool canceled = t.IsCanceled;

could result in both faulted and canceled being true. But for the 90% use case of:

int i = await SomeAsync();

whether the thing returned from SomeAsync is canceled or faulted is indistinguishable, and for the 9% use case of:

Task<int> t = SomeAsync().AsTask();

you'll never look at the ValueTask and so it won't matter that its view of IsFaulted differs slightly from that of Task, and for the 0.9% optimization use case of:

ValueTask<int> vt = SomeAsync();
int i = vt.IsCompletedSuccessfully ? vt.Result : await vt;

you're not looking at IsFaulted or IsCanceled, so it also doesn't matter. It's only for that 0.1% use case (obviously I'm making up these numbers, but they seem accurate 😄) that you'd see a potential discrepancy. And that didn't seem worth the extra perf hit to me.

(Honestly, I wish we hadn't added IsCanceled to Task or ValueTask... the only reason it ended up there is effectively a legacy reason. Because in .NET 4 we crashed the process if a failed Task had its exception go unobserved, we wanted to special-case cancellation so you didn't have to observe it, but then post-.NET 4 we stopped crashing by default for that anyway, and with async methods, they really end up recombining them into just success or exception.)

Anyway, that's how I ended up here. Do you disagree with the approach or see a flaw in my reasoning?

does all of this work on .NET Standard 2.0? I'm assuming yes?

Yes. Prior to .NET Core 2.1, ValueTask<T> was in the System.Threading.Tasks.Extensions.dll NuGet package, which has a netstandard1.0 build. In .NET Core 2.1, these types also move into System.Private.CoreLib and it's those types that are used for netcoreapp, but System.Threading.Tasks.Extensions retains its netstandard1.0 build. For this proposal, I've added the new types into both System.Private.CoreLib for netcoreapp and System.Threading.Tasks.Extensions for netstandard1.0, so the functionality will be available everywhere. That said, there are optimizations in the netcoreapp implementation not possible in the netstandard1.0 implementation; for example, AsTask is less efficient in the netstandard1.0 implementation.

Can you give some details on what the ValueTaskAwaitableOnCompletedFlags do? I assume this is at least partly related to ConfigureAwait?

Awaiters can implement the INotifyCompletion interface, which provides OnCompleted, or the ICriticalNotifyCompletion interface, which inherits INotifyCompletion and then also provides UnsafeOnCompleted. The only difference between the two is whether they flow ExecutionContext or not, e.g. if you put something into an AsyncLocal<T> before calling OnCompleted, that value will be available in the continuation callback, where if you do so before calling UnsafeOnCompleted, it may not be there because the implementation need not flow the ExecutionContext that contains that AsyncLocal<T> data. I've collapsed that distinction into the FlowExecutionContext flag, so if OnCompleted gets (flags & FlowExecutionContext) != 0, it must flow the context (e.g. use ExecutionContext.Capture() in OnCompleted and then use ExecutionContext.Run to invoke the callback), and if it gets (flags & FlowExecutionContext) == 0, it need not. The 99% case is that the flag won't be set, because ValueTask<T>'s awaiters implement ICriticalNotifyCompletion, and the compiler-generated state machine will call to the async method builder's AwaitUnsafeOnCompleted method if it does. The runtime always flows ExecutionContext across awaits (unless ExecutionContext.SuppressFlow() is called), so the only reason this exists on the awaiter as well is because it's public API surface area that can be called directly rather than only being usable via await. It's effectively legacy that both APIs are exposed rather than being collapsed as I've done, due to a now defunct security model.

UseSchedulingContext is just the bool continueOnCapturedContext value that's passed to ValueTask<T>.ConfigureAwait(bool); it's set if continueOnCapturedContext is true (or if ConfigureAwait wasn't used), and not set if it's false. If it's set, the implementation needs to query for the current scheduling context, and if it exists, use it when executing the callback. Generally this means at least looking at SynchronizationContext.Current, but Task also looks at TaskScheduler.Current, so those are the semantics I've implemented. Another implementation, like in pipelines, might choose to also factor in its pipelines scheduler. So, for example, when OnCompleted is called the implementation will check SynchronizationContext.Current, and if it's non-null/non-default, it'll hold on to it, and then when it's time to invoke the continuation, it'll use that context's Post method to queue the callback back to that SynchronizationContext. This is how work that runs on the UI thread, for example, gets back to the UI thread.

Will it make impossible/complicated to use AsTask and then WhenAll/WhenAny?

No. It'd be perfectly fine to do:

await Task.WhenAll(
    SomeAsync().AsTask(),
    SomeAsync().AsTask(),
    SomeAsync().AsTask());

So the following will be unsupported?

It really depends on the implementation of IValueTaskObject/IValueTaskObject<T>, and APIs that return ValueTask/ValueTask<T> will need to be clear on what's allowed and what's not.

For example, today it's perfectly acceptable to have multiple operations outstanding on a Socket at time, both sends and receives. But while it's allowed, it's quite rare to have multiple reads outstanding or multiple writes outstanding, while it's very common to have a single read outstanding with a single write outstanding. Thus the implementation I've provided of ReceiveAsync and SendAsync that return ValueTask<int> allow for any number of outstanding operations, but only optimize for the single outstanding read / single outstanding write case. The implementation maintains a single reusable IValueTaskObject<int> for receives and a single reusable IValueTaskObject<int> for sends, and the implementation tracks whether GetResult has been called. When a new receive/send operation is issued, the implementation tries to reserve the corresponding singleton, which it can only do if no one else is currently using it; if no one else is using it, then we reuse that singleton for that operation, but if someone else is using it, you get a slower implementation that does allocate. Thus, with code like:

int bytesReceived = await socket.ReceiveAsync(memory, cancellationToken);
bytesReceived += await socket.ReceiveAsync(memory, cancellationToken);
bytesReceived += await socket.ReceiveAsync(memory, cancellationToken);

each of those operations will take the fast, non-allocating path, but with code like:

ValueTask<int> vt1 = socket.ReceiveAsync(memory1, cancellationToken);
ValueTask<int> vt2 = socket.ReceiveAsync(memory2, cancellationToken);
ValueTask<int> vt3 = socket.ReceiveAsync(memory3, cancellationToken);
int bytesReceived = await vt1;
bytesReceived += await vt2;
bytesReceived += await vt3;

the vt1 operation will be non-allocating but the vt2 and vt3 operations both will be. What's explicitly not supported and will very much be a bug on the developer's part is if they touch one of these ValueTask<int> instances after it's already been awaited, e.g. this code is bad:

ValueTask<int> vt1 = socket.ReceiveAsync(memory1, cancellationToken);
await vt1;
await vt1; // BUG BUG BUG

In contrast, for example, System.Threading.Channels has a single-consumer specialized unbounded channel, e.g.

Channel<int> c = Channel.CreateUnbounded<int>(new UnboundedChannelOptions { SingleReader = true });

That implementation explicitly only supports a single reader at a time (with any number of writers), and thus it caches a singleton IValueTaskObject<T> that's reused for every ReadAsync on the channel. It's thus fine to do:

T item1 = await c.Reader.ReadAsync();
T item2 = await c.Reader.ReadAsync();

and fine to do:

ValueTask<T> vt = c.Reader.ReadAsync();
await c.Writer.WriteAsync(producedItem);
T consumedItem = await vt;

but it's very much an error on the developer's part to do:

ValueTask<T> vt1 = c.Reader.ReadAsync();
ValueTask<T> vt2 = c.Reader.ReadAsync(); // BUG BUG BUG

and an error to do:

ValueTask<T> vt = c.Reader.ReadAsync();
await vt;
await vt; // BUG BUG BUG

and an error to do:

ValueTask<T> vt = c.Reader.ReadAsync();
await c.Reader.ReadAsync(); // BUG BUG BUG
await vt; // BUG BUG BUG

In other words, it's really up to the API returning the ValueTask or ValueTask<T> what semantics it wants to provide.

so that they can serve as the solution to "make Task covariant" (dotnet/roslyn#2981) and "have a generic IAwaitable" (#15469).

I explicitly opted away from that, for the reasons outlined earlier in this response.

ValueTask keeps a version of the IValueTaskObject. If you reuse IValueTaskObject, its version changes. If you then attempt to access the value from ValueTask, you get an exception. (Though this would probably increase the size of ValueTask, which is not good.)

I considered the cookie approach. Basically ValueTask/ValueTask<int> would store an int or a long version number, populated into the struct when constructed with an IValueTaskObject, e.g.

public struct ValueTask<T>
{
    public ValueTask(IValueTaskObject<T> obj, long version);
    ...
}

and then all of IValueTaskObject<T>'s members would also take a version value:

public interface IValueTaskObject<out T>
{
    public bool IsCompleted(long version);
    public bool IsCompletedSuccessfully(long version);
    public T GetResult(long version);
    public void OnCompleted(Action continuation, ValueTaskObjectOnCompletedFlags flags, long version);
}

and it would be up to the IValueTaskObject<T> implementation to store a corresponding version and fail the operation if the two cookies didn't match. That's feasible. But as you say, it increases the size of ValueTask and ValueTask<T>, purely for a diagnostic benefit in the case of misuse. Generally I prefer to avoid such overheads, especially since these types end up not only getting returned out of methods, but often stored into the state machines of the async methods consuming them, thus increasing the size of those state machines.

I know @KrzysztofCwalina was a fan of this approach, though, at least in principle.

ValueTask has a way to tell IValueTaskObject that it can be reused and ValueTask itself can be Disposed

I'm not understanding this... wouldn't it be the other way around, the IValueTaskObject<T> telling the ValueTask<T> whether the IValueTaskObject<T> can be reused? (I don't know what it means for a ValueTask<T> to know whether it itself is reusable or not. Regardless, I don't think that really addresses the issue. The 99% case here is:

await SomeAsync();

and await doesn't Dispose of the thing it's given. Rather, it calls GetResult as the last step, so effectively GetResult is the signal that the object can then be reused. If a Dispose were required, that would effectively nullify any benefits here, as to enable reuse you'd need to change all such awaits to instead be:

using (ValueTask<int> vt = SomeAsync())
{
    await vt;
}

and not only is that klunky and more expensive, I actually would expect it would lead to more errors, as it promotes storing the ValueTask<T> into a local, making it more likely you'll accidentally reuse it.

Provide a Roslyn analyzer that ensures you're not using ValueTask incorrectly.

That might be reasonable, though I expect it would likely have both false negatives and false positives. Happy to be proven wrong, though.

@svick

This comment has been minimized.

Contributor

svick commented Feb 25, 2018

I'm not understanding this... wouldn't it be the other way around, the IValueTaskObject<T> telling the ValueTask<T> whether the IValueTaskObject<T> can be reused?

What I meant is that the ValueTask<T> would tell IValueTaskObject<T> whether the IValueTaskObject<T> can be reused. The code would effectively be something like:

struct ValueTask<T>
{
    public void Dispose() => valueTaskObject?.Release();
}

The 99% case here is: await SomeAsync(); […] If a Dispose were required, that would effectively nullify any benefits here

The proposed design means that the 99% case is efficient, but also makes it easy to write buggy code.

I don't like to sacrifice safety for performance, because performance often doesn't matter much, while safety always matters. And with this proposal, any API that returns ValueTask<T> becomes dangerous.

What if it wasn't Dispose(), but instead a method on ValueTask<T> that returns another awaitable?

await SomeAsync(); // allocates

var vt1 = SomeAsync();
await vt1;
await vt1; // ok

await SomeAsync().IKnowICantReuseTheReturnedValueISwear(); // does not allocate

var vt2 = SomeAsync().IKnowICantReuseTheReturnedValueISwear();
await vt2;
await vt2; // bug

(With IKnowICantReuseTheReturnedValueISwear obviously having a different name.)

This makes it much easier to use than Dispose(), but only becomes dangerous if you explicitly opt-in.


I guess the important question here is: will almost all code that uses ValueTask<T> be performance critical (to the point that allocations matter)? If ValueTask<T> will always be only part of APIs that are specifically designed for performance, then it's okay if it's a bit dangerous.

But if ValueTask<T> will become part of general-purpose APIs (that won't have Task<T>-returning alternatives), then I think it's important to make sure it's not dangerous.

@benaadams

This comment has been minimized.

Collaborator

benaadams commented Feb 25, 2018

Anyway, that's how I ended up here. Do you disagree with the approach or see a flaw in my reasoning?

No; just checking :)

ValueTask<T> vt = c.Reader.ReadAsync();
await vt;
await vt; // BUG BUG BUG

Could ValueTask both clear its IValueTaskObject on return from first await (so second await failed) and throw if a second action is queued to OnCompleted if an action is already present?

It wouldn't cater for struct copies; but would that reduce the 0.1% to 0.001%? 😉

@stephentoub

This comment has been minimized.

Member

stephentoub commented Feb 25, 2018

Could ValueTask both clear its IValueTaskObject on return from first await (so second await failed) and throw if a second action is queued to OnCompleted if an action is already present?

It's a readonly struct, and even if it weren't, by definition there's a copy when getting the awaiter from it, so each time you await it you're seeing a different copy of the struct. An implementation of IValueTaskObject could try to put some safeguards in place, but what it can detect would be limited by how much reuse it wants to allow.

ValueTask would tell IValueTaskObject whether the IValueTaskObject can be reused

It already does that: if GetResult hasn't been called, the implementation must be considered still in use, and an implementation could choose to throw or take a slower path or some such thing.

The proposed design means that the 99% case is efficient, but also makes it easy to write buggy code.

I disagree. Look at all of the code that uses tasks that's been written in the last few years; 99% of it just awaits the operation directly... it's fairly rare to get a handle to the task and do something other than await it. Sometimes you use operations with combinators, but notice combinators aren't exposed here for ValueTask and ValueTask<T>... you need to use AsTask to then use Task.WhenAll/Any, etc. Storing the operation into a local and doing something other than awaiting it is generally only done when you're trying to do something special, often for performance reasons, and then you already need to know what you're doing.

And with this proposal, any API that returns ValueTask becomes dangerous.

Many APIs in .NET (and any programming language for that matter) can be misused in a dangerous way. Access a Dictionary<TKey,TValue> from multiple concurrent operations (which is really easy to accidentally do, if for example you put one in a static and access it from multiple web requests) and you can corrupt it easily, in ways that can lead to corrupted data, infinite loops, and other messes. Issue multiple ReadAsync calls on an arbitrary Stream implementation without waiting for the previous one to complete, and you'll likely corrupt data (a few streams allow this, but most don't, and many don't implement any safeties). Using an object after it's been Disposed; some objects detect and throw for this, some don't. Using ArrayPool and accidentally returning the same array multiple times, or forgetting to return the arrays, or continuing to use an array after it's already been returned. Etc.

because performance often doesn't matter much

If performance doesn't matter for a method, it can simply return a Task or Task<T>. Performance with operations like Stream.Read/WriteAsync often really does matter. Notice I've explicitly not said that all methods should return ValueTask and ValueTask<T> moving forward, as I don't believe they should.

This makes it much easier to use than Dispose(), but only becomes dangerous if you explicitly opt-in.

I don't see how that's a feasible design. By the time SomeAsync returns to the caller, it's already scheduled the asynchronous operation, and already needs to know what object it's talking to upon completion.

@mattnischan

This comment has been minimized.

mattnischan commented Feb 25, 2018

Doesn't this create the situation in which the consumer of your API now needs to understand how to properly await based on the kind of ValueTask you're returning? I'm seeing this have huge benefits for code that, say, deserializes data off the network, but if I return a ValueTask that wraps an IValueTaskObject, how do I indicate to my consumers to avoid the various pitfalls, like double awaiting?

With custom awaitables like PipeAwaiter, it just isn't as easy for API consumers to do the wrong thing. Definitely not as easy to compose, I agree, and writing custom awaiters has its own share of pitfalls, but again, those aren't exposed to the consumer.

@stephentoub

This comment has been minimized.

Member

stephentoub commented Feb 25, 2018

Doesn't this create the situation in which the consumer of your API now needs to understand how to properly await based on the kind of ValueTask you're returning?

A consumer of an API needs to know the semantics of that API, including details about how its return type behaves. By default, you need to assume if you get a ValueTask/ValueTask<T> from an arbitrary method that you know nothing else about that you can either await it once or use AsTask once, before doing other arbitrary things with that same API. If you know more about the semantics of the method from which you got it, you may be able to get away with more.

With custom awaitables like PipeAwaiter, it just isn't as easy for API consumers to do the wrong thing.

Why not?

var result1 = pipeReader.ReadAsync();
await result1;
var result2 = pipeReader.ReadAsync();
await result1;

How does misuse difficulty change here based on the concrete type of the var?

@alefranz

This comment has been minimized.

alefranz commented Feb 25, 2018

This is a really great work @stephentoub !
I recently had a performance bottleneck on an application that this would allow to solve in a much cleaner way.

With the increase in focus on performance in the .NET ecosystem I can see this used not only in core pieces of the stack or performance critical business applications, but also to a wider set of library and applications.
However the drawbacks and risk of run-time errors due to misuses will either discourage the use of this construct or risk affecting the stability/correctness lot of applications and, although it will be a developer error, I believe this can be bad for reputation.

I believe the semantic of ValueTask/ValueTask<T> should be defined with certainty (await once or AsTask once) and enforced by the compiler. Maybe also the fact that only one operation should be in-flight should be a common convention and lead to a warning otherwise.

Have you also considered not making it compatible with Task (given also some other differences like the IsCancelled behaviour) and make this a first class concept, eventually with is own keywords (e.g. asynconce/awaitonce)?

@stephentoub

This comment has been minimized.

Member

stephentoub commented Feb 25, 2018

This is a really great work

Thanks.

I believe the semantic of ValueTask/ValueTask should be defined with certainty (await once or AsTask once)

Essentially that's what I'm saying. If you happen to know more about the implementation, you might be able to get away with more.

Have you also considered not making it compatible with Task

ValueTask<T> already shipped. We can't change that (nor would I want to).

@mattnischan

This comment has been minimized.

mattnischan commented Feb 25, 2018

In this particular case, because PipeAwaiter has a specific set of usage semantics. It isn't a Task or ValueTask, so that gives me heads up that I need to understand what it supports.

I'm not at all against the idea, it's just that my consumer has to understand how I'm using ValueTask internally and then make decisions based on that. It isn't intuitive that there are situations which are totally fine with a normally wrapped ValueTask but not with one that wraps IValueTaskObject. If the usages are distinct, it feels like the types should also be distinct. AsyncValueTask, if you will.

Now, that may be non-trivial for a myriad of other reasons.

@stephentoub

This comment has been minimized.

Member

stephentoub commented Feb 25, 2018

In this particular case, because PipeAwaiter has a specific set of usage semantics. It isn't a Task or ValueTask, so that gives me heads up that I need to understand what it supports.

I'd argue this isn't about PipeAwaiter, it's about the API that returned it, and that's the case regardless of the return type. Further, I'd argue that the 99% case (and it's really probably more the 99.99% case) is no one knows or cares what the return type is because they simply await it. It's just compiler goo to let you write await SomethingAsync(), just like you generally don't care whether an enumerable type exposes a struct enumerable and instead just foreach (var item in enumerable)... that you might be using a nested struct type isn't relevant, other than for its perf implications.

@mattnischan

This comment has been minimized.

mattnischan commented Feb 25, 2018

BTW Stephen, despite my hesitations (I write a lot of low level stuff that junior devs need to be able to easily consume for our system), this is a really great idea and awesome stuff which could solve a lot of the "data coming off the wire" async allocation scenarios.

@alefranz

This comment has been minimized.

alefranz commented Feb 25, 2018

Have you also considered not making it compatible with Task

ValueTask already shipped. We can't change that (nor would I want to).

Sorry, I meant this new feature.
So not making it an extension of ValueTask<T> but a new concept (as @mattnischan is suggesting) and even more obvious with different keywords (which eventually behind the scene can do something similar to Dispose as @svick was suggesting)

@stephentoub

This comment has been minimized.

Member

stephentoub commented Feb 25, 2018

So not making it an extension of ValueTask but a new concept (as @mattnischan is suggesting)

Whatever this thing is, it's going to need to wrap Tasks, as the vast majority of these async APIs are going to still return tasks under the covers. And it's going to need to be as efficient as possible with Task, which means it shouldn't go through an interface to get to it, plus the fact that there'd be no way to make it work with netstandard2.0 if Task needed to implement another interface to make that work.

At that point, you're just introducing another type with the exact same support as ValueTask<T> and just calling it something else. ValueTask<T> has only ever been released in an out-of-band NuGet package, and use is limited. I think it's perfectly reasonable to say now that you need to assume this conservative view of what you can do with a ValueTask<T>.

Further, even with something that was given a different name, there are still going to be differences between APIs that return one, as it's up to the implementation what level of reuse is possible. As I noted earlier, Socket for example lets you make any number of calls to receive/send data, and doing so won't invalidate a previously returned ValueTask<T>, but doing so with PipeReader will. NetworkStream's override of ReadAsync supports one thing, but DeflateStream's override of ReadAsync supports another (and that's true today even with them both returning the same type), and they necessarily need to return the same type. You simply need to understand the semantics of the method you're using; that's always been true and will continue to be true; the return type is largely irrelevant.

I do not see how shipping a different type addresses safety concerns here. It might address the small inconsistency around IsCanceled, but I don't believe that's an important issue necessitating introducing another type.

If we introduced a differently named shared type for this, ValueTask<T> will become completely defunct, with no reason to ever use it, and all of the same issues will exist with the newly named type.

@benaadams

This comment has been minimized.

Collaborator

benaadams commented Feb 25, 2018

I'm happy with it; your initial warnings were more scary than how it would match to use cases.

i.e. normally you await an operation prior to initializing the same operation, and with Task.When requiring the .AsTask() cast it should cover most scenarios (with docs).

IValueTaskObject sounds overly generic though; mainly due to Object which doesn't suggest the methods. IValueTaskResultSource or similar would be better?

Bikeshedding on names aside, thank you for taking time to explain 😄

@alefranz

This comment has been minimized.

alefranz commented Feb 25, 2018

Thanks @stephentoub for the comprehensive explanation.
I personally would prefer if the semantic was on the safe side, discouraging reuse so that it could be eventually enforced by the compiler in future (as you explained having different types for the scenarios will not help, unless you have completely different APIs/keywords, but as you said then it can't be delivered to the current ecosystem)
I agree that you should understand the semantic of the operation you invoke, but its generally better if the semantic can be made explicit in the contract, similar to the evolution of the language to have nullable reference types.
I'm probably just worrying too much, sorry for polluting this thread.

@stephentoub

This comment has been minimized.

Member

stephentoub commented Feb 25, 2018

IValueTaskResultSource or similar would be better

I like IValueTaskSource.

your initial warnings were more scary than how it would match to use cases.

Yeah 😄 I just wanted to be upfront about potential concerns. If I was actually scared of it, we wouldn't be having this conversation as I wouldn't have opened the issue. 😉

Thanks @stephentoub for the comprehensive explanation

You're very welcome. Thank you for participating!

eventually enforced by the compiler

What does that look like? How does an awaitonce keyword prevent:

var t = SomeAsync();
var s = t;
awaitonce t;
awaitonce s;

?

@maksimkim

This comment has been minimized.

maksimkim commented Feb 25, 2018

Does it also make sense to provide a way to create ValueTask() from Exception instance to have allocation-free equivalent for Task.FromException(...). This might be useful for cases where operation should fail synchronously (so no IValueTaskObject instance allocation or acquire from reusable pool is required) but for some reason it's inconvenient/impossible to just throw and force all calling parties to wrap the call with try/catch. Basically same reason to use as for Task.FromException.

@unicomp21

This comment has been minimized.

unicomp21 commented Feb 25, 2018

@stephentoub Nice! You the man!!! On the high performance side, this will open up all sorts of opportunities.

@stephentoub

This comment has been minimized.

Member

stephentoub commented Feb 25, 2018

Does it also make sense to provide a way to create ValueTask() from Exception instance to have allocation-free equivalent for Task.FromException(...)

The only way I know of to do that would be to make ValueTask bigger, which would incur cost for all uses. I don't think that's a desirable trade-off. Exceptions are already very expensive.

@marksmeltzer

This comment has been minimized.

marksmeltzer commented Jul 22, 2018

@juepiezhongren

This comment has been minimized.

juepiezhongren commented Jul 22, 2018

@marksmeltzer for iValueTaskSource, ValueTask is not just for pseudo-async senarios!

@marksmeltzer

This comment has been minimized.

marksmeltzer commented Jul 22, 2018

@juepiezhongren

This comment has been minimized.

juepiezhongren commented Jul 22, 2018

@marksmeltzer it is best that ivalueTaskSource is allocated in pooling heap where init and gc shall be manually done. Task.WhenAll and WhenAny definitely need to be tuned or new api to be done, considering that Task and ValueTask methods would be used together for sync.

@stephentoub
i believe there r a lot more work is required, considering async's 5 main senarios:

  1. IO; 2) Net; 3) Timer; 4) long Computing; 5) UI;
    where vTask with IValueTaskSource will definitely bring a lot of benefits;

btw, TaskCompletionSource is required to be reusable and pool-cached

@juepiezhongren

This comment has been minimized.

juepiezhongren commented Jul 22, 2018

@juepiezhongren

This comment has been minimized.

juepiezhongren commented Oct 12, 2018

@stephentoub ,Task.Delay is too often used, ValueTask.Delay with IValueTaskSource instance injected is really a strongly-needed API

@stephentoub

This comment has been minimized.

Member

stephentoub commented Oct 12, 2018

@juepiezhongren, I do not understand what that would solve here. What costs do you believe would be avoided in doing so? If you believe it's important, your best bet is to code it up and show measurements of exactly how it would improve things and by how much.

@juepiezhongren

This comment has been minimized.

juepiezhongren commented Oct 12, 2018

@stephentoub just make less alloc(wont delay create new task instance? maybe i got some mistakes), task.delay is great already now

@stephentoub

This comment has been minimized.

Member

stephentoub commented Oct 12, 2018

Task.Delay will not only create a Task, it'll also create a Timer, a TimerHolder, and a TimerQueueTimer as part of the underlying implementation. But a ValueTask.Delay would incur the latter three as well, and it would need some object to be the IValueTaskSource. Either that object would need to be allocated, in which case it might as well just be Task, or it would need to be pooled in some way, which brings with it its own costs... managing a process-wide object pool and doing it well is not easy (or necessarily cheap).

@juepiezhongren

This comment has been minimized.

juepiezhongren commented Oct 12, 2018

ur answer is perfect, thx.
in order to make delay totally free-alloc, some complete re-implementation is needed

@benaadams

This comment has been minimized.

Collaborator

benaadams commented Oct 12, 2018

Could use it for a reoccurring awaitable Task.Delay type loop; reusing the same timer?

@juepiezhongren

This comment has been minimized.

juepiezhongren commented Oct 12, 2018

i've heard @davidfowl has done some things for delay

@stephentoub

This comment has been minimized.

Member

stephentoub commented Oct 12, 2018

There are certainly things you can layer on top, e.g. you can coalesce if you're willing to trade off accuracy, e.g. https://blogs.msdn.microsoft.com/pfxteam/2011/12/03/coalescing-cancellationtokens-from-timeouts/.

Could use it for a reoccurring awaitable Task.Delay type loop; reusing the same timer?

Sure, but that brings with it other problems, e.g. when you're done with it, who disposes of the underlying Timer? At that point you're not going to want to use ValueTask, because you're going to want an API that lets you control such things.

@stephentoub

This comment has been minimized.

Member

stephentoub commented Oct 12, 2018

(One addendum... I forgot I previously made a change so that Task.Delay just goes straight to the TimerQueueTimer and skips allocating the Timer and the TimerHolder objects. Part of dotnet/coreclr#14527.)

@juepiezhongren

This comment has been minimized.

juepiezhongren commented Oct 12, 2018

so, valueTask is possible!!!! Great Stephen

@benaadams

This comment has been minimized.

Collaborator

benaadams commented Oct 12, 2018

Sure, but that brings with it other problems, e.g. when you're done with it, who disposes of the underlying Timer?

Could wrap the source in a using e.g.

using (var timer = new PeriodicAsyncTimer(period: 1000))
{
    timer.Start();
    while (!token.IsCancellationRequested)
    {
        await timer;

       // Do stuff
   }
}

Whereas going via period on timer directly its harder to dispose as its just a callback (need access to timer via whatever is injected as state; or an external disposal)

@stephentoub

This comment has been minimized.

Member

stephentoub commented Oct 12, 2018

so, valueTask is possible!!!!

@benaadams' suggestion would be that ValueTask.Delay would still allocate, you could just reuse that same ValueTask awaiting it multiple times. But, FWIW, I think that's a bad precedent to set. We do not want to suggest to developers that they can await the same ValueTask object repeatedly... in fact we explicitly say not to do that... many APIs that return ValueTask will very clearly break if that's done, and that would be the case if, for example, ValueTask.Delay were to pool. We could probably get away with a ValueTask.Delay only allocating one object rather than two, but does that really move the needle? After all, even in a "tight" loop just awaiting Task.Delay(1), on Windows that's generally only going to happen every 15ms. We should not be adding new APIs just because we can.

@stephentoub

This comment has been minimized.

Member

stephentoub commented Oct 12, 2018

Could wrap the source in a using

Sure. Then it's no longer a ValueTask.

@benaadams

This comment has been minimized.

Collaborator

benaadams commented Oct 12, 2018

Could wrap the source in a using

Sure. Then it's no longer a ValueTask.

D'oh, I was thinking it of it using the IValueTaskSource and returning ValueTasks but there's no need for that as it can just be a regular awaitable.

@juepiezhongren

This comment has been minimized.

juepiezhongren commented Oct 12, 2018

@stephentoub to make me self-control timer is a good idea, for a lot of delays are together with while, so this is a perfect solution

@juepiezhongren

This comment has been minimized.

juepiezhongren commented Oct 12, 2018

while{
    await Task.Delay(50);
}

VTaks + IValueTaskSource is just for regular-pattern await senarios, things like while-delay really need a VTask solution, in my opinion.

@stephentoub

This comment has been minimized.

Member

stephentoub commented Oct 12, 2018

things like this really need a VTask solution. In my opinion,

Why? You have a scenario where the 20 small allocations / second that might save you is critical?

@juepiezhongren

This comment has been minimized.

juepiezhongren commented Oct 12, 2018

Just follow your philosophy in this article, current words it's not about perf.

var myTimer=new Timer().....
while(true){
    await ValueTask.Delay(10,myTimer);
.......
}
myTimer.Dispose();

but, this is not bad

@marksmeltzer

This comment has been minimized.

marksmeltzer commented Oct 12, 2018

@juepiezhongren

This comment has been minimized.

juepiezhongren commented Oct 12, 2018

@marksmeltzer DotNetCore, in my mind, is somthing that will implement everything and every dream out of Midori & Singularity, 20 times per second is not important for server app, but is for system.

@marksmeltzer

This comment has been minimized.

marksmeltzer commented Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment