Skip to content
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

ADO.NET APIs force needless Task allocations #24067

Closed
roji opened this issue Nov 7, 2017 · 4 comments
Closed

ADO.NET APIs force needless Task allocations #24067

roji opened this issue Nov 7, 2017 · 4 comments
Milestone

Comments

@roji
Copy link
Member

roji commented Nov 7, 2017

The ADO.NET API added async methods (e.g. DbDataReader.ReadAsync(), DbDataReader.NextResultAsync()) before ValueTask existed. Unfortunately this means that these methods return Task instances, which are heap-allocated, although in many (most?) cases the ADO.NET driver will already have the next row or resultset cached in its internal buffer (this is the case for Npgsql).

Ideally new API methods would be introduced, which would be identical in every way except that they return a ValueTask rather than a Task (how these new methods would be named is a depressing question...). Note that the above two examples may not be the only ones that could benefit from this.

@benaadams
Copy link
Member

As they are boolean returns you can use cached true/false tasks and only go async when you need more data; something like:

private readonly static Task<bool> TrueTask = Task.FromResult(true);
private readonly static Task<bool> FalseTask = Task.FromResult(false);

public overrides Task<bool> ReadAsync(CancellationToken cancellationToken) 
{ 
    if (!NeedsMoreData)
    {
        return TryMoveNextRow() ? TrueTask : FalseTask;
    }
    else
    {
        return ReadAsyncAwaited(cancellationToken);
    }
}


private async Task<bool> ReadAsyncAwaited(CancellationToken cancellationToken) 
{ 
    await ReadMoreData(cancellationToken);

    return TryMoveNextRow();
}

@roji
Copy link
Member Author

roji commented Nov 7, 2017

@benaadams of course, silly of me to have forgotten this. In fact, unless I'm mistaken an async method that returns Task<bool> implicitly returns a cached instance rather than instantiating. Thanks.

@roji roji closed this as completed Nov 7, 2017
@benaadams
Copy link
Member

You'll still pay the cost of the async statemachine; so may be better to split into two methods (as above) for a Task returning sync version that requires no extra data, and then have it call the async version when you need to get more data

@roji
Copy link
Member Author

roji commented Nov 7, 2017

Yep, Npgsql has this pattern in some performance-critical paths (if data is cached return that, otherwise call an async method to load).

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 19, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants