Add ValueTask<T> returning async APIs to ADO.NET

[roji]

With https://github.com/dotnet/corefx/issues/27445 allowing elimination of allocations in asynchronously-executing async methods, we should look into adding ValueTask-returning counterparts to ADO.NET. This would potentially allow zero-allocation database access.

Here are the current async methods:

Task DbConnection.OpenAsync(...);

Task<DbDataReader> DbCommand.ExecuteReaderAsync(...);
Task<DbDataReader> DbCommand.ExecuteDbDataReaderAsync(...);
Task<object> DbCommand.ExecuteScalarAsync(...);
Task DbCommand.ExecuteNonQueryAsync(...);

Task<bool> DbDataReader.ReadAsync(...);
Task<T> DbDataReader.GetFieldValueAsync<T>(...);
Task<bool> DbDataReader.IsDBNullAsync(...);

Notes:

1. Naming the new methods is going to be complicated. Some parts of corefx are lucky in that they're introducing ValueTask<T> overloads along with Span<T> (e.g. Stream), but ADO.NET has no such upcoming parameter changes/additions. Since there can't be yet another overload that only differs on the return type, and the "standard" name with just the Async suffix is taken, we need to come up with a new naming pattern.

2. There are some missing async methods in ADO.NET in general (e.g. DbCommand.Prepare(), DbConnection.BeginTransactionAsync(...), DbTransaction.CommitAsync(...)...). These can be added directly with return value ValueTask<T> (covered by issue https://github.com/dotnet/corefx/issues/35012). Naming of these new methods should probably be decided in the context of whatever new naming pattern we use for the ValueTask<T> returning variants of the existing async methods.

3. Also note that async APIs that return Task or Task<bool> (such as ReadAsync() and IsDBNullAsync()) can be implemented to return cached Task<T> instances when they complete synchronously to gain efficiency instead of adopting ValueTask<TResult>. This mitigates the need for ValueTask<T> returning variants for most APIs. Also, in terms of allocations, the advantage of ValueTask<T> is much greater the more fine grained the async methods are.

4. Compatibility for providers: It seems desirable to offer a default implementation of the Task-based method that calls AsTask() on the ValueTask<TResult> instance returned by the new API, so that new providers don't need to implement the old Task<TResult> version of the API but instead only implement the more efficient ValueTask<TResult> version. It also seems good not to force existing providers to switch.

---

**Updated by @divega on 1/13/2019 to consolidate with https://github.com/dotnet/corefx/issues/15011.

[divega]

System.Data Triage: We should consider this, but since it isn’t for 2.1, moving to future.

[davidfowl]

Are any of these methods streaming? Would IAsyncEnumerable be interesting here?

[divega]

@davidfowl DbDataReader.Read is streaming. The current API does not resemble IEnumerable (possibly because it predates it 😄), but the new API could use IAsyncEnumerable if the timing is good.

See this was mentioned on aspnet/DataAccessPerformance#32.

[roji]

DbDataReader implement IEnumerable, exposing rows as DbDataRecord. It's an old API (non-generic, sync-only), and the default implementation, which uses DbEnumerator, is also pretty inefficient: it calls DbDataReader.GetValues(), which copies the row's values into an object[] (which gets allocated on each MoveNext()...).

But I actually don't really see a reason not to also have DbDataReader implement IAsyncEnumerable... It could provide a modern and efficient way to stream over the the rows of a resultset. Opened dotnet/corefx#27684 to track.

[GSPP]

Is the allocation overhead really significant when opening a connection or executing a reader? Executing select null against SQL Server on the same machine over shared memory and using synchronous IO takes about 200us. This already is so much more than allocating a few objects and collecting them.

Adding ValueTask versions for reading rows and fields might be valuable.

[roji]

You're right that when OpenAsync() returns asynchronously (i.e. a physical open or wait), the memory overhead is probably negligible. For synchronous invocations (no I/O, pooled idle connection is returned) there is no memory overhead, since OpenAsync() returns a non-generic task which can be cached. So I agree this has more value for DbDataReadet.Read().

However, don't forget that ADO.NET is sometimes used to access databases like Sqlite, where the physical open can be much much faster (no networking), so it could be significant there.

[Grauenwolf]

ValueTask DblDataReader.GetFieldValueAsync(...);
ValueTask DbDataReader.IsDBNullAsync(...);

If we're going to touch this, we might as well fix the IsDBNull problem at the same time. Currently we need to make two calls, once to check for nulls and again to actually read the value. When I profile my ORM, IsDBNull is often flagged as most expensive operation in aggregate. (EDIT: this is no longer true.)

We can probably make it a lot more convenient efficient with methods such as

ValueTask<T?> DblDataReader.GetFieldValueOrNullAsync(...);

[roji]

Depending on the ADO.NET provider implementation, calling IsDBNull() separately from GetFieldValue<T>() isn't necessary a big perf problem (I'm not sure it's the case with Npgsql). But introducing something like GetFieldValueOrNull() (and its async counterpart) would be a good idea regardless, if only for improving the API/usability.

Note that using the async versions of these methods is probably a bad idea unless CommandBehavior.SequentialAccess is specified, since the entire row is pretty much guaranteed to be buffered in memory anyway...

[Grauenwolf]

You're right. When I last benchmarked it on .NET Framework a year or two ago it was a major issue. Now it's barely visible in the performance profiler.

[Grauenwolf]

Still, it would be nice for those who are using ADO.NET directly to not have to write those checks.

[roji]

Absolutely, it would be great if ADO.NET got a bit of a face-lift in terms of API usability, regardless of any perf aspects...

Good to know that perf-wise things are better, there have been a lot of changes in recent two years.

[GSPP]

If this proposed deserialization API is implemented then the ADO.NET APIs will not be very chatty. There will be less of a need for ValueTask. ValueTask can be used internally.

[divega]

@roji @Grauenwolf I would like us to track the idea of GetFieldValueOrNull[Async] as a separate issue. Would you like to create it?

[Grauenwolf]

I haven't read the underlying code yet, so I think someone else would be better suited to proposing an API.

[MgSam]

I vote against mixing ValueTask and Task return types arbitrarily based solely on the age of the method. ValueTask return types should only be highly targeted to methods likely to be called over and over.

I do agree BeginTransactionAsync and CommitTransactionAsync are needed, though they should not be ValueTask.