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

Consider using IUtf8SpanParseable/IUtf8SpanFormattable in STJ's built-in converters. #84305

Open
EgorBo opened this issue Apr 4, 2023 · 9 comments
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@EgorBo
Copy link
Member

EgorBo commented Apr 4, 2023

MinimalAPI benchmark has a DateOnly field in an object it then serializes to utf8 json.

It seems that STJ doesn't have a fast-path DateOnly -> UTF8 like it has for e.g. DateTime and DateTimeOffset. Instead, it does DateOnly -> UTF16 string and then converts it to Utf8.

Span<char> buffer = stackalloc char[FormatLength];
bool formattedSuccessfully = value.TryFormat(buffer, out int charsWritten, "O", CultureInfo.InvariantCulture);
Debug.Assert(formattedSuccessfully && charsWritten == FormatLength);
writer.WriteStringValue(buffer);

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Apr 4, 2023
@ghost
Copy link

ghost commented Apr 4, 2023

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

MinimalAPI benchmark has a DateOnly field in an object it then serializes to utf8 json.

It seems that STJ doesn't have a fast-path DateOnly -> UTF8 like it has for e.g. DateTime and DateTimeOffset. Instead, it does DateOnly -> UTF16 string and then converts it to Utf8.

Span<char> buffer = stackalloc char[FormatLength];
bool formattedSuccessfully = value.TryFormat(buffer, out int charsWritten, "O", CultureInfo.InvariantCulture);
Debug.Assert(formattedSuccessfully && charsWritten == FormatLength);
writer.WriteStringValue(buffer);

Author: EgorBo
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

STJ uses bespoke formatting logic in order to support formatting dates directly to UTF-8. I wouldn't be too keen to repeat this for DateOnly, or whatever other type we want to support out of the box. Instead, we should consider adding DateOnly and TimeOnly overloads to the Utf8Formatter class so the converter can use that in newer TFMs.

@ghost
Copy link

ghost commented Apr 5, 2023

Tagging subscribers to this area: @dotnet/area-system-buffers
See info in area-owners.md if you want to be subscribed.

Issue Details

MinimalAPI benchmark has a DateOnly field in an object it then serializes to utf8 json.

It seems that STJ doesn't have a fast-path DateOnly -> UTF8 like it has for e.g. DateTime and DateTimeOffset. Instead, it does DateOnly -> UTF16 string and then converts it to Utf8.

Span<char> buffer = stackalloc char[FormatLength];
bool formattedSuccessfully = value.TryFormat(buffer, out int charsWritten, "O", CultureInfo.InvariantCulture);
Debug.Assert(formattedSuccessfully && charsWritten == FormatLength);
writer.WriteStringValue(buffer);

Author: EgorBo
Assignees: -
Labels:

area-System.Buffers, untriaged

Milestone: -

@eiriktsarpalis eiriktsarpalis changed the title MinimalApi: DateOnly json serialization Consider adding DateOnly and TimeOnly support to the Utf8Formatter class. Apr 5, 2023
@eiriktsarpalis eiriktsarpalis changed the title Consider adding DateOnly and TimeOnly support to the Utf8Formatter class. Consider adding DateOnly and TimeOnly support to the Utf8Formatter/Parser classes. Apr 5, 2023
@tannergooding
Copy link
Member

Worth noting we approved IUtf8SpanFormattable and IUtf8SpanParsable. The plan is to implement those on all the primitive/built-in types for .NET 8, including DateOnly and TimeOnly: #81500

Utf8Formatter/Parser support may still be beneficial since it has slightly different semantics (stops at the first invalid character, rather than fails at the first invalid character). However, that sounds like it may not be an overly important distinction for this scenario.

@stephentoub
Copy link
Member

#84469 adds IUtf8SpanFormattable to DateOnly.

But the time we're done with implementing IUtf8SpanFormattable everywhere that currently implements ISpanFormattable, I expect Utf8Formatter will effectively be obsolete.

@ghost
Copy link

ghost commented Apr 9, 2023

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

MinimalAPI benchmark has a DateOnly field in an object it then serializes to utf8 json.

It seems that STJ doesn't have a fast-path DateOnly -> UTF8 like it has for e.g. DateTime and DateTimeOffset. Instead, it does DateOnly -> UTF16 string and then converts it to Utf8.

Span<char> buffer = stackalloc char[FormatLength];
bool formattedSuccessfully = value.TryFormat(buffer, out int charsWritten, "O", CultureInfo.InvariantCulture);
Debug.Assert(formattedSuccessfully && charsWritten == FormatLength);
writer.WriteStringValue(buffer);

Author: EgorBo
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

In that case, I'm reassigning this back to STJ and scoping this to adding IUtf8SpanFormattable /IUtf8SpanParseable support in STJ's converters.

@eiriktsarpalis eiriktsarpalis changed the title Consider adding DateOnly and TimeOnly support to the Utf8Formatter/Parser classes. Consider using IUtf8SpanParseable/IUtf8SpanFormattable in STJ's built-in converters. Apr 9, 2023
@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Apr 9, 2023
@eiriktsarpalis eiriktsarpalis added this to the 8.0.0 milestone Apr 9, 2023
@eiriktsarpalis eiriktsarpalis added the enhancement Product code improvement that does NOT require public API changes/additions label Apr 9, 2023
@eiriktsarpalis
Copy link
Member

Addressed in part by #86931.

@eiriktsarpalis
Copy link
Member

Moving to 9.0, since at this point we'll have less time to react to potential regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

4 participants