Skip to content
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.

Sign up for GitHub

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

Backport bugfixes, infrastructural changes and perf improvements from the polymorphism feature branch #54420

Merged
5 commits merged into from
Jun 18, 2021
Merged

Backport bugfixes, infrastructural changes and perf improvements from the polymorphism feature branch #54420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a858d3b
backport opportunistic bugfixes
eiriktsarpalis Jun 18, 2021
99529f7
strengthen debug assertions and add clarifying comments
eiriktsarpalis Jun 18, 2021
4ba2497
Rework WriteStack/ReadStack implementations
eiriktsarpalis Jun 18, 2021
cb59430
Take advantage of JIT optimizations in the serialization hot path
eiriktsarpalis Jun 18, 2021
e17ebeb
address PR feedback
eiriktsarpalis Jun 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion ....Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs
View file
Open in desktop
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
Open in desktop
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;
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
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;
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved

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
Open in desktop
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;
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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