-
Couldn't load subscription status.
- Fork 5.2k
Perf improvements for small or value-type POCOs #37976
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
Conversation
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.
Note this is for consistency with other code. Also applies to another case of this later.
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.
Note that GetPropertyName is marked with AggressiveInline but LookupProperty is not (it used to be, but there became 4 references to it over time causing code bloat).
| else if (Converter.CanUseDirectReadOrWrite) | ||
| { | ||
| if (!(isNullToken && IgnoreDefaultValuesOnRead && Converter.CanBeNull)) | ||
| if (!isNullToken || !IgnoreDefaultValuesOnRead || !Converter.CanBeNull) |
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.
Changed this for readability (and to make short-circuiting of || obvious).
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs
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
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.
LGTM
src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.Escaping.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.Escaping.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.Escaping.cs
Outdated
Show resolved
Hide resolved
...tem.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs
Show resolved
Hide resolved
...ibraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Helpers.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.Cache.cs
Show resolved
Hide resolved
|
@stephentoub Please excuse me for not providing the code review on time. @steveharter The numbers look really good! Thank you for sharing all the benchmark results! |
For a TechEmPower benchmark which uses a one-property struct, this shows a ~1.2x serialization improvement during serialization.
Note a value-type (struct) POCO is not common and should only be used when there are very few properties -- instead a POCO should be a reference-type (class).
Fixes #36635
Summary of changes:
into avoid unnecessary copies. The more serializable properties a value-type contains, the bigger the savings.AggressiveInliningfast-path changes to since the code path is now internal and specific to this optimization.AggressiveInliningchanges. Both added and removed. The crossgen size of STJ.dll is now 15K smaller (1071K to 1056K).ulongkey avoiding calls toSequenceEquals()when the length is different. This doesn't affect the TechEmPower scenarios.Click to expand for benchmarks
Note the items in the "Slower" section appear to be random differences on my machine.Changes <= 2% are ignored.
summary:
better: 60, geomean: 1.081
worse: 6, geomean: 1.042
total diff: 66
Click to expand for temporary TechEmpower benchmark
Summary: 1.22x faster (when using a cached buffer)
Before:
After:
Code: