From 170a2d9c11e0ce6e486455b050f0d5f10b84b7ae Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Tue, 22 Feb 2022 23:19:29 +0000 Subject: [PATCH 1/2] Use ConvereterStrategy.Object in ObjectConverter --- .../src/System.Text.Json.csproj | 3 +- .../Json/Serialization/ConverterStrategy.cs | 20 +- .../Collection/JsonCollectionConverter.cs | 8 +- .../Collection/JsonDictionaryConverter.cs | 8 +- .../FSharp/FSharpOptionConverter.cs | 10 +- .../FSharp/FSharpValueOptionConverter.cs | 10 +- .../{Value => Object}/ObjectConverter.cs | 37 +- .../Object/ObjectDefaultConverter.cs | 20 +- ...ctWithParameterizedConstructorConverter.cs | 2 +- .../Converters/Value/NullableConverter.cs | 10 +- .../Serialization/JsonConverter.ReadAhead.cs | 4 +- .../Text/Json/Serialization/JsonConverter.cs | 9 + .../JsonConverterOfT.ReadCore.cs | 4 +- .../JsonConverterOfT.WriteCore.cs | 8 +- .../Json/Serialization/JsonConverterOfT.cs | 362 +++++++----------- .../JsonSerializer.Read.HandleMetadata.cs | 3 +- .../JsonSerializer.Write.HandleMetadata.cs | 98 +---- .../Metadata/JsonPropertyInfoOfT.cs | 4 +- .../PolymorphicSerializationState.cs | 20 + .../Text/Json/Serialization/ReadStack.cs | 4 +- .../Text/Json/Serialization/ReadStackFrame.cs | 2 + .../Text/Json/Serialization/WriteStack.cs | 13 +- .../Json/Serialization/WriteStackFrame.cs | 67 +++- 23 files changed, 350 insertions(+), 376 deletions(-) rename src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/{Value => Object}/ObjectConverter.cs (55%) create mode 100644 src/libraries/System.Text.Json/src/System/Text/Json/Serialization/PolymorphicSerializationState.cs diff --git a/src/libraries/System.Text.Json/src/System.Text.Json.csproj b/src/libraries/System.Text.Json/src/System.Text.Json.csproj index 0373251d6fda4a..1b9fbb39d70613 100644 --- a/src/libraries/System.Text.Json/src/System.Text.Json.csproj +++ b/src/libraries/System.Text.Json/src/System.Text.Json.csproj @@ -122,6 +122,7 @@ System.Text.Json.Nodes.JsonValue + @@ -165,6 +166,7 @@ System.Text.Json.Nodes.JsonValue + @@ -189,7 +191,6 @@ System.Text.Json.Nodes.JsonValue - diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ConverterStrategy.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ConverterStrategy.cs index 7ef9b3bf9eb87b..00cd124305b919 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ConverterStrategy.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ConverterStrategy.cs @@ -12,15 +12,25 @@ namespace System.Text.Json /// internal enum ConverterStrategy : byte { - // Default - no class type. + /// + /// Default value; not used by any converter. + /// None = 0x0, - // JsonObjectConverter<> - objects with properties. + /// + /// Objects with properties. + /// Object = 0x1, - // JsonConverter<> - simple values. + /// + /// Simple values or user-provided custom converters. + /// Value = 0x2, - // JsonIEnumerableConverter<> - all enumerable collections except dictionaries. + /// + /// Enumerable collections except dictionaries. + /// Enumerable = 0x8, - // JsonDictionaryConverter<,> - dictionary types. + /// + /// Dictionary types. + /// Dictionary = 0x10, } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonCollectionConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonCollectionConverter.cs index 6d3a805cdf3749..f8052a9b2b672a 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonCollectionConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonCollectionConverter.cs @@ -180,7 +180,7 @@ internal override bool OnTryRead( { state.Current.PropertyState = StackFramePropertyState.ReadValue; - if (!SingleValueReadWithReadAhead(elementConverter.ConverterStrategy, ref reader, ref state)) + if (!SingleValueReadWithReadAhead(elementConverter.RequiresReadAhead, ref reader, ref state)) { value = default; return false; @@ -270,11 +270,7 @@ internal override bool OnTryWrite( if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve) { MetadataPropertyName metadata = JsonSerializer.WriteReferenceForCollection(this, value, ref state, writer); - if (metadata == MetadataPropertyName.Ref) - { - return true; - } - + Debug.Assert(metadata != MetadataPropertyName.Ref); state.Current.MetadataPropertyName = metadata; } else diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonDictionaryConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonDictionaryConverter.cs index b086d954ca3b54..dd3cacec79f659 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonDictionaryConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonDictionaryConverter.cs @@ -239,7 +239,7 @@ internal sealed override bool OnTryRead( { state.Current.PropertyState = StackFramePropertyState.ReadValue; - if (!SingleValueReadWithReadAhead(_valueConverter.ConverterStrategy, ref reader, ref state)) + if (!SingleValueReadWithReadAhead(_valueConverter.RequiresReadAhead, ref reader, ref state)) { state.Current.DictionaryKey = key; value = default; @@ -311,10 +311,8 @@ internal sealed override bool OnTryWrite( writer.WriteStartObject(); if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve) { - if (JsonSerializer.WriteReferenceForObject(this, dictionary, ref state, writer) == MetadataPropertyName.Ref) - { - return true; - } + MetadataPropertyName propertyName = JsonSerializer.WriteReferenceForObject(this, dictionary, ref state, writer); + Debug.Assert(propertyName != MetadataPropertyName.Ref); } state.Current.JsonPropertyInfo = state.Current.JsonTypeInfo.ElementTypeInfo!.PropertyInfoForTypeInfo; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/FSharp/FSharpOptionConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/FSharp/FSharpOptionConverter.cs index e8d78a55700c2b..9614e8492a52dc 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/FSharp/FSharpOptionConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/FSharp/FSharpOptionConverter.cs @@ -29,11 +29,11 @@ public FSharpOptionConverter(JsonConverter elementConverter) _optionValueGetter = FSharpCoreReflectionProxy.Instance.CreateFSharpOptionValueGetter(); _optionConstructor = FSharpCoreReflectionProxy.Instance.CreateFSharpOptionSomeConstructor(); - // temporary workaround for JsonConverter base constructor needing to access - // ConverterStrategy when calculating `CanUseDirectReadOrWrite`. - // TODO move `CanUseDirectReadOrWrite` from JsonConverter to JsonTypeInfo. - _converterStrategy = _elementConverter.ConverterStrategy; - CanUseDirectReadOrWrite = _converterStrategy == ConverterStrategy.Value; + // Workaround for the base constructor depending on the (still unset) ConverterStrategy + // to derive the CanUseDirectReadOrWrite and RequiresReadAhead values. + _converterStrategy = elementConverter.ConverterStrategy; + CanUseDirectReadOrWrite = elementConverter.CanUseDirectReadOrWrite; + RequiresReadAhead = elementConverter.RequiresReadAhead; } internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, out TOption? value) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/FSharp/FSharpValueOptionConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/FSharp/FSharpValueOptionConverter.cs index b11c346dc8df14..ccafcdf180e1b6 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/FSharp/FSharpValueOptionConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/FSharp/FSharpValueOptionConverter.cs @@ -29,11 +29,11 @@ public FSharpValueOptionConverter(JsonConverter elementConverter) _optionValueGetter = FSharpCoreReflectionProxy.Instance.CreateFSharpValueOptionValueGetter(); _optionConstructor = FSharpCoreReflectionProxy.Instance.CreateFSharpValueOptionSomeConstructor(); - // temporary workaround for JsonConverter base constructor needing to access - // ConverterStrategy when calculating `CanUseDirectReadOrWrite`. - // TODO move `CanUseDirectReadOrWrite` from JsonConverter to JsonTypeInfo. - _converterStrategy = _elementConverter.ConverterStrategy; - CanUseDirectReadOrWrite = _converterStrategy == ConverterStrategy.Value; + // Workaround for the base constructor depending on the (still unset) ConverterStrategy + // to derive the CanUseDirectReadOrWrite and RequiresReadAhead values. + _converterStrategy = elementConverter.ConverterStrategy; + CanUseDirectReadOrWrite = elementConverter.CanUseDirectReadOrWrite; + RequiresReadAhead = elementConverter.RequiresReadAhead; } internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, out TValueOption value) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/ObjectConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectConverter.cs similarity index 55% rename from src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/ObjectConverter.cs rename to src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectConverter.cs index 7ce4a2cb706b55..90d413dffd2e71 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/ObjectConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectConverter.cs @@ -7,6 +7,15 @@ namespace System.Text.Json.Serialization.Converters { internal sealed class ObjectConverter : JsonConverter { + internal override ConverterStrategy ConverterStrategy => ConverterStrategy.Object; + + public ObjectConverter() + { + CanBePolymorphic = true; + // JsonElement/JsonNode parsing does not support async; force read ahead for now. + RequiresReadAhead = true; + } + public override object? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { if (options.UnknownTypeHandling == JsonUnknownTypeHandling.JsonElement) @@ -14,17 +23,43 @@ internal sealed class ObjectConverter : JsonConverter return JsonElement.ParseValue(ref reader); } + Debug.Assert(options.UnknownTypeHandling == JsonUnknownTypeHandling.JsonNode); return JsonNodeConverter.Instance.Read(ref reader, typeToConvert, options); } public override void Write(Utf8JsonWriter writer, object? value, JsonSerializerOptions options) { Debug.Assert(value?.GetType() == typeof(object)); - writer.WriteStartObject(); writer.WriteEndObject(); } + internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, out object? value) + { + if (options.UnknownTypeHandling == JsonUnknownTypeHandling.JsonElement) + { + JsonElement element = JsonElement.ParseValue(ref reader); + + // Edge case where we want to lookup for a reference when parsing into typeof(object) + if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve && + JsonSerializer.TryGetReferenceFromJsonElement(ref state, element, out object? referenceValue)) + { + value = referenceValue; + } + else + { + value = element; + } + + return true; + } + + Debug.Assert(options.UnknownTypeHandling == JsonUnknownTypeHandling.JsonNode); + value = JsonNodeConverter.Instance.Read(ref reader, typeToConvert, options)!; + // TODO reference lookup for JsonNode deserialization. + return true; + } + internal override object ReadAsPropertyNameCore(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { ThrowHelper.ThrowNotSupportedException_DictionaryKeyTypeNotSupported(TypeToConvert, this); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs index 9e543b48f60514..032b6209826c8f 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs @@ -261,10 +261,8 @@ internal sealed override bool OnTryWrite( writer.WriteStartObject(); if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve) { - if (JsonSerializer.WriteReferenceForObject(this, obj, ref state, writer) == MetadataPropertyName.Ref) - { - return true; - } + MetadataPropertyName propertyName = JsonSerializer.WriteReferenceForObject(this, obj, ref state, writer); + Debug.Assert(propertyName != MetadataPropertyName.Ref); } if (obj is IJsonOnSerializing onSerializing) @@ -313,10 +311,8 @@ internal sealed override bool OnTryWrite( writer.WriteStartObject(); if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve) { - if (JsonSerializer.WriteReferenceForObject(this, obj, ref state, writer) == MetadataPropertyName.Ref) - { - return true; - } + MetadataPropertyName propertyName = JsonSerializer.WriteReferenceForObject(this, obj, ref state, writer); + Debug.Assert(propertyName != MetadataPropertyName.Ref); } if (obj is IJsonOnSerializing onSerializing) @@ -339,9 +335,7 @@ internal sealed override bool OnTryWrite( if (!jsonPropertyInfo.GetMemberAndWriteJson(obj!, ref state, writer)) { - Debug.Assert(jsonPropertyInfo.ConverterBase.ConverterStrategy != ConverterStrategy.Value || - jsonPropertyInfo.ConverterBase.TypeToConvert == JsonTypeInfo.ObjectType); - + Debug.Assert(jsonPropertyInfo.ConverterBase.ConverterStrategy != ConverterStrategy.Value); return false; } @@ -443,7 +437,7 @@ protected static bool ReadAheadPropertyValue(ref ReadStack state, ref Utf8JsonRe if (!state.Current.UseExtensionProperty) { - if (!SingleValueReadWithReadAhead(jsonPropertyInfo.ConverterBase.ConverterStrategy, ref reader, ref state)) + if (!SingleValueReadWithReadAhead(jsonPropertyInfo.ConverterBase.RequiresReadAhead, ref reader, ref state)) { return false; } @@ -451,7 +445,7 @@ protected static bool ReadAheadPropertyValue(ref ReadStack state, ref Utf8JsonRe else { // The actual converter is JsonElement, so force a read-ahead. - if (!SingleValueReadWithReadAhead(ConverterStrategy.Value, ref reader, ref state)) + if (!SingleValueReadWithReadAhead(requiresReadAhead: true, ref reader, ref state)) { return false; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs index b6b12f199c23ae..6e08550d1b31f2 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs @@ -364,7 +364,7 @@ private bool HandleConstructorArgumentWithContinuation( // Returning false below will cause the read-ahead functionality to finish the read. state.Current.PropertyState = StackFramePropertyState.ReadValue; - if (!SingleValueReadWithReadAhead(jsonParameterInfo.ConverterBase.ConverterStrategy, ref reader, ref state)) + if (!SingleValueReadWithReadAhead(jsonParameterInfo.ConverterBase.RequiresReadAhead, ref reader, ref state)) { return false; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverter.cs index 4f671c53bff282..292d70e42b646f 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverter.cs @@ -16,11 +16,13 @@ internal sealed class NullableConverter : JsonConverter where T : struct public NullableConverter(JsonConverter elementConverter) { _elementConverter = elementConverter; - ConverterStrategy = elementConverter.ConverterStrategy; IsInternalConverterForNumberType = elementConverter.IsInternalConverterForNumberType; - // temporary workaround for JsonConverter base constructor needing to access - // ConverterStrategy when calculating `CanUseDirectReadOrWrite`. - CanUseDirectReadOrWrite = elementConverter.ConverterStrategy == ConverterStrategy.Value; + + // Workaround for the base constructor depending on the (still unset) ConverterStrategy + // to derive the CanUseDirectReadOrWrite and RequiresReadAhead values. + ConverterStrategy = elementConverter.ConverterStrategy; + CanUseDirectReadOrWrite = elementConverter.CanUseDirectReadOrWrite; + RequiresReadAhead = elementConverter.RequiresReadAhead; } internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, out T? value) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.ReadAhead.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.ReadAhead.cs index fa75dfe851fbcf..813dad7d423bbd 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.ReadAhead.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.ReadAhead.cs @@ -17,9 +17,9 @@ public abstract partial class JsonConverter // AggressiveInlining used since this method is on a hot path and short. The optionally called // method DoSingleValueReadWithReadAhead is not inlined. [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal static bool SingleValueReadWithReadAhead(ConverterStrategy converterStrategy, ref Utf8JsonReader reader, ref ReadStack state) + internal static bool SingleValueReadWithReadAhead(bool requiresReadAhead, ref Utf8JsonReader reader, ref ReadStack state) { - bool readAhead = state.ReadAhead && converterStrategy == ConverterStrategy.Value; + bool readAhead = requiresReadAhead && state.ReadAhead; if (!readAhead) { return reader.Read(); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs index 6a1ef235dc3b30..3a48b99021724e 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs @@ -33,8 +33,17 @@ internal JsonConverter() { } /// internal virtual bool CanHaveIdMetadata => false; + /// + /// The converter supports polymorphic writes; only reserved for System.Object types. + /// internal bool CanBePolymorphic { get; set; } + /// + /// The serializer must read ahead all contents of the next JSON value + /// before calling into the converter for deserialization. + /// + internal bool RequiresReadAhead { get; set; } + /// /// Used to support JsonObject as an extension property in a loosely-typed, trimmable manner. /// diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.ReadCore.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.ReadCore.cs index 3d51b2b415f822..9c91ac3d7fced4 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.ReadCore.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.ReadCore.cs @@ -24,7 +24,7 @@ public partial class JsonConverter { if (!state.IsContinuation) { - if (!SingleValueReadWithReadAhead(ConverterStrategy, ref reader, ref state)) + if (!SingleValueReadWithReadAhead(RequiresReadAhead, ref reader, ref state)) { if (state.SupportContinuation) { @@ -51,7 +51,7 @@ public partial class JsonConverter { // For a continuation, read ahead here to avoid having to build and then tear // down the call stack if there is more than one buffer fetch necessary. - if (!SingleValueReadWithReadAhead(ConverterStrategy.Value, ref reader, ref state)) + if (!SingleValueReadWithReadAhead(requiresReadAhead: true, ref reader, ref state)) { state.BytesConsumed += reader.BytesConsumed; return default; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.WriteCore.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.WriteCore.cs index da003ec3f09459..c1449f26ec28d9 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.WriteCore.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.WriteCore.cs @@ -11,7 +11,13 @@ internal sealed override bool WriteCoreAsObject( JsonSerializerOptions options, ref WriteStack state) { - if (IsValueType) + if ( +#if NET5_0_OR_GREATER + // Short-circuit the check against "is not null"; treated as a constant by recent versions of the JIT. + typeof(T).IsValueType) +#else + IsValueType) +#endif { // Value types can never have a null except for Nullable. if (value == null && Nullable.GetUnderlyingType(TypeToConvert) == null) 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 index 7b47f436b16714..eb9cced2bb364a 100644 --- 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 @@ -2,8 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; -using System.Diagnostics.CodeAnalysis; -using System.Runtime.CompilerServices; using System.Text.Json.Serialization.Converters; using System.Text.Json.Serialization.Metadata; @@ -20,25 +18,25 @@ public abstract partial class JsonConverter : JsonConverter /// protected internal JsonConverter() { + IsValueType = typeof(T).IsValueType; IsInternalConverter = GetType().Assembly == typeof(JsonConverter).Assembly; - // Today only the internal JsonConverter can have polymorphic writes. - CanBePolymorphic = IsInternalConverter && TypeToConvert == JsonTypeInfo.ObjectType; - IsValueType = TypeToConvert.IsValueType; - CanBeNull = default(T) is null; if (HandleNull) { HandleNullOnRead = true; HandleNullOnWrite = true; } + else + { + // For the HandleNull == false case, either: + // 1) The default values are assigned in this type's virtual HandleNull property + // or + // 2) A converter overrode HandleNull and returned false so HandleNullOnRead and HandleNullOnWrite + // will be their default values of false. + } - // For the HandleNull == false case, either: - // 1) The default values are assigned in this type's virtual HandleNull property - // or - // 2) A converter overroad HandleNull and returned false so HandleNullOnRead and HandleNullOnWrite - // will be their default values of false. - - CanUseDirectReadOrWrite = !CanBePolymorphic && IsInternalConverter && ConverterStrategy == ConverterStrategy.Value; + CanUseDirectReadOrWrite = ConverterStrategy == ConverterStrategy.Value && IsInternalConverter; + RequiresReadAhead = ConverterStrategy == ConverterStrategy.Value; } /// @@ -87,7 +85,7 @@ public virtual bool HandleNull // If the type doesn't support null, allow the converter a chance to modify. // These semantics are backwards compatible with 3.0. - HandleNullOnRead = !CanBeNull; + HandleNullOnRead = default(T) is not null; // The framework handles null automatically on writes. HandleNullOnWrite = false; @@ -106,11 +104,6 @@ public virtual bool HandleNull /// internal bool HandleNullOnWrite { get; private set; } - /// - /// Can be assigned to ? - /// - internal bool CanBeNull { get; } - // This non-generic API is sealed as it just forwards to the generic version. internal sealed override bool TryWriteAsObject(Utf8JsonWriter writer, object? value, JsonSerializerOptions options, ref WriteStack state) { @@ -147,23 +140,22 @@ internal virtual bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, J internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, out T? value) { - if (ConverterStrategy == ConverterStrategy.Value) + // For perf and converter simplicity, handle null here instead of forwarding to the converter. + if (reader.TokenType == JsonTokenType.Null && !HandleNullOnRead && !state.IsContinuation) { - // A value converter should never be within a continuation. - Debug.Assert(!state.IsContinuation); - - // For perf and converter simplicity, handle null here instead of forwarding to the converter. - if (reader.TokenType == JsonTokenType.Null && !HandleNullOnRead) + if (default(T) is not null) { - if (!CanBeNull) - { - ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(TypeToConvert); - } - - value = default; - return true; + ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(TypeToConvert); } + value = default; + return true; + } + + if (ConverterStrategy == ConverterStrategy.Value) + { + // A value converter should never be within a continuation. + Debug.Assert(!state.IsContinuation); #if !DEBUG // For performance, only perform validation on internal converters on debug builds. if (IsInternalConverter) @@ -201,27 +193,13 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali ref reader); } - 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. - Debug.Assert(TypeToConvert == typeof(object)); - - if (JsonSerializer.TryGetReferenceFromJsonElement(ref state, element, out object? referenceValue)) - { - value = (T?)referenceValue; - } - } - return true; } + Debug.Assert(IsInternalConverter); + bool isContinuation = state.IsContinuation; bool success; - // 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; @@ -229,73 +207,37 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali state.Push(); Debug.Assert(TypeToConvert == state.Current.JsonTypeInfo.Type); -#if !DEBUG +#if DEBUG // For performance, only perform validation on internal converters on debug builds. - if (IsInternalConverter) + if (!isContinuation) { - if (reader.TokenType == JsonTokenType.Null && !HandleNullOnRead && !wasContinuation) - { - if (!CanBeNull) - { - ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(TypeToConvert); - } + Debug.Assert(state.Current.OriginalTokenType == JsonTokenType.None); + state.Current.OriginalTokenType = reader.TokenType; - // For perf and converter simplicity, handle null here instead of forwarding to the converter. - value = default; - success = true; - } - else - { - success = OnTryRead(ref reader, typeToConvert, options, ref state, out value); - } + Debug.Assert(state.Current.OriginalDepth == 0); + state.Current.OriginalDepth = reader.CurrentDepth; } - else #endif + success = OnTryRead(ref reader, typeToConvert, options, ref state, out value); +#if DEBUG + if (success) { - if (!wasContinuation) + if (state.IsContinuation) { - // For perf and converter simplicity, handle null here instead of forwarding to the converter. - if (reader.TokenType == JsonTokenType.Null && !HandleNullOnRead) - { - if (!CanBeNull) - { - ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(TypeToConvert); - } - - value = default; - state.Pop(true); -#if DEBUG - Debug.Assert(ReferenceEquals(originalJsonTypeInfo, state.Current.JsonTypeInfo)); -#endif - return true; - } - - Debug.Assert(state.Current.OriginalTokenType == JsonTokenType.None); - state.Current.OriginalTokenType = reader.TokenType; - - Debug.Assert(state.Current.OriginalDepth == 0); - state.Current.OriginalDepth = reader.CurrentDepth; + // The resumable converter did not forward to the next converter that previously returned false. + ThrowHelper.ThrowJsonException_SerializationConverterRead(this); } - success = OnTryRead(ref reader, typeToConvert, options, ref state, out value); - if (success) - { - if (state.IsContinuation) - { - // The resumable converter did not forward to the next converter that previously returned false. - ThrowHelper.ThrowJsonException_SerializationConverterRead(this); - } + VerifyRead( + state.Current.OriginalTokenType, + state.Current.OriginalDepth, + bytesConsumed: 0, + isValueConverter: false, + ref reader); - VerifyRead( - state.Current.OriginalTokenType, - state.Current.OriginalDepth, - bytesConsumed: 0, - isValueConverter: false, - ref reader); - - // No need to clear state.Current.* since a stack pop will occur. - } + // No need to clear state.Current.* since a stack pop will occur. } +#endif state.Pop(success); #if DEBUG @@ -314,7 +256,7 @@ internal override sealed bool TryReadAsObject(ref Utf8JsonReader reader, JsonSer /// /// Performance optimization. /// The 'in' modifier in 'TryWrite(in T Value)' causes boxing for Nullable{T}, so this helper avoids that. - /// TODO: Remove this work-around once #50915 is addressed. + /// TODO: Remove this work-around once https://github.com/dotnet/runtime/issues/50915 is addressed. /// private static bool IsNull(T value) => value is null; @@ -333,7 +275,28 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions return true; } - bool ignoreCyclesPopReference = false; + if (ConverterStrategy == ConverterStrategy.Value) + { + Debug.Assert(!state.IsContinuation); + + int originalPropertyDepth = writer.CurrentDepth; + + if (state.Current.NumberHandling != null && IsInternalConverterForNumberType) + { + WriteNumberWithCustomHandling(writer, value, state.Current.NumberHandling.Value); + } + else + { + Write(writer, value, options); + } + + VerifyWrite(originalPropertyDepth, writer); + return true; + } + + Debug.Assert(IsInternalConverter); + bool isContinuation = state.IsContinuation; + bool success; if ( #if NET5_0_OR_GREATER @@ -342,121 +305,73 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions #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) + value is not null && + // Do not handle objects that have already been + // handled by a polymorphic converter for a base type. + state.Current.PolymorphicSerializationState != PolymorphicSerializationState.PolymorphicReEntryStarted) { + JsonConverter? polymorphicConverter = CanBePolymorphic ? + state.Current.ResolvePolymorphicConverter(value, TypeToConvert, options) : + null; + + Debug.Assert(polymorphicConverter is null || state.CurrentDepth > 0, + "root-level polymorphic converters should not be handled here."); - 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) + if (!isContinuation) { - // 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); + switch (options.ReferenceHandlingStrategy) + { + case ReferenceHandlingStrategy.IgnoreCycles: + ReferenceResolver resolver = state.ReferenceResolver; + if (resolver.ContainsReferenceForCycleDetection(value)) + { + writer.WriteNullValue(); + return true; + } - ReferenceResolver resolver = state.ReferenceResolver; + resolver.PushReferenceForCycleDetection(value); + // WriteStack reuses root-level stackframes for its children as a performance optimization; + // we want to avoid writing any data for the root-level object to avoid corrupting the stack. + // This is fine since popping the root object at the end of serialization is not essential. + state.Current.IsPushedReferenceForCycleDetection = state.CurrentDepth > 0; + break; - // Write null to break reference cycles. - if (resolver.ContainsReferenceForCycleDetection(value)) - { - writer.WriteNullValue(); - return true; - } + case ReferenceHandlingStrategy.Preserve: + bool canHaveIdMetata = polymorphicConverter?.CanHaveIdMetadata ?? CanHaveIdMetadata; + if (canHaveIdMetata && JsonSerializer.TryGetReferenceForValue(value, ref state, writer)) + { + // We found a repeating reference and wrote the relevant metadata; serialization complete. + return true; + } + break; - // 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; + default: + Debug.Assert(options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.None); + break; + } } - if (CanBePolymorphic) + if (polymorphicConverter is not null) { - Debug.Assert(IsInternalConverter); + Debug.Assert(!polymorphicConverter.CanBePolymorphic, "Only ObjectConverter supports polymorphism."); - Type type = value.GetType(); + state.Current.EnterPolymorphicConverter(); + success = polymorphicConverter.TryWriteAsObject(writer, value, options, ref state); + state.Current.ExitPolymorphicConverter(success); - if (type != TypeToConvert) + if (success) { - // 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); - - // For boxed value types: invoke the reference handler - // before the instance gets unboxed by the subtype converter. - if (jsonConverter.IsValueType) - { - switch (options.ReferenceHandlingStrategy) - { - case ReferenceHandlingStrategy.Preserve when (jsonConverter.CanHaveIdMetadata && !state.IsContinuation): - if (JsonSerializer.TryWriteReferenceForBoxedStruct(value, ref state, writer)) - { - return true; - } - break; - - case ReferenceHandlingStrategy.IgnoreCycles: - state.ReferenceResolver.PushReferenceForCycleDetection(value); - ignoreCyclesPopReference = true; - break; - default: - break; - } - } - - // We found a different converter; forward to that. - bool success2 = jsonConverter.TryWriteAsObject(writer, value, options, ref state); - - if (ignoreCyclesPopReference) + if (state.Current.IsPushedReferenceForCycleDetection) { state.ReferenceResolver.PopReferenceForCycleDetection(); + state.Current.IsPushedReferenceForCycleDetection = false; } - - return success2; } - } - } - - if (ConverterStrategy == ConverterStrategy.Value) - { - Debug.Assert(!state.IsContinuation); - int originalPropertyDepth = writer.CurrentDepth; - - if (state.Current.NumberHandling != null && IsInternalConverterForNumberType) - { - WriteNumberWithCustomHandling(writer, value, state.Current.NumberHandling.Value); - } - else - { - Write(writer, value, options); + return success; } - - VerifyWrite(originalPropertyDepth, writer); - - if ( -#if NET5_0_OR_GREATER - // 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 - // of type object using the internal object converter. - Debug.Assert(value?.GetType() == typeof(object) && IsInternalConverter); - state.ReferenceResolver.PopReferenceForCycleDetection(); - } - - return true; } - bool isContinuation = state.IsContinuation; - #if DEBUG // DEBUG: ensure push/pop operations preserve stack integrity JsonTypeInfo originalJsonTypeInfo = state.Current.JsonTypeInfo; @@ -464,29 +379,34 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions state.Push(); Debug.Assert(TypeToConvert == state.Current.JsonTypeInfo.Type); +#if DEBUG + // For performance, only perform validation on internal converters on debug builds. if (!isContinuation) { Debug.Assert(state.Current.OriginalDepth == 0); state.Current.OriginalDepth = writer.CurrentDepth; } - - bool success = OnTryWrite(writer, value, options, ref state); +#endif + success = OnTryWrite(writer, value, options, ref state); +#if DEBUG if (success) { VerifyWrite(state.Current.OriginalDepth, writer); - // No need to clear state.Current.OriginalDepth since a stack pop will occur. } - - state.Pop(success); -#if DEBUG - Debug.Assert(ReferenceEquals(originalJsonTypeInfo, state.Current.JsonTypeInfo)); #endif + state.Pop(success); - if (ignoreCyclesPopReference) + if (success) { - state.ReferenceResolver.PopReferenceForCycleDetection(); + if (state.Current.IsPushedReferenceForCycleDetection) + { + state.ReferenceResolver.PopReferenceForCycleDetection(); + state.Current.IsPushedReferenceForCycleDetection = false; + } } - +#if DEBUG + Debug.Assert(ReferenceEquals(originalJsonTypeInfo, state.Current.JsonTypeInfo)); +#endif return success; } @@ -545,6 +465,8 @@ internal bool TryWriteDataExtensionProperty(Utf8JsonWriter writer, T value, Json internal void VerifyRead(JsonTokenType tokenType, int depth, long bytesConsumed, bool isValueConverter, ref Utf8JsonReader reader) { + Debug.Assert(isValueConverter == (ConverterStrategy == ConverterStrategy.Value)); + switch (tokenType) { case JsonTokenType.StartArray: @@ -572,24 +494,26 @@ internal void VerifyRead(JsonTokenType tokenType, int depth, long bytesConsumed, break; default: - // A non-value converter (object or collection) should always have Start and End tokens - if (!isValueConverter) + if (isValueConverter) { - // with the exception of converters that support null value reads - if (!HandleNullOnRead || tokenType != JsonTokenType.Null) + // A value converter should not make any reads. + if (reader.BytesConsumed != bytesConsumed) { ThrowHelper.ThrowJsonException_SerializationConverterRead(this); } } - // A value converter should not make any reads. - else if (reader.BytesConsumed != bytesConsumed) + else { - ThrowHelper.ThrowJsonException_SerializationConverterRead(this); + // A non-value converter (object or collection) should always have Start and End tokens + // unless it is polymorphic or supports null value reads. + if (!CanBePolymorphic && !(HandleNullOnRead && tokenType == JsonTokenType.Null)) + { + ThrowHelper.ThrowJsonException_SerializationConverterRead(this); + } } // Should not be possible to change token type. Debug.Assert(reader.TokenType == tokenType); - break; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleMetadata.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleMetadata.cs index b7a5dffe77e7c5..b34efae2572d31 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleMetadata.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleMetadata.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Runtime.CompilerServices; using System.Text.Json.Serialization; @@ -398,7 +399,7 @@ internal static MetadataPropertyName GetMetadataPropertyName(ReadOnlySpan internal static bool TryGetReferenceFromJsonElement( ref ReadStack state, JsonElement element, - out object? referenceValue) + [NotNullWhen(true)] out object? referenceValue) { bool refMetadataFound = false; referenceValue = default; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleMetadata.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleMetadata.cs index e792bcf5eb1242..143060080ac90e 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleMetadata.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleMetadata.cs @@ -19,42 +19,15 @@ internal static MetadataPropertyName WriteReferenceForObject( ref WriteStack state, Utf8JsonWriter writer) { - MetadataPropertyName writtenMetadataName; - - if (state.BoxedStructReferenceId != null) + if (state.NewReferenceId != null) { - // We're serializing a struct that has been handled by a polymorphic converter; - // emit the reference id that was recorded for the boxed instance. - - Debug.Assert(jsonConverter.IsValueType && jsonConverter.CanHaveIdMetadata); - writer.WriteString(s_metadataId, state.BoxedStructReferenceId); - writtenMetadataName = MetadataPropertyName.Id; - state.BoxedStructReferenceId = null; - } - else if (!jsonConverter.CanHaveIdMetadata || jsonConverter.IsValueType) - { - // If the jsonConverter supports immutable dictionaries or value types, don't write any metadata - writtenMetadataName = MetadataPropertyName.NoMetadata; - } - else - { - string referenceId = state.ReferenceResolver.GetReference(currentValue, out bool alreadyExists); - Debug.Assert(referenceId != null); - - if (alreadyExists) - { - writer.WriteString(s_metadataRef, referenceId); - writer.WriteEndObject(); - writtenMetadataName = MetadataPropertyName.Ref; - } - else - { - writer.WriteString(s_metadataId, referenceId); - writtenMetadataName = MetadataPropertyName.Id; - } + Debug.Assert(jsonConverter.CanHaveIdMetadata); + writer.WriteString(s_metadataId, state.NewReferenceId); + state.NewReferenceId = null; + return MetadataPropertyName.Id; } - return writtenMetadataName; + return MetadataPropertyName.NoMetadata; } internal static MetadataPropertyName WriteReferenceForCollection( @@ -63,73 +36,42 @@ internal static MetadataPropertyName WriteReferenceForCollection( ref WriteStack state, Utf8JsonWriter writer) { - MetadataPropertyName writtenMetadataName; - - if (state.BoxedStructReferenceId != null) + if (state.NewReferenceId != null) { - // We're serializing a struct that has been handled by a polymorphic converter; - // emit the reference id that was recorded for the boxed instance. - - Debug.Assert(jsonConverter.IsValueType && jsonConverter.CanHaveIdMetadata); - + Debug.Assert(jsonConverter.CanHaveIdMetadata); writer.WriteStartObject(); - writer.WriteString(s_metadataId, state.BoxedStructReferenceId); + writer.WriteString(s_metadataId, state.NewReferenceId); writer.WriteStartArray(s_metadataValues); - writtenMetadataName = MetadataPropertyName.Id; - state.BoxedStructReferenceId = null; - } - else if (!jsonConverter.CanHaveIdMetadata || jsonConverter.IsValueType) - { - // If the jsonConverter supports immutable enumerables or value type collections, don't write any metadata - writer.WriteStartArray(); - writtenMetadataName = MetadataPropertyName.NoMetadata; - } - else - { - string referenceId = state.ReferenceResolver.GetReference(currentValue, out bool alreadyExists); - Debug.Assert(referenceId != null); - - if (alreadyExists) - { - writer.WriteStartObject(); - writer.WriteString(s_metadataRef, referenceId); - writer.WriteEndObject(); - writtenMetadataName = MetadataPropertyName.Ref; - } - else - { - writer.WriteStartObject(); - writer.WriteString(s_metadataId, referenceId); - writer.WriteStartArray(s_metadataValues); - writtenMetadataName = MetadataPropertyName.Id; - } + state.NewReferenceId = null; + return MetadataPropertyName.Id; } - return writtenMetadataName; + // If the jsonConverter supports immutable enumerables or value type collections, don't write any metadata + writer.WriteStartArray(); + return MetadataPropertyName.NoMetadata; } /// - /// Used by polymorphic converters that are handling references for values that are boxed structs. + /// Compute reference id for the next value to be serialized. /// - internal static bool TryWriteReferenceForBoxedStruct(object currentValue, ref WriteStack state, Utf8JsonWriter writer) + internal static bool TryGetReferenceForValue(object currentValue, ref WriteStack state, Utf8JsonWriter writer) { - Debug.Assert(state.BoxedStructReferenceId == null); - Debug.Assert(currentValue.GetType().IsValueType); + Debug.Assert(state.NewReferenceId == null); string referenceId = state.ReferenceResolver.GetReference(currentValue, out bool alreadyExists); Debug.Assert(referenceId != null); if (alreadyExists) { + // Instance already serialized, write as { "$ref" : "referenceId" } writer.WriteStartObject(); writer.WriteString(s_metadataRef, referenceId); writer.WriteEndObject(); } else { - // Since we cannot run `ReferenceResolver.GetReference` twice for newly encountered instances, - // need to store the reference id for use by the subtype converter we're dispatching to. - state.BoxedStructReferenceId = referenceId; + // New instance, store computed reference id in the state + state.NewReferenceId = referenceId; } return alreadyExists; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs index 1b28d169285737..e28bdee6d691e2 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs @@ -250,9 +250,9 @@ internal override bool GetMemberAndWriteJson(object obj, ref WriteStack state, U #endif Options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycles && value is not null && + !state.IsContinuation && // .NET types that are serialized as JSON primitive values don't need to be tracked for cycle detection e.g: string. - // However JsonConverter that uses ConverterStrategy == Value is an exception. - (Converter.CanBePolymorphic || ConverterStrategy != ConverterStrategy.Value) && + ConverterStrategy != ConverterStrategy.Value && state.ReferenceResolver.ContainsReferenceForCycleDetection(value)) { // If a reference cycle is detected, treat value as null. diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/PolymorphicSerializationState.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/PolymorphicSerializationState.cs new file mode 100644 index 00000000000000..735c10e22af990 --- /dev/null +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/PolymorphicSerializationState.cs @@ -0,0 +1,20 @@ +// 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 +{ + internal enum PolymorphicSerializationState : byte + { + None, + + /// + /// Dispatch to a polymorphic converter has been initiated. + /// + PolymorphicReEntryStarted, + + /// + /// Current frame is a continuation using a suspended polymorphic converter. + /// + PolymorphicReEntrySuspended + } +} 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 index 0db9ef4650c02c..db984ab726d3dc 100644 --- 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 @@ -113,7 +113,9 @@ public void Push() { if (_count == 0) { - // The first stack frame is held in Current. + // Performance optimization: reuse the first stackframe on the first push operation. + // NB need to be careful when making writes to Current _before_ the first `Push` + // operation is performed. _count = 1; } else diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs index 137305f1d5b431..616880392f508b 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs @@ -25,9 +25,11 @@ internal struct ReadStackFrame // Stores the non-string dictionary keys for continuation. public object? DictionaryKey; +#if DEBUG // Validation state. public int OriginalDepth; public JsonTokenType OriginalTokenType; +#endif // Current object (POCO or IEnumerable). public object? ReturnValue; // The current return value used for re-entry. 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 index f966a1a77cb7a0..eb52fd569833d5 100644 --- 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 @@ -15,6 +15,8 @@ namespace System.Text.Json [DebuggerDisplay("Path:{PropertyPath()} Current: ConverterStrategy.{ConverterStrategy.JsonTypeInfo.PropertyInfoForTypeInfo.ConverterStrategy}, {Current.JsonTypeInfo.Type.Name}")] internal struct WriteStack { + public int CurrentDepth => _count; + /// /// Exposes the stackframe that is currently active. /// @@ -77,9 +79,9 @@ internal struct WriteStack public bool SupportContinuation; /// - /// Stores a reference id that has been calculated by a polymorphic converter handling a newly encountered boxed struct. + /// Stores a reference id that has been calculated for a newly serialized object. /// - public string? BoxedStructReferenceId; + public string? NewReferenceId; private void EnsurePushCapacity() { @@ -99,7 +101,6 @@ private void EnsurePushCapacity() public JsonConverter Initialize(Type type, JsonSerializerOptions options, bool supportContinuation) { JsonTypeInfo jsonTypeInfo = options.GetOrAddJsonTypeInfoForRootType(type); - Debug.Assert(options == jsonTypeInfo.Options); return Initialize(jsonTypeInfo, supportContinuation); } @@ -127,12 +128,14 @@ public void Push() { if (_count == 0) { - // The first stack frame is held in Current. + // Performance optimization: reuse the first stackframe on the first push operation. + // NB need to be careful when making writes to Current _before_ the first `Push` + // operation is performed. _count = 1; } else { - JsonTypeInfo jsonTypeInfo = Current.GetPolymorphicJsonPropertyInfo().JsonTypeInfo; + JsonTypeInfo jsonTypeInfo = Current.GetNestedJsonTypeInfo(); JsonNumberHandling? numberHandling = Current.NumberHandling; EnsurePushCapacity(); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStackFrame.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStackFrame.cs index de718fc366b926..fd3cef55ed3ac4 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStackFrame.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStackFrame.cs @@ -71,18 +71,16 @@ internal struct WriteStackFrame // Preserve Reference public MetadataPropertyName MetadataPropertyName; - /// - /// The run-time JsonPropertyInfo that contains the TypeInfo and ConverterBase for polymorphic scenarios. - /// - /// - /// For objects, it is the for the class and current property. - /// For collections, it is the for the class and current element. - /// - private JsonPropertyInfo? PolymorphicJsonPropertyInfo; + // Serialization state for the child value serialized by the current frame. + public PolymorphicSerializationState PolymorphicSerializationState; + // Holds the entered polymorphic type info and acts as an LRU cache for element/field serializations. + private JsonPropertyInfo? PolymorphicJsonTypeInfo; // Whether to use custom number handling. public JsonNumberHandling? NumberHandling; + public bool IsPushedReferenceForCycleDetection; + public void EndDictionaryElement() { PropertyState = StackFramePropertyState.None; @@ -92,33 +90,64 @@ public void EndProperty() { JsonPropertyInfo = null!; JsonPropertyNameAsString = null; - PolymorphicJsonPropertyInfo = null; PropertyState = StackFramePropertyState.None; } /// - /// Return the property that contains the correct polymorphic properties including - /// the ConverterStrategy and ConverterBase. + /// Returns the JsonTypeInfo instance for the nested value we are trying to access. /// - public JsonPropertyInfo GetPolymorphicJsonPropertyInfo() + public JsonTypeInfo GetNestedJsonTypeInfo() { - return PolymorphicJsonPropertyInfo ?? JsonPropertyInfo!; + JsonPropertyInfo? propInfo = + PolymorphicSerializationState == PolymorphicSerializationState.PolymorphicReEntryStarted ? + PolymorphicJsonTypeInfo : + JsonPropertyInfo; + + return propInfo!.JsonTypeInfo; } /// /// Initializes the state for polymorphic cases and returns the appropriate converter. /// - public JsonConverter InitializeReEntry(Type type, JsonSerializerOptions options) + public JsonConverter? ResolvePolymorphicConverter(object value, Type typeToConvert, JsonSerializerOptions options) { - // For perf, avoid the dictionary lookup in GetOrAddClass() for every element of a collection + Debug.Assert(value != null); + Debug.Assert(PolymorphicSerializationState != PolymorphicSerializationState.PolymorphicReEntryStarted); + + if (PolymorphicSerializationState == PolymorphicSerializationState.PolymorphicReEntrySuspended) + { + // Quickly retrieve the polymorphic converter in case of a re-entrant continuation + Debug.Assert(PolymorphicJsonTypeInfo != null && value.GetType() == PolymorphicJsonTypeInfo.PropertyType); + return PolymorphicJsonTypeInfo.ConverterBase; + } + + Type runtimeType = value.GetType(); + if (runtimeType == typeToConvert) + { + return null; + } + + // For perf, avoid the dictionary lookup in GetOrAddJsonTypeInfo() for every element of a collection // if the current element is the same type as the previous element. - if (PolymorphicJsonPropertyInfo?.PropertyType != type) + if (PolymorphicJsonTypeInfo?.PropertyType != runtimeType) { - JsonTypeInfo typeInfo = options.GetOrAddJsonTypeInfo(type); - PolymorphicJsonPropertyInfo = typeInfo.PropertyInfoForTypeInfo; + JsonTypeInfo typeInfo = options.GetOrAddJsonTypeInfo(runtimeType); + PolymorphicJsonTypeInfo = typeInfo.PropertyInfoForTypeInfo; } - return PolymorphicJsonPropertyInfo.ConverterBase; + return PolymorphicJsonTypeInfo.ConverterBase; + } + + public void EnterPolymorphicConverter() + { + Debug.Assert(PolymorphicSerializationState != PolymorphicSerializationState.PolymorphicReEntryStarted); + PolymorphicSerializationState = PolymorphicSerializationState.PolymorphicReEntryStarted; + } + + public void ExitPolymorphicConverter(bool success) + { + Debug.Assert(PolymorphicSerializationState == PolymorphicSerializationState.PolymorphicReEntryStarted); + PolymorphicSerializationState = success ? PolymorphicSerializationState.None : PolymorphicSerializationState.PolymorphicReEntrySuspended; } } } From 9ecc1729acf0e9cfe0eafb5dfd16a4988328e80d Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Tue, 1 Mar 2022 11:12:31 +0000 Subject: [PATCH 2/2] address feedback --- .../Converters/Collection/JsonCollectionConverter.cs | 2 +- .../Converters/Collection/JsonDictionaryConverter.cs | 2 +- .../Converters/Object/ObjectDefaultConverter.cs | 4 ++-- .../System/Text/Json/Serialization/JsonConverterOfT.cs | 9 +++------ .../Serialization/JsonSerializer.Write.HandleMetadata.cs | 2 -- 5 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonCollectionConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonCollectionConverter.cs index f8052a9b2b672a..05184a325d949e 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonCollectionConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonCollectionConverter.cs @@ -269,7 +269,7 @@ internal override bool OnTryWrite( state.Current.ProcessedStartToken = true; if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve) { - MetadataPropertyName metadata = JsonSerializer.WriteReferenceForCollection(this, value, ref state, writer); + MetadataPropertyName metadata = JsonSerializer.WriteReferenceForCollection(this, ref state, writer); Debug.Assert(metadata != MetadataPropertyName.Ref); state.Current.MetadataPropertyName = metadata; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonDictionaryConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonDictionaryConverter.cs index dd3cacec79f659..9782a0a8f8e91b 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonDictionaryConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/JsonDictionaryConverter.cs @@ -311,7 +311,7 @@ internal sealed override bool OnTryWrite( writer.WriteStartObject(); if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve) { - MetadataPropertyName propertyName = JsonSerializer.WriteReferenceForObject(this, dictionary, ref state, writer); + MetadataPropertyName propertyName = JsonSerializer.WriteReferenceForObject(this, ref state, writer); Debug.Assert(propertyName != MetadataPropertyName.Ref); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs index 032b6209826c8f..e4f139bb7982d4 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs @@ -261,7 +261,7 @@ internal sealed override bool OnTryWrite( writer.WriteStartObject(); if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve) { - MetadataPropertyName propertyName = JsonSerializer.WriteReferenceForObject(this, obj, ref state, writer); + MetadataPropertyName propertyName = JsonSerializer.WriteReferenceForObject(this, ref state, writer); Debug.Assert(propertyName != MetadataPropertyName.Ref); } @@ -311,7 +311,7 @@ internal sealed override bool OnTryWrite( writer.WriteStartObject(); if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve) { - MetadataPropertyName propertyName = JsonSerializer.WriteReferenceForObject(this, obj, ref state, writer); + MetadataPropertyName propertyName = JsonSerializer.WriteReferenceForObject(this, ref state, writer); Debug.Assert(propertyName != MetadataPropertyName.Ref); } 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 index eb9cced2bb364a..5a99d5cd8aecbe 100644 --- 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 @@ -396,13 +396,10 @@ value is not null && #endif state.Pop(success); - if (success) + if (success && state.Current.IsPushedReferenceForCycleDetection) { - if (state.Current.IsPushedReferenceForCycleDetection) - { - state.ReferenceResolver.PopReferenceForCycleDetection(); - state.Current.IsPushedReferenceForCycleDetection = false; - } + state.ReferenceResolver.PopReferenceForCycleDetection(); + state.Current.IsPushedReferenceForCycleDetection = false; } #if DEBUG Debug.Assert(ReferenceEquals(originalJsonTypeInfo, state.Current.JsonTypeInfo)); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleMetadata.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleMetadata.cs index 143060080ac90e..68eff22631808b 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleMetadata.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleMetadata.cs @@ -15,7 +15,6 @@ public static partial class JsonSerializer internal static MetadataPropertyName WriteReferenceForObject( JsonConverter jsonConverter, - object currentValue, ref WriteStack state, Utf8JsonWriter writer) { @@ -32,7 +31,6 @@ internal static MetadataPropertyName WriteReferenceForObject( internal static MetadataPropertyName WriteReferenceForCollection( JsonConverter jsonConverter, - object currentValue, ref WriteStack state, Utf8JsonWriter writer) {