Fix perf regression by using ReferenceHandlerStrategy field in JsonSe…

…rializerOptions
dotnet · Jan 26, 2021 · 1905462 · 1905462
1 parent 2e55d73
commit 1905462
Show file tree

Hide file tree

Showing 15 changed files with 51 additions and 37 deletions.
diff --git a/src/libraries/System.Text.Json/src/System.Text.Json.csproj b/src/libraries/System.Text.Json/src/System.Text.Json.csproj
@@ -177,6 +177,7 @@
     <Compile Include="System\Text\Json\Serialization\ReadStackFrame.cs" />
     <Compile Include="System\Text\Json\Serialization\ReferenceHandler.cs" />
     <Compile Include="System\Text\Json\Serialization\ReferenceHandlerOfT.cs" />
+    <Compile Include="System\Text\Json\Serialization\ReferenceHandlingStrategy.cs" />
     <Compile Include="System\Text\Json\Serialization\ReferenceResolver.cs" />
     <Compile Include="System\Text\Json\Serialization\ReflectionEmitMemberAccessor.cs" />
     <Compile Include="System\Text\Json\Serialization\ReflectionMemberAccessor.cs" />

diff --git a/...on/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs b/...on/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs
@@ -133,7 +133,7 @@ internal sealed override bool OnTryRead(
                 }
 
                 // Handle the metadata properties.
-                bool preserveReferences = JsonSerializer.IsPreserveReferencesEnabled(options);
+                bool preserveReferences = options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve;
                 if (preserveReferences && state.Current.ObjectState < StackFrameObjectState.PropertyValue)
                 {
                     if (JsonSerializer.ResolveMetadataForJsonObject<TCollection>(ref reader, ref state, options))
@@ -272,7 +272,7 @@ internal sealed override bool OnTryWrite(
             {
                 state.Current.ProcessedStartToken = true;
                 writer.WriteStartObject();
-                if (JsonSerializer.IsPreserveReferencesEnabled(options))
+                if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve)
                 {
                     if (JsonSerializer.WriteReferenceForObject(this, dictionary, ref state, writer) == MetadataPropertyName.Ref)
                     {

diff --git a/...n/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs b/...n/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs
@@ -90,7 +90,7 @@ internal override bool OnTryRead(
             {
                 // Slower path that supports continuation and preserved references.
 
-                bool preserveReferences = JsonSerializer.IsPreserveReferencesEnabled(options);
+                bool preserveReferences = options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve;
                 if (state.Current.ObjectState == StackFrameObjectState.None)
                 {
                     if (reader.TokenType == JsonTokenType.StartArray)
@@ -236,7 +236,7 @@ internal sealed override bool OnTryWrite(
                 if (!state.Current.ProcessedStartToken)
                 {
                     state.Current.ProcessedStartToken = true;
-                    if (JsonSerializer.IsPreserveReferencesEnabled(options))
+                    if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve)
                     {
                         MetadataPropertyName metadata = JsonSerializer.WriteReferenceForCollection(this, value, ref state, writer);
                         if (metadata == MetadataPropertyName.Ref)

diff --git a/....Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs b/....Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs
@@ -75,7 +75,7 @@ internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert,
                 // Handle the metadata properties.
                 if (state.Current.ObjectState < StackFrameObjectState.PropertyValue)
                 {
-                    if (JsonSerializer.IsPreserveReferencesEnabled(options))
+                    if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve)
                     {
                         if (JsonSerializer.ResolveMetadataForJsonObject<T>(ref reader, ref state, options))
                         {
@@ -233,7 +233,7 @@ internal sealed override bool OnTryWrite(
             if (!state.SupportContinuation)
             {
                 writer.WriteStartObject();
-                if (JsonSerializer.IsPreserveReferencesEnabled(options))
+                if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve)
                 {
                     if (JsonSerializer.WriteReferenceForObject(this, objectValue, ref state, writer) == MetadataPropertyName.Ref)
                     {
@@ -288,7 +288,7 @@ internal sealed override bool OnTryWrite(
                 if (!state.Current.ProcessedStartToken)
                 {
                     writer.WriteStartObject();
-                    if (JsonSerializer.IsPreserveReferencesEnabled(options))
+                    if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve)
                     {
                         if (JsonSerializer.WriteReferenceForObject(this, objectValue, ref state, writer) == MetadataPropertyName.Ref)
                         {

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceHandler.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceHandler.cs
@@ -5,7 +5,7 @@ namespace System.Text.Json.Serialization
 {
     internal sealed class IgnoreReferenceHandler : ReferenceHandler
     {
-        public IgnoreReferenceHandler() => UsePreserveSemantics = false;
+        public IgnoreReferenceHandler() => HandlingStrategy = ReferenceHandlingStrategy.IgnoreCycle;
 
         public override ReferenceResolver CreateResolver() => new IgnoreReferenceResolver();
     }

diff --git a/...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.WriteCore.cs b/...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.WriteCore.cs
@@ -20,7 +20,7 @@ internal sealed override bool WriteCoreAsObject(
                 }
 
                 // Root object is a boxed value type, we need to push it to the reference stack before it gets unboxed here.
-                if (value != null && JsonSerializer.IsIgnoreCyclesEnabled(options))
+                if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycle && value != null)
                 {
                     state.ReferenceResolver.PushReferenceForCycleDetection(value);
                 }

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs
@@ -196,7 +196,8 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali
                         ref reader);
                 }
 
-                if (CanBePolymorphic && JsonSerializer.IsPreserveReferencesEnabled(options) && value is JsonElement element)
+                if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve &&
+                    CanBePolymorphic && value is JsonElement element)
                 {
                     // Edge case where we want to lookup for a reference when parsing into typeof(object)
                     // instead of return `value` as a JsonElement.
@@ -303,20 +304,17 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions
                 ThrowHelper.ThrowJsonException_SerializerCycleDetected(options.EffectiveMaxDepth);
             }
 
-            bool ignoreCyclesEnabled = JsonSerializer.IsIgnoreCyclesEnabled(options);
-            bool ignoreCyclesPopReference = false;
-
-            if (CanBeNull && IsNull(value))
+            if (CanBeNull && !HandleNullOnWrite && IsNull(value))
             {
-                if (!HandleNullOnWrite)
-                {
-                    // We do not pass null values to converters unless HandleNullOnWrite is true. Null values for properties were
-                    // already handled in GetMemberAndWriteJson() so we don't need to check for IgnoreNullValues here.
-                    writer.WriteNullValue();
-                    return true;
-                }
+                // We do not pass null values to converters unless HandleNullOnWrite is true. Null values for properties were
+                // already handled in GetMemberAndWriteJson() so we don't need to check for IgnoreNullValues here.
+                writer.WriteNullValue();
+                return true;
             }
-            else if (ignoreCyclesEnabled && !IsValueType)
+
+            bool ignoreCyclesPopReference = false;
+            if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycle &&
+                !IsValueType && !IsNull(value))
             {
                 ReferenceResolver resolver = state.ReferenceResolver;
                 // Write null to break reference cycles.
@@ -363,9 +361,10 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions
                     JsonConverter jsonConverter = state.Current.InitializeReEntry(type, options);
                     if (jsonConverter != this)
                     {
-                        if (ignoreCyclesEnabled && jsonConverter.IsValueType)
+                        if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycle &&
+                            jsonConverter.IsValueType)
                         {
-                            // For boxed value types: push the value before it gets unboxed.
+                            // For boxed value types: push the value before it gets unboxed on TryWriteAsObject.
                             state.ReferenceResolver.PushReferenceForCycleDetection(value);
                             ignoreCyclesPopReference = true;
                         }

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs
@@ -138,8 +138,9 @@ public override bool GetMemberAndWriteJson(object obj, ref WriteStack state, Utf
         {
             T value = Get!(obj);
 
-            if (JsonSerializer.IsIgnoreCyclesEnabled(Options) && !Converter.IsValueType &&
-                value != null && state.ReferenceResolver.ContainsReferenceForCycleDetection(value))
+            if (Options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycle &&
+                !Converter.IsValueType && value != null &&
+                state.ReferenceResolver.ContainsReferenceForCycleDetection(value))
             {
                 // If a reference cycle is detected, treat value as null.
                 value = default!;

diff --git a/...em.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs b/...em.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs
@@ -79,7 +79,7 @@ internal static ReadOnlySpan<byte> GetPropertyName(
                 unescapedPropertyName = propertyName;
             }
 
-            if (JsonSerializer.IsPreserveReferencesEnabled(options))
+            if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve)
             {
                 if (propertyName.Length > 0 && propertyName[0] == '$')
                 {

diff --git a/...ystem.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleMetadata.cs b/...ystem.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleMetadata.cs
@@ -84,11 +84,5 @@ internal static MetadataPropertyName WriteReferenceForCollection(
 
             return writtenMetadataName;
         }
-
-        internal static bool IsPreserveReferencesEnabled(JsonSerializerOptions options)
-            => options.ReferenceHandler?.UsePreserveSemantics ?? false;
-
-        internal static bool IsIgnoreCyclesEnabled(JsonSerializerOptions options)
-            => !options.ReferenceHandler?.UsePreserveSemantics ?? false;
     }
 }
diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs
@@ -89,6 +89,7 @@ public JsonSerializerOptions(JsonSerializerOptions options)
 
             Converters = new ConverterList(this, (ConverterList)options.Converters);
             EffectiveMaxDepth = options.EffectiveMaxDepth;
+            ReferenceHandlingStrategy = options.ReferenceHandlingStrategy;
 
             // _classes is not copied as sharing the JsonClassInfo and JsonPropertyInfo caches can result in
             // unnecessary references to type metadata, potentially hindering garbage collection on the source options.
@@ -487,9 +488,13 @@ public ReferenceHandler? ReferenceHandler
             {
                 VerifyMutable();
                 _referenceHandler = value;
+                ReferenceHandlingStrategy = value?.HandlingStrategy ?? ReferenceHandlingStrategy.None;
             }
         }
 
+        // The cached value used to determine if ReferenceHandler should use Preserve or IgnoreCycle semanitcs or None of them.
+        internal ReferenceHandlingStrategy ReferenceHandlingStrategy = ReferenceHandlingStrategy.None;
+
         internal MemberAccessor MemberAccessorStrategy
         {
             get

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs
@@ -89,7 +89,7 @@ public void Initialize(Type type, JsonSerializerOptions options, bool supportCon
 
             Current.NumberHandling = Current.JsonPropertyInfo.NumberHandling;
 
-            bool preserveReferences = JsonSerializer.IsPreserveReferencesEnabled(options);
+            bool preserveReferences = options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve;
             if (preserveReferences)
             {
                 ReferenceResolver = options.ReferenceHandler!.CreateResolver(writing: false);

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs
@@ -11,9 +11,10 @@ namespace System.Text.Json.Serialization
     public abstract class ReferenceHandler
     {
         /// <summary>
-        /// Whether we read and write preserve references or we ignore references when writing.
+        /// One of the enumeration values that indicates whether this ReferenceHandler implementation should use Preserve semantics or IgnoreCycle semantics.
+        /// The defualt is Preserve.
         /// </summary>
-        internal bool UsePreserveSemantics = true;
+        internal ReferenceHandlingStrategy HandlingStrategy = ReferenceHandlingStrategy.Preserve;
 
         /// <summary>
         /// Metadata properties will be honored when deserializing JSON objects and arrays into reference types and written when serializing reference types. This is necessary to create round-trippable JSON from objects that contain cycles or duplicate references.

diff --git a/...ibraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandlingStrategy.cs b/...ibraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandlingStrategy.cs
@@ -0,0 +1,12 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+namespace System.Text.Json.Serialization
+{
+    internal enum ReferenceHandlingStrategy
+    {
+        None,
+        Preserve,
+        IgnoreCycle
+    }
+}
diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs
@@ -73,8 +73,9 @@ public JsonConverter Initialize(Type type, JsonSerializerOptions options, bool s
             Current.DeclaredJsonPropertyInfo = jsonClassInfo.PropertyInfoForClassInfo;
             Current.NumberHandling = Current.DeclaredJsonPropertyInfo.NumberHandling;
 
-            if (options.ReferenceHandler != null)
+            if (options.ReferenceHandlingStrategy != ReferenceHandlingStrategy.None)
             {
+                Debug.Assert(options.ReferenceHandler != null);
                 ReferenceResolver = options.ReferenceHandler.CreateResolver(writing: true);
             }