Change relevant async APIs to return ValueTask instead of Task

[ajcvickers]

Following discussion on #14551

[ajcvickers]

@divega and @roji To add details from the internal threads on this.

[roji]

Some general guidance given by @stephentoub:

ValueTask/ValueTask<T> is much more limited in how you can consume it (e.g. you have to first convert it to a task to be able to do things like WhenAny), and more error prone (if anyone does anything with it other than awaiting it immediately, it’s most likely a bug in their code). It also comes with its own expenses, e.g. it takes more room to store in a state machine, so just the presence of “await valueTask” makes the state machine bigger than if it were “await task”, and optimizations around structs (and structs that wrap other structs) aren’t where we’d like them to be. So you really only want to use it when the pros outweigh the cons. The benefit is avoiding allocation when:

• Your method is likely to be used on a hot path and allocation overhead matters, and
• Your method returns a T and is likely to complete synchronously and that T isn’t cacheable, or
• Your method returns asynchronously and the allocation in that case is so important that you’re willing to hand-roll a manual IValueTaskSource implementation.

This is why I say you should default to Task/Task<T>, and only opt for ValueTask/ValueTask<T> if you prove to yourself the perf benefits are worth the usability negatives and the potential perf negatives.

And specifically about non-generic ValueTask:

I think it’ll be much more common to see new APIs returning ValueTask<T> rather than ones returning ValueTask. The benefits to the former are much more significant (albeit depending on the T there are potentially more perf costs, since the ValueTask<T> carries around a T field plus the Task<T> field), since it has benefits for synchronous completion; the latter only has benefits for asynchronous completion, and even those are much harder to realize. So I wouldn’t be concerned about EF/ADO.NET APIs returning ValueTask<T> in places where you expect them to be used on hot paths; I’d be more skeptical of APIs returning the non-generic ValueTask. It’s unfortunately really hard to leverage the non-generic ValueTask’s benefits today, and as such there are only a handful of places in corefx where we do… in fact, off the top of my head, the only one I can think of is NetworkStream.WriteAsync.

Additional thought about abstractions such as IAsyncDisposable/IAsyncEnumerable... in some cases, the API being designed is extremely abstract and has no knowledge of concrete implementations and their perf profile. In these cases adopting ValueTask is the right choice, since it allows a specific implementation to achieve the highest perf possible (e.g. via IValueTaskSource). Within EF Core we generally don't have that kind of abstractions (although we still need to consider IAsyncEnumerable).

[roji]

@divega @ajcvickers I think a good next step would be to list some major APIs and discuss concretely. We may want to make a distinction between APIs which never return synchronously (the majority?) and those which may return synchronously. As a good example for the latter, I think there's a clear case for modifying DbContext.FindAsync() to return ValueTask instead of Task (opened #15221).

Conversely SaveChanges() seems like a good example of something which should continue to return a Task - it never returns synchronously and in any case it doesn't return anything.

[stephentoub]

@roji, FYI, your paste of my comments ends up rendering without <T>s, making it difficult to understand statements like I think it’ll be much more common to see new APIs returning ValueTask rather than ones returning ValueTask that should be I think it’ll be much more common to see new APIs returning ValueTask<T> rather than ones returning ValueTask 😉

[roji]

@stephentoub oops, thanks... Edited to make the angle brackets visible, everything does make a bit more sense now :)

[roji]

Here's a first pass listing important/public APIs with my opinion of what should be done, any comments welcome.

Seems that it should return ValueTask

• AddAsync(): Convert to ValueTask. Always completes synchronously unless special value generators are set up (e.g. hi-lo).
• FindAsync(): Convert to ValueTask. Completes synchronously if the object is already tracked.
• Value generator APIs: Convert to async, most will never do I/O.

Seems that it should return Task

• AddRangeAsync(): Returns non-generic Task, so already doesn't allocate on sync completion. We probably wouldn't optimize async completion with IValueTaskSource.
• RelationalDatabaseFacadeExtensions, migration-related: Completely non-perf-sensitive.
• RelationalDatabaseFacadeExtensions.OpenConnectionAsync(): Returns non-generic Task, so already doesn't allocate on sync completion. We probably wouldn't optimize async completion with IValueTaskSource.
• ExecutionStrategyExtensions.ExecuteAsync(). Never completes synchronously, not worth optimizing with IValueTaskSource.
• All reverse engineering: Completely non-perf-sensitive.
• DatabaseFacade.Ensure{Created,Deleted}(): Completely non-perf-sensitive.
• RelationalDatabaseFacadeExtensions.ExecuteSqlCommand(): Never completes synchronously, probably not worth optimizing with IValueTaskSource. ADO.NET API returns Task for now.

Discussion needed

• SaveChangesAsync(). Only returns synchronously when no changes needed to be saved, in which case a cached 0-value Task can be returned if we really want to. Otherwise never returns synchronously. Seems like we wouldn't optimize this with IValueTaskSource, so this should probably continue to return Task.
• RelationalDatabaseFacadeExtensions.BeginTransactionAsync() should discuss, same as https://github.com/dotnet/corefx/issues/35012.
• All LINQ operators in EntityFrameworkQueryableExtensions: let's first understand exactly what's going on with IAsyncEnumerable, the new C# 8 support and Ix.Async.

[roji]

Submitted #15232 for AddAsync(), FindAsync() and the value generators. We can identify other APIs in the comment above and continue discussion. In general, a memory profiling session of various common scenarios of EF Core would be a good idea.

[ajcvickers]

@roji Looks good; thanks for doing this analysis.

[roji]

Discussion in design meeting confirms analysis in #15184 (comment), and no additional APIs currently seem like they'd benefit from returning ValueTask.

[roji]

@ajcvickers @divega this was missing the 3.0.0-preview4 milestone - added it.