-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 serializer performance #57327
Merged
Merged
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
bc27b79
Improve serializer performance
steveharter 5467cd9
Change main null check in TryWrite(); other misc
steveharter ee345f6
Fix up entry method coding patterns
steveharter e1d10a7
2: Change main null check in TryWrite(); other misc
steveharter 39bab22
fix assert
steveharter 77bb810
Make IsNull() private static. Other misc non-functional
steveharter d2852ad
ReadSpan() -> ReadFromSpan() naming
steveharter File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,6 +3,7 @@ | |
|
||
using System.Diagnostics; | ||
using System.Diagnostics.CodeAnalysis; | ||
using System.Runtime.CompilerServices; | ||
using System.Text.Json.Serialization.Metadata; | ||
|
||
namespace System.Text.Json.Serialization | ||
|
@@ -143,6 +144,9 @@ internal virtual bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, J | |
/// <remarks>Note that the value of <seealso cref="HandleNull"/> determines if the converter handles null JSON tokens.</remarks> | ||
public abstract T? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options); | ||
|
||
#if NET6_0_OR_GREATER | ||
[MethodImpl(MethodImplOptions.AggressiveOptimization)] | ||
#endif | ||
internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, out T? value) | ||
{ | ||
if (ConverterStrategy == ConverterStrategy.Value) | ||
|
@@ -310,10 +314,13 @@ internal override sealed bool TryReadAsObject(ref Utf8JsonReader reader, JsonSer | |
} | ||
|
||
/// <summary> | ||
/// Overridden by the nullable converter to prevent boxing of values by the JIT. | ||
/// Performance optimization. The 'in' modifier in TryWrite(in T Value) will cause boxing, so this helper method avoids that. | ||
steveharter marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// </summary> | ||
internal virtual bool IsNull(in T value) => value == null; | ||
internal bool IsNull(T value) => value is null; | ||
steveharter marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
#if NET6_0_OR_GREATER | ||
[MethodImpl(MethodImplOptions.AggressiveOptimization)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was not able to squeeze out any performance gains when applying aggressive optimizations to this method in the past. |
||
#endif | ||
internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions options, ref WriteStack state) | ||
{ | ||
if (writer.CurrentDepth >= options.EffectiveMaxDepth) | ||
|
@@ -333,10 +340,14 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions | |
|
||
if ( | ||
#if NET5_0_OR_GREATER | ||
!typeof(T).IsValueType && // treated as a constant by recent versions of the JIT. | ||
// Short-circuit the check against "is not null"; treated as a constant by recent versions of the JIT. | ||
!typeof(T).IsValueType && | ||
#else | ||
!IsValueType && | ||
#endif | ||
// Since we may have checked for a null value above we may have a redundant check here, | ||
// but this seems to be better than trying to cache that value when considering all permutations: | ||
// int?, int?(null value), int, object, object(null value) | ||
value is not null) | ||
{ | ||
|
||
|
@@ -433,11 +444,12 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions | |
|
||
if ( | ||
#if NET5_0_OR_GREATER | ||
!typeof(T).IsValueType && // treated as a constant by recent versions of the JIT. | ||
// Short-circuit the check against ignoreCyclesPopReference; treated as a constant by recent versions of the JIT. | ||
!typeof(T).IsValueType && | ||
#endif | ||
ignoreCyclesPopReference) | ||
{ | ||
// should only be entered if we're serializing instances | ||
// Should only be entered if we're serializing instances | ||
// of type object using the internal object converter. | ||
Debug.Assert(value?.GetType() == typeof(object) && IsInternalConverter); | ||
state.ReferenceResolver.PopReferenceForCycleDetection(); | ||
|
This file contains 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
This file contains 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
This file contains 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
This file contains 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
This file contains 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
Oops, something went wrong.
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.
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.
Out of curiosity, what are some of the optimizations that were being missed without this attribute? I had tried using it a few months back on the
JsonConverter<T>.TryWrite
method (which is also fairly large) and did not receive any perf improvements.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.
I am not sure how to find this out, with tiered jitting and all (e.g. hit a breakpoint after running a 1,000 times to inspect the native code?). Also, I did not run with
crossgen2
yet, but typically that has the same results once the warm-up phase is done.UPDATE: the assembly can be viewed by a checked build and using DOTNET_JitDisasm=methodName.
The results are small when there are a couple properties but should be greater when there are more properties. I had to run benchmarks a few times to make sure noise isn't a factor. I welcome investigation from others on this.
Here's some results with and without
AggressiveOptimization
. Again, I assume the more properties, the more savings. Also I tend to focus on theMin
column more thanMean
.With AO and without
Using a class with 2 string properties; I ran 5 times and took the fastest.
With AO:
Without AO:
Running
ReadJson<LargeStructWithProperties>
andWriteJson<LargeStructWithProperties>
since it has 10 properties. I ran the benchmarks 3 times and took the fastest. The "Stream" cases didn't get any faster -- not sure why, although that does have a different code path in the object converter.With AO:
Without AO:
With AO:
Without AO:
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.
In the end, after running thousands of times, the jitted (native) code may be more or less the same with or without the [AO]. But [AO] should produce faster code sooner.
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.
Can we please avoid [AO]?
Methods with [AO] aren't instrumented so won't be able to benefit from PGO (devirtualize calls, move cold parts of methods further from hot ones, etc) - e.g. run your benchmark again with
DOTNET_ReadyToRun=0
,DOTNET_TC_QuickJitForLoops=1
,DOTNET_TieredPGO=1
with and without[AO]
Also, when a method is promoted to tier1 naturally (without [AO]`) we can:
static readonly
fields to constantscall
tokens and produce better results/cc @AndyAyersMS
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.
[AO] should be avoided. It causes methods to be optimized prematurely. We use it only in a handful of places where we feel like we can't afford to ever run unoptimized code.
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.
Thanks, I'll re-run with those settings.
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.
See #58209 for reverting the 4 usages of [AO]. Note I was going to keep the 2 in
JsonConverter<T>
since there does seem to benefits at least sometimes, but for V7 I think it makes sense to remove them based upon offline discussion with @EgorBo and "DynamicPGO" scenarios. I don't plan on porting that PR to V6.