Perf improvements for small or value-type POCOs#37976
Merged
steveharter merged 3 commits intodotnet:masterfrom Jun 18, 2020
Merged
Perf improvements for small or value-type POCOs#37976steveharter merged 3 commits intodotnet:masterfrom
steveharter merged 3 commits intodotnet:masterfrom
Conversation
steveharter
commented
Jun 16, 2020
Contributor
Author
There was a problem hiding this comment.
Note this is for consistency with other code. Also applies to another case of this later.
steveharter
commented
Jun 16, 2020
Contributor
Author
There was a problem hiding this comment.
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).
steveharter
commented
Jun 17, 2020
| else if (Converter.CanUseDirectReadOrWrite) | ||
| { | ||
| if (!(isNullToken && IgnoreDefaultValuesOnRead && Converter.CanBeNull)) | ||
| if (!isNullToken || !IgnoreDefaultValuesOnRead || !Converter.CanBeNull) |
Contributor
Author
There was a problem hiding this comment.
Changed this for readability (and to make short-circuiting of || obvious).
layomia
reviewed
Jun 18, 2020
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
layomia
approved these changes
Jun 18, 2020
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
Member
|
@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! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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: