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

Add ADO.NET support for Utf8String #28966

Closed
bgrainger opened this issue Mar 14, 2019 · 3 comments
Closed

Add ADO.NET support for Utf8String #28966

bgrainger opened this issue Mar 14, 2019 · 3 comments
Milestone

Comments

@bgrainger
Copy link
Contributor

Utf8String is being added to .NET: #933 dotnet/corefx#35989

There may be opportunities to use this type in the ADO.NET API, particularly for database protocols/providers that use UTF-8 on the wire.

The most obvious enhancement seems like:

public abstract class DbDataReader
{
    public virtual Utf8String GetUtf8String(int ordinal) => new Utf8String(GetString(ordinal));
    public virtual Utf8String GetUtf8String(string name) => GetUtf8String(GetOrdinal(name));
}

Perhaps less realistically, a new DbCommand.CommandTextUtf8 property could be added, which would be preferred over DbCommand.CommandText if it were set.

public abstract class DbCommand
{
    public virtual Utf8String CommandTextUtf8
    {
        get => CommandText is null ? null : new Utf8String(CommandText);
        set => CommandText = value?.ToString();
    }
}

Finally, implementations should be encouraged to support an Utf8String as the value of DbParameter.Value.

Are there any other opportunities or easy wins?

Related: #28135, mysql-net/MySqlConnector#618

@stephentoub
Copy link
Member

Utf8String is being added to .NET: #933 dotnet/corefx#35989

Note that Utf8String is still experimental; that implementation is not planned to be officially supported in .NET Core 3.0.

Are there any other opportunities or easy wins?

Thanks for suggesting this particular addition. As we did with span, as an initial starting point, rather than opening individual issues for each possible place that Utf8String might be useful, I personally think it'd be better if someone (@GrabYourPitchforks?) went through the core frameworks and proposed a summary for all of the initial places we'd want to see Utf8String exposed (and why); that can then be discussed and reviewed holistically rather than piecemeal.

@GrabYourPitchforks
Copy link
Member

I agree with @stephentoub. We have some ideas on what might benefit from being enlightened about any new types, but I haven't gotten around to publishing that document yet because it needs significant cleanup. One thing we'll really need to be mindful of is that we don't just take this type and splat it everywhere thoughout the Framework. It instead should have some sort of measurable value.

As a concrete example, we've been playing with the idea of a modified Utf8Regex class that would support UTF-8 only. It would not be bolted on to the existing Regex class (which is implemented in terms of String all the way down), as doing so would probably not get the performance gains we want.

I mention this in the context of the work item proposed here because it has similar performance concerns. In the DbDataReader example, the most straightforward thing to do is what you already proposed: add new virtual methods that delegate to the authoritative string-based implementations. But now there's a subtle problem: the Utf8String methods become slower than the string methods (because they're wrappers around these methods but with extra work), and given the abstraction layer there's no real way for the caller to know whether calling this overload is more expensive or less expensive than calling the string-based method.

This doesn't mean that DbDataReader is a bad candidate type for this. But I wanted to convey some of the subtleties of this and why we're being slow / deliberate as we proceed here. It is heartening though to see such enthusiasm for it! :)

@stephentoub
Copy link
Member

I'm going to close this in the meantime. This can/should be factored in when doing the all-up analysis. Thanks!

@msftgits msftgits transferred this issue from dotnet/corefx Feb 1, 2020
@msftgits msftgits added this to the 3.0 milestone Feb 1, 2020
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 13, 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

4 participants