-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Improve deserialization perf for case-insensitive and missing-property cases #35848
Conversation
Tagging subscribers to this area: @jozkee |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
src/libraries/System.Text.Json/src/System/Text/Json/JsonEncodedText.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.Cache.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/PropertyRef.cs
Outdated
Show resolved
Hide resolved
_preserveHandlingOnSerialize = preserveHandlingOnSerialize; | ||
_preserveHandlingOnDeserialize = preserveHandlingOnDeserialize; | ||
_shouldReadPreservedReferences = preserveHandlingOnDeserialize == PreserveReferencesHandling.All; | ||
_shouldWritePreservedReferences = preserveHandlingOnSerialize == PreserveReferencesHandling.All; | ||
} | ||
|
||
internal bool ShouldReadPreservedReferences() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could these be internal properties instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes; didn't change. cc @jozkee
...ystem.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.Cache.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.Cache.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
Outdated
Show resolved
Hide resolved
@@ -245,7 +251,7 @@ private void InitializeConstructorParameters(ConstructorInfo constructorInfo) | |||
|
|||
// One object property cannot map to multiple constructor | |||
// parameters (ConvertName above can't return multiple strings). | |||
parameterCache.Add(jsonParameterInfo.NameAsString, jsonParameterInfo); | |||
parameterCache.Add(jsonPropertyInfo.NameAsString!, jsonParameterInfo); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we should assert that jsonPropertyInfo.NameAsString
and jsonParameterInfo.NameAsString
are equal (I can do this in a follow up)
Test failures in runtime |
~1.75x faster deserialization of a simple POCO when:
JsonSerializerOptions.PropertyNameCaseInsensitive
is true AND[JsonPropertyName]
, or the property naming policy) ANDIn addition, similar perf results for property-misses cases (property names in JSON do not match any property).
Also there are less cache memory allocations for property names. This also has a startup CPU improvement - a simple POCO resulted in a .5ms startup improvements on first deserialization (~23.5ms to ~23.0ms). Private bytes savings were also verified using Process Explorer (very long property names were used).
Fixes #30789 and the increased startup perf and reduced memory consumption helps with 5.0 serialization goals per dotnet/designs#113.
In addition, there are less stack-based allocations for case-insensitive and missing-property scenarios (shown below in the benchmarks).
This may help with ASP.NET deserialization scenarios since they enable case-insensitive by default (cc @pranavkm). However, since ASP.NET also uses a camel-casing property naming policy, the benefit will not occur if the incoming JSON exactly matches what was produced by the camel-casing policy, as would be the case if all serialization occurs with System.Text.Json.
No measurable perf regressions on existing deserialization benchmarks (within +- 3%). Todo: add a case-insensitive and missing-property benchmark.
The main CPU benefit comes from avoiding the dictionary-based lookup for case-insensitive and missing-property scenarios and instead use the array-backed cache. The previous code would also unnecessarily grow and max out that array (64 elements) in certain cases before using the dictionary.
During testing, it was found that "missing properties" in JSON that are added to extension data did not properly support JsonPath semantics (used when a
JsonException
is thrown to report the invalid property). That was fixed and a test added.Below are benchmarks for a simple 4-property POCO test class that has property names > 7 bytes.