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.

[divega]

@Grauenwolf stating the problem with the current API would be enough. But no worries.

[Grauenwolf]

I'm ok with BeginTransactionAsync and CommitTransactionAsync not being ValueTask.

Really it is only DBDataReader that I care regarding ValueTask because it is in a tight loop.

[divega]

@roji I wonder how this would look like now that we have a better understanding of the guidance re ValueTask<T> vs. Task<T> (from dotnet/efcore#15184).

It seems to me that only the API on DbDataReader is worth considering.

[roji]

@divega I agree - for the execution APIs which never return synchronously, it doesn't make much sense to think about ValueTask. I will update the description soon to reflect this.

There's also dotnet/corefx#35012 (adding missing async methods, as opposed to ValueTask counterparts to existing async methods). It seems that proposal already follows the guidance - the only ValueTask-returning methods are DisposeAsync() (so coming from IAsyncDisposable) and CloseAsync(), which is almost identical to DisposeAsync().

[divega]

@roji perhaps we should get everything we want from this issue copied into dotnet/corefx#35012 and bring that one to API review. I am saying this because the scope of this issue (#27682) is probably now small enough that it wouldn't affect the chances of dotnet/corefx#35012. I also think it helps have a more holistic view during the API review.

[divega]

Though there is still the naming problem 😄

[roji]

Yeah, and for GetFieldValue there's also (in theory) still the question of better nullability handling (even though right now we don't seem to have anything better). That's why it may be good to handle them differently - I think dotnet/corefx#35012 can be brought to design review right away.

[divega]

I added api-ready-for-review on dotnet/corefx#35012. Let's at least discuss the ValueTask<bool> version of ReadAsync there.

[roji]

@divega a small note after the recent discussions: ValueTask<bool> seems to be the same case as ValueTask, in that it provides no value for synchronous completion: Task is already cached (only two possible values). It may still make sense to add ValueTask<bool> overloads in case a provider ever decides to optimize the asynchronous completion case (via IValueTaskSource) - although at this point that seems pretty unlikely, given the effort required (see @stephentoub's comment quoted in dotnet/efcore#15184 (comment)).

We could defer adding those APIs until an easier alternative is introduced (as is planned for some point) - since until then the ValueTask<bool> version will actually be a bit slower than the Task one (due to the overhead of the added layer etc.). What do you think?

However, in any case we still have GetFieldValueAsync() which would benefit from an overload returning a ValueTask<T>.

[divega]

Yes @roji, however DbDataReader.ReadAsync is exactly like IAsyncEnumerable<T>.MoveNextAsync, which is already optimized for the async completion case. I suspect someone willing to optimize it could reuse ManualResetValueTaskSourceCore<TResult> and duplicate the logic used to build async iterators.

I am hoping that in the API review meeting we can get the right people to ask about this.

[roji]

@divega I'll try to investigate a bit how that works. But you're that in any case, even if today it's difficult to benefit from the async completion optimization, it may become easier in the future, and this specific API is definitely a place where that may make sense. As you say, raising this in the API review meeting would probably get us the answers we need.

Regardless, I'll soon update the description to reflect this discussion.

[GSPP]

An argument can be made for providing the highest possible performance alternative (likely ValueTask): Who still uses raw ADO.NET these days? That is rare. Normally, people use an ORM or a thin wrapper such as Dapper. Inconvenient APIs are hidden from most callers.

[MgSam]

@GSPP Tons and tons of people still use ADO.NET. There are huge classes of developers that refuse to use anything that is not shipped with .NET. Not to mention all the legacy code out there.

[roji]

@GSPP can you explain how your argument that users don't usually use ADO.NET directly relates to this issue? At least at this point it isn't really recommended to add APIs everywhere that return non-generic ValueTask, as it is very unlikely that this would help perf in any way (it would actually degrade perf in some cases compared to returning non-generic Task).

There's also a direction of thought about making ADO.NET more usable directly (as opposed to usable only via other layers).

[jaredpar]

VB didn’t get Task likes?

Correct. It is still limited to defining async methods that return Task / Task<T>. It can consume ValueTask / ValueTask<T>, or any other type that implements the awaiter pattern.

https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQBsQDdhJiAligHwEkwAHAewRigAIBlAT1jjAFgAoEiq2x5sAHQAVABYI4EACa4AdgHNh0ANZQOABQCuwFLgDGNAMIpotAxxoWam7XpoBBKAxn6AYhucxc5GTQCyAChh7WiFlAEpgmlCoJXNLeLsAdwhcIJg4iwBRGUkaNw8vGQyaYusdfQcnV3ddT28/ACZAyIA1CBQNOGilCIcaNo6u5WKE5NSadPZ47Nz82sKOGcNjKCggA==

[Grauenwolf]

Do we need to ensure VB can be used to implement these interfaces/abstract classes? Jonathan Allen 619-933-8527

…

________________________________ From: Jared Parsons <notifications@github.com> Sent: Wednesday, September 4, 2019 1:36:26 PM To: dotnet/corefx <corefx@noreply.github.com> Cc: Jonathan Allen <grauenwolf@gmail.com>; Mention <mention@noreply.github.com> Subject: Re: [dotnet/corefx] Add ValueTask<T> returning async APIs to ADO.NET (#27682) VB didn’t get Task likes? Correct. It is still limited to defining async methods that return Task / Task<T>. It can consume ValueTask / ValueTask<T>, or any other type that implements the awaiter pattern. https://sharplab.io/#v2:EYLgtghgzgLgpgJwDQBsQDdhJiAligHwEkwAHAewRigAIBlAT1jjAFgAoEiq2x5sAHQAVABYI4EACa4AdgHNh0ANZQOABQCuwFLgDGNAMIpotAxxoWam7XpoBBKAxn6AYhucxc5GTQCyAChh7WiFlAEpgmlCoJXNLeLsAdwhcIJg4iwBRGUkaNw8vGQyaYusdfQcnV3ddT28/ACZAyIA1CBQNOGilCIcaNo6u5WKE5NSadPZ47Nz82sKOGcNjKCggA== — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://github.com/dotnet/corefx/issues/27682?email_source=notifications&email_token=ACRCUTMRMTTOYFUH6N7O7YLQH7WZVA5CNFSM4ETLMPM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD54LRBY#issuecomment-528005255>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ACRCUTP65RC2X2J33RZ65WDQH7WZVANCNFSM4ETLMPMQ>.

[jaredpar]

There is nothing preventing VB from implementing a member that has a ValueTask return type. VB just can't do it using the Async function style. If an Async style method was needed then a simple thunking approach can be used to put the logic in an Async style method and call that in a ValueTask returning method directly.

[Grauenwolf]

Sounds reasonable to me. Not ideal, but how many database drivers are written in VB. Jonathan Allen 619-933-8527

…

________________________________ From: Jared Parsons <notifications@github.com> Sent: Wednesday, September 4, 2019 3:03:31 PM To: dotnet/corefx <corefx@noreply.github.com> Cc: Jonathan Allen <grauenwolf@gmail.com>; Mention <mention@noreply.github.com> Subject: Re: [dotnet/corefx] Add ValueTask<T> returning async APIs to ADO.NET (#27682) There is nothing preventing VB from implementing a member that has a ValueTask return type. VB just can't do it using the Async function style. If an Async style method was needed then a simple thunking approach can be used to put the logic in an Async style method and call that in a ValueTask returning method directly. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://github.com/dotnet/corefx/issues/27682?email_source=notifications&email_token=ACRCUTO7LSFO3PXMBUPD7WDQIABAHA5CNFSM4ETLMPM2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD54T6GY#issuecomment-528039707>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ACRCUTO4MCOXNNVCSRTU6XLQIABAHANCNFSM4ETLMPMQ>.

[dotnet-policy-service]

Due to lack of recent activity, this issue has been marked as a candidate for backlog cleanup. It will be closed if no further activity occurs within 14 more days. Any new comment (by anyone, not necessarily the author) will undo this process.

This process is part of our issue cleanup automation.

[dotnet-policy-service]

This issue will now be closed since it had been marked no-recent-activity but received no further activity in the past 14 days. It is still possible to reopen or comment on the issue, but please note that the issue will be locked if it remains inactive for another 30 days.

[Wraith2]

I'm not sure if we want this closed. It would be very useful to have ValueTask field value getters at least. We can make them provider specific if we need to but it would be preferable not to. The problem is naming. The name GetFieldValueValueTaskAsync<T> is clumsy and the best alternative I can come up with is GetValueTask2Async<T>

[roji]

Reopening to keep this on our radar... FWIW I think GetFieldValueAsync() is generally rarely needed, since it's mainly useful when CommandBehavior.SequentialAccess is specified (i.e. big rows being streamed); in the regular/default mode, the entire row is buffered anyway (and can be read in random order), so using the sync API (GetFieldValue) is perfectly fine.

But yeah, for sequential access mode we should indeed have a non-allocating API - which would ideally also improve the usability around nulls as well.

[Wraith2]

The knowledge you need to have to understand that using GetFieldValue is probably ok is quite high. Enabling users in async contexts to casually use await GetFieldValue2Async without exposing them to the current high costs of Task versions is desirable.

[roji]

Fair enough, though I wouldn't call the costs of Task allocations "high" for the typical application. In any case I agree this is a good thing to do.