dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.98k stars 4.66k forks source link

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

Open EgorBo opened 1 year ago

EgorBo commented 1 year ago

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. https://github.com/dotnet/runtime/blob/50c9dca330d2c8e7eab6e3ca65673fa02c1f1950/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/DateOnlyConverter.cs#L60-L63

ghost commented 1 year ago

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](https://github.com/aspnet/Benchmarks/blob/main/src/BenchmarksApps/TodosApi/Todo.cs#L11) 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. https://github.com/dotnet/runtime/blob/50c9dca330d2c8e7eab6e3ca65673fa02c1f1950/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/DateOnlyConverter.cs#L60-L63
Author: EgorBo
Assignees: -
Labels: `area-System.Text.Json`, `untriaged`
Milestone: -
eiriktsarpalis commented 1 year ago

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 commented 1 year ago

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](https://github.com/aspnet/Benchmarks/blob/main/src/BenchmarksApps/TodosApi/Todo.cs#L11) 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. https://github.com/dotnet/runtime/blob/50c9dca330d2c8e7eab6e3ca65673fa02c1f1950/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/DateOnlyConverter.cs#L60-L63
Author: EgorBo
Assignees: -
Labels: `area-System.Buffers`, `untriaged`
Milestone: -
tannergooding commented 1 year ago

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: https://github.com/dotnet/runtime/issues/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 commented 1 year ago

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 commented 1 year ago

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](https://github.com/aspnet/Benchmarks/blob/main/src/BenchmarksApps/TodosApi/Todo.cs#L11) 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. https://github.com/dotnet/runtime/blob/50c9dca330d2c8e7eab6e3ca65673fa02c1f1950/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/DateOnlyConverter.cs#L60-L63
Author: EgorBo
Assignees: -
Labels: `area-System.Text.Json`, `untriaged`
Milestone: -
eiriktsarpalis commented 1 year ago

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

eiriktsarpalis commented 1 year ago

Addressed in part by https://github.com/dotnet/runtime/pull/86931.

eiriktsarpalis commented 1 year ago

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