-
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
Perf improvements for small or value-type POCOs #37976
Conversation
@@ -62,7 +62,8 @@ protected override void CreateCollection(ref Utf8JsonReader reader, ref ReadStac | |||
return false; | |||
} | |||
|
|||
if (!converter.TryWrite(writer, enumerator.Current, options, ref state)) | |||
object? element = enumerator.Current; |
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.
@@ -48,10 +49,10 @@ internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, | |||
// Read method would have thrown if otherwise. | |||
Debug.Assert(tokenType == JsonTokenType.PropertyName); | |||
|
|||
ReadOnlySpan<byte> unescapedPropertyName = JsonSerializer.GetPropertyName(ref state, ref reader, options); |
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).
@@ -193,7 +191,7 @@ public override bool ReadJsonAndSetMember(object obj, ref ReadStack state, ref U | |||
} | |||
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:
in
to avoid unnecessary copies. The more serializable properties a value-type contains, the bigger the savings.AggressiveInlining
fast-path changes to since the code path is now internal and specific to this optimization.AggressiveInlining
changes. Both added and removed. The crossgen size of STJ.dll is now 15K smaller (1071K to 1056K).ulong
key 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: