Skip to content

Conversation

@layomia
Copy link
Contributor

@layomia layomia commented Sep 1, 2020

Don't take slightly slower code-paths that honor custom number handling when the property/type is not a number or collection of numbers. Cost for the number handling feature is now only paid per number property.

Brings (de)serialization performance of hypothetical object graphs that don't contain numbers when JsonSerializerOptions.NumberHandling != JsonNumberHandling.Strict to par with when the option is the default value (JsonNumberHandling.Strict).

This change is especially useful now that JsonNumberHandling.AllowReadingFromString is being set as a JsonSerializer web default.

Slower diff/base Base Median (ns) Diff Median (ns) Modality
System.Text.Json.Serialization.Tests.WriteJson<HashSet>.SerializeToUtf8B 1.02 6100.48 6227.82
System.Text.Json.Serialization.Tests.WriteJson.SerializeToUtf8Bytes 1.01 125.73 127.45
System.Text.Json.Serialization.Tests.ReadJson.DeserializeFromUtf8Byte 1.00 65975.66 66303.68
System.Text.Json.Serialization.Tests.WriteJson.Serial 1.00 658.27 660.00
Faster base/diff Base Median (ns) Diff Median (ns) Modality
System.Text.Json.Serialization.Tests.WriteJson<Dictionary<String, String>>.Seria 1.08 10510.39 9716.08
System.Text.Json.Serialization.Tests.ReadJson.DeserializeFromUtf8Bytes 1.06 1265.23 1194.23
System.Text.Json.Serialization.Tests.ReadJson<ImmutableDictionary<String, String 1.05 48111.39 45775.44
System.Text.Json.Serialization.Tests.ReadJson<Dictionary<String, String>>.Deseri 1.04 21900.22 21016.85
System.Text.Json.Serialization.Tests.ReadJson.DeserializeFromUtf 1.04 29986.44 28819.17
System.Text.Json.Serialization.Tests.ReadJson<HashSet>.DeserializeFromUt 1.03 12735.50 12329.60
System.Text.Json.Serialization.Tests.ReadJson.Deseria 1.02 1176.97 1150.93
System.Text.Json.Serialization.Tests.ReadJson.DeserializeFromUtf8Bytes 1.02 86.27 84.97
System.Text.Json.Serialization.Tests.WriteJson.SerializeToUtf8By 1.02 16981.86 16729.83

Base = before this change, JsonSerializerOptions.NumberHandling == AllowReadingFromString | WriteAsString
Diff = after this change, JsonSerializerOptions.NumberHandling == AllowReadingFromString | WriteAsString
Benchmark classes contain little or no numbers.

@layomia layomia added area-System.Text.Json breaking-change Issue or PR that represents a breaking API or functional change over a previous release. labels Sep 1, 2020
@layomia layomia added this to the 6.0.0 milestone Sep 1, 2020
@layomia layomia self-assigned this Sep 1, 2020
@layomia
Copy link
Contributor Author

layomia commented Sep 1, 2020

This is a breaking change because applying JsonNumberHandlingAttribute on a collection of non-numbers e.g. List<Poco> was previously (in 5.0) a no-op. Now, InvalidOperationException is thrown.

public class ClassWith_AttributeOnComplexListProperty
{
    // Attribute not allowed here.
    [JsonNumberHandling(JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.WriteAsString)]
    public List<ClassWith_StrictAttribute> MyList { get; set; }

    // Attribute allowed here and will be honored.
    [JsonNumberHandling(JsonNumberHandling.AllowReadingFromString | JsonNumberHandling.WriteAsString)]
    public List<int> MyOtherList { get; set; }
}

Motivation

Even though it's a behavorial no-op to put the attribute on a collection of non-numbers, the serializer would still go down a slower code-path if we allowed the attribute. Generally in the serializer, we throw when a setting is indicated but not honored. A somewhat related example is with preserve-reference handling when object is deserialized with a parameterized ctor. Rather than do a no-op (i.e deserialize as usual without the objects pointing to each other), we throw up front so that the user fully understands how the setting works.

The attribute on a POCO-typed property is truly a no-op and will not influence how number properties are (de)serialized. The serializer doesn't pass options down in that manner for any scenario, but it should be achievable for number handling if desired by many users. This is another reason to throw explicitly now rather than let this be a no-op and then change the behavior in the future.

The ways to influence number handling of a number property today:

  • put the JsonNumberHandlingAttribute on the number property
  • put the attribute on the declaring type of the property
  • set the NumberHandling property on JsonSerializerOptions

Making this change allows us to have clean logic in the serializer today which matches with supported behavior, and also maintain the best perf with this feature on: the JsonNumberHandlingAttribute, when applied to a property/field is only valid on a number or a collection of numbers.

@layomia layomia merged commit 149b235 into dotnet:master Sep 8, 2020
@layomia layomia deleted the number_handling branch September 8, 2020 18:08
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
@ericstj ericstj added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label May 19, 2021
@layomia
Copy link
Contributor Author

layomia commented Nov 2, 2021

Breaking change doc: dotnet/docs#26771.

@layomia layomia removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Nov 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Text.Json breaking-change Issue or PR that represents a breaking API or functional change over a previous release.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants