Skip to content

Commit

Permalink
Backport bugfixes, infrastructural changes and perf improvements from…
Browse files Browse the repository at this point in the history 
… the polymorphism feature branch (#54420)

* backport opportunistic bugfixes

* strengthen debug assertions and add clarifying comments

* Rework WriteStack/ReadStack implementations

* Use array instead of list for storing frames. Previous frames can now be passed by reference
* Ensure that the _count field always reflects the current depth.
* Remove StackFrame.Reset() methods which are occassionally a source of dirty frames being reused.

* Take advantage of JIT optimizations in the serialization hot path

* address PR feedback
  • Loading branch information
@eiriktsarpalis
eiriktsarpalis authored Jun 18, 2021
1 parent f722ad0 commit 6737dce
Show file tree
Hide file tree
Showing 9 changed files with 235 additions and 249 deletions.
4 changes: 3 additions & 1 deletion ....Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,9 @@ internal sealed override bool OnTryWrite(

if (!jsonPropertyInfo.GetMemberAndWriteJson(objectValue!, ref state, writer))
{
Debug.Assert(jsonPropertyInfo.ConverterBase.ConverterStrategy != ConverterStrategy.Value);
Debug.Assert(jsonPropertyInfo.ConverterBase.ConverterStrategy != ConverterStrategy.Value ||
jsonPropertyInfo.ConverterBase.TypeToConvert == JsonTypeInfo.ObjectType);

return false;
}

Expand Down
144 changes: 81 additions & 63 deletions src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ protected internal JsonConverter()
// In the future, this will be check for !IsSealed (and excluding value types).
CanBePolymorphic = TypeToConvert == JsonTypeInfo.ObjectType;
IsValueType = TypeToConvert.IsValueType;
CanBeNull = !IsValueType || TypeToConvert.IsNullableOfT();
CanBeNull = default(T) is null;
IsInternalConverter = GetType().Assembly == typeof(JsonConverter).Assembly;

if (HandleNull)
Expand Down Expand Up @@ -220,7 +220,12 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali
// Remember if we were a continuation here since Push() may affect IsContinuation.
bool wasContinuation = state.IsContinuation;

#if DEBUG
// DEBUG: ensure push/pop operations preserve stack integrity
JsonTypeInfo originalJsonTypeInfo = state.Current.JsonTypeInfo;
#endif
state.Push();
Debug.Assert(TypeToConvert.IsAssignableFrom(state.Current.JsonTypeInfo.Type));

#if !DEBUG
// For performance, only perform validation on internal converters on debug builds.
Expand Down Expand Up @@ -257,6 +262,9 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali

value = default;
state.Pop(true);
#if DEBUG
Debug.Assert(ReferenceEquals(originalJsonTypeInfo, state.Current.JsonTypeInfo));
#endif
return true;
}

Expand Down Expand Up @@ -288,6 +296,9 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali
}

state.Pop(success);
#if DEBUG
Debug.Assert(ReferenceEquals(originalJsonTypeInfo, state.Current.JsonTypeInfo));
#endif
return success;
}

Expand All @@ -298,6 +309,9 @@ internal override sealed bool TryReadAsObject(ref Utf8JsonReader reader, JsonSer
return success;
}

/// <summary>
/// Overridden by the nullable converter to prevent boxing of values by the JIT.
/// </summary>
internal virtual bool IsNull(in T value) => value == null;

internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions options, ref WriteStack state)
Expand All @@ -307,7 +321,7 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions
ThrowHelper.ThrowJsonException_SerializerCycleDetected(options.EffectiveMaxDepth);
}

if (CanBeNull && !HandleNullOnWrite && IsNull(value))
if (default(T) is null && !HandleNullOnWrite && IsNull(value))
{
// 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.
Expand All @@ -317,81 +331,76 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions

bool ignoreCyclesPopReference = false;

if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycles &&
// .NET types that are serialized as JSON primitive values don't need to be tracked for cycle detection e.g: string.
ConverterStrategy != ConverterStrategy.Value &&
!IsValueType && !IsNull(value))
if (
#if NET6_0_OR_GREATER
!typeof(T).IsValueType && // treated as a constant by recent versions of the JIT.
#else
!IsValueType &&
#endif
value is not null)
{
// Custom (user) converters shall not track references
// it is responsibility of the user to break cycles in case there's any
// if we compare against Preserve, objects don't get preserved when a custom converter exists
// given that the custom converter executes prior to the preserve logic.
Debug.Assert(IsInternalConverter);
Debug.Assert(value != null);

ReferenceResolver resolver = state.ReferenceResolver;

// Write null to break reference cycles.
if (resolver.ContainsReferenceForCycleDetection(value))
{
writer.WriteNullValue();
return true;
}

// For boxed reference types: do not push when boxed in order to avoid false positives
// when we run the ContainsReferenceForCycleDetection check for the converter of the unboxed value.
Debug.Assert(!CanBePolymorphic);
resolver.PushReferenceForCycleDetection(value);
ignoreCyclesPopReference = true;
}

if (CanBePolymorphic)
{
if (value == null)
if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycles &&
// .NET types that are serialized as JSON primitive values don't need to be tracked for cycle detection e.g: string.
ConverterStrategy != ConverterStrategy.Value)
{
Debug.Assert(ConverterStrategy == ConverterStrategy.Value);
Debug.Assert(!state.IsContinuation);
Debug.Assert(HandleNullOnWrite);
// Custom (user) converters shall not track references
// it is responsibility of the user to break cycles in case there's any
// if we compare against Preserve, objects don't get preserved when a custom converter exists
// given that the custom converter executes prior to the preserve logic.
Debug.Assert(IsInternalConverter);

int originalPropertyDepth = writer.CurrentDepth;
Write(writer, value, options);
VerifyWrite(originalPropertyDepth, writer);
ReferenceResolver resolver = state.ReferenceResolver;

return true;
}
// Write null to break reference cycles.
if (resolver.ContainsReferenceForCycleDetection(value))
{
writer.WriteNullValue();
return true;
}

Type type = value.GetType();
if (type == JsonTypeInfo.ObjectType)
{
writer.WriteStartObject();
writer.WriteEndObject();
return true;
// For boxed reference types: do not push when boxed in order to avoid false positives
// when we run the ContainsReferenceForCycleDetection check for the converter of the unboxed value.
Debug.Assert(!CanBePolymorphic);
resolver.PushReferenceForCycleDetection(value);
ignoreCyclesPopReference = true;
}

if (type != TypeToConvert && IsInternalConverter)
if (CanBePolymorphic)
{
// For internal converter only: Handle polymorphic case and get the new converter.
// Custom converter, even though polymorphic converter, get called for reading AND writing.
JsonConverter jsonConverter = state.Current.InitializeReEntry(type, options);
Debug.Assert(jsonConverter != this);

if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycles &&
jsonConverter.IsValueType)
Type type = value.GetType();
if (type == JsonTypeInfo.ObjectType)
{
// For boxed value types: push the value before it gets unboxed on TryWriteAsObject.
state.ReferenceResolver.PushReferenceForCycleDetection(value);
ignoreCyclesPopReference = true;
writer.WriteStartObject();
writer.WriteEndObject();
return true;
}

// We found a different converter; forward to that.
bool success2 = jsonConverter.TryWriteAsObject(writer, value, options, ref state);

if (ignoreCyclesPopReference)
if (type != TypeToConvert && IsInternalConverter)
{
state.ReferenceResolver.PopReferenceForCycleDetection();
}
// For internal converter only: Handle polymorphic case and get the new converter.
// Custom converter, even though polymorphic converter, get called for reading AND writing.
JsonConverter jsonConverter = state.Current.InitializeReEntry(type, options);
Debug.Assert(jsonConverter != this);

if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycles &&
jsonConverter.IsValueType)
{
// For boxed value types: push the value before it gets unboxed on TryWriteAsObject.
state.ReferenceResolver.PushReferenceForCycleDetection(value);
ignoreCyclesPopReference = true;
}

// We found a different converter; forward to that.
bool success2 = jsonConverter.TryWriteAsObject(writer, value, options, ref state);

return success2;
if (ignoreCyclesPopReference)
{
state.ReferenceResolver.PopReferenceForCycleDetection();
}

return success2;
}
}
}

Expand All @@ -416,7 +425,12 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions

bool isContinuation = state.IsContinuation;

#if DEBUG
// DEBUG: ensure push/pop operations preserve stack integrity
JsonTypeInfo originalJsonTypeInfo = state.Current.JsonTypeInfo;
#endif
state.Push();
Debug.Assert(TypeToConvert.IsAssignableFrom(state.Current.JsonTypeInfo.Type));

if (!isContinuation)
{
Expand All @@ -432,6 +446,9 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions
}

state.Pop(success);
#if DEBUG
Debug.Assert(ReferenceEquals(originalJsonTypeInfo, state.Current.JsonTypeInfo));
#endif

if (ignoreCyclesPopReference)
{
Expand Down Expand Up @@ -476,6 +493,7 @@ internal bool TryWriteDataExtensionProperty(Utf8JsonWriter writer, T value, Json

// Ignore the naming policy for extension data.
state.Current.IgnoreDictionaryKeyPolicy = true;
state.Current.DeclaredJsonPropertyInfo = state.Current.JsonTypeInfo.ElementTypeInfo!.PropertyInfoForTypeInfo;

success = dictionaryConverter.OnWriteResume(writer, value, options, ref state);
if (success)
Expand Down
3 changes: 0 additions & 3 deletions ...raries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.String.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,9 +125,6 @@ private static string WriteUsingMetadata<TValue>(in TValue value, JsonTypeInfo?
throw new ArgumentNullException(nameof(jsonTypeInfo));
}

WriteStack state = default;
state.Initialize(jsonTypeInfo, supportContinuation: false);

JsonSerializerOptions options = jsonTypeInfo.Options;

using (var output = new PooledByteBufferWriter(options.DefaultBufferSize))
Expand Down
12 changes: 9 additions & 3 deletions ...aries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,11 +234,17 @@ internal override bool GetMemberAndWriteJson(object obj, ref WriteStack state, U
{
T value = Get!(obj);

if (Options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycles &&
value != null &&
if (
#if NET6_0_OR_GREATER
!typeof(T).IsValueType && // treated as a constant by recent versions of the JIT.
#else
!Converter.IsValueType &&
#endif
Options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycles &&
value is not null &&
// .NET types that are serialized as JSON primitive values don't need to be tracked for cycle detection e.g: string.
// However JsonConverter<object> that uses ConverterStrategy == Value is an exception.
(Converter.CanBePolymorphic || (!Converter.IsValueType && ConverterStrategy != ConverterStrategy.Value)) &&
(Converter.CanBePolymorphic || ConverterStrategy != ConverterStrategy.Value) &&
state.ReferenceResolver.ContainsReferenceForCycleDetection(value))
{
// If a reference cycle is detected, treat value as null.
Expand Down
Loading

0 comments on commit 6737dce

Please sign in to comment.