Skip to content

Commit

Permalink
Minor refactorings (#1)
Browse files Browse the repository at this point in the history 
* Makes the following changes:

* Move singleton initialization for DefaultTypeInfoResolver behind a static property.
* Consolidate JsonSerializerContext & IJsonTypeInfoResolver values to a single field.
* Move reflection fallback logic away from JsonSerializerContext and into JsonSerializerOptions

* Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs

* remove testing of removed field
  • Loading branch information
@eiriktsarpalis
eiriktsarpalis authored May 31, 2022
1 parent 29c7468 commit 055ce33
Show file tree
Hide file tree
Showing 19 changed files with 179 additions and 235 deletions.
1 change: 1 addition & 0 deletions src/libraries/System.Text.Json/ref/System.Text.Json.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1215,6 +1215,7 @@ public abstract partial class JsonTypeInfo<T> : System.Text.Json.Serialization.M
{
internal JsonTypeInfo() { }
public new System.Func<T>? CreateObject { get { throw null; } set { } }
[System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
public System.Action<System.Text.Json.Utf8JsonWriter, T>? SerializeHandler { get { throw null; } }
}
public enum JsonTypeInfoKind
Expand Down
39 changes: 12 additions & 27 deletions src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ConfigurationList.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,25 @@
using System.Collections;
using System.Collections.Generic;
using System.Diagnostics;
using System.Globalization;

namespace System.Text.Json.Serialization
{
/// <summary>
/// A list of configuration items that respects the options class being immutable once (de)serialization occurs.
/// A list of configuration items that can be locked for modification
/// </summary>
internal sealed class ConfigurationList<TItem> : IList<TItem>
internal abstract class ConfigurationList<TItem> : IList<TItem>
{
private readonly List<TItem> _list;

public Action<TItem>? OnElementAdded { get; set; }
public Func<bool>? IsReadOnlyFunc { get; set; }
public Action? ThrowImmutableFunc { get; set; }

public ConfigurationList()
public ConfigurationList(IList<TItem>? source = null)
{
_list = new List<TItem>();
_list = source is null ? new List<TItem>() : new List<TItem>(source);
}

public ConfigurationList(IList<TItem> source)
{
_list = new List<TItem>(source is ConfigurationList<TItem> cl ? cl._list : source);
}
protected abstract bool IsLockedInstance { get; }
protected abstract void VerifyMutable();
protected virtual void OnItemAdded(TItem item) { }

public TItem this[int index]
{
Expand All @@ -43,24 +39,13 @@ public TItem this[int index]

VerifyMutable();
_list[index] = value;
OnElementAdded?.Invoke(value);
OnItemAdded(value);
}
}

public int Count => _list.Count;

public bool IsReadOnly => IsReadOnlyFunc != null ? IsReadOnlyFunc() : false;

private void VerifyMutable()
{
Debug.Assert((IsReadOnlyFunc == null) == (ThrowImmutableFunc == null), "IsReadOnlyFunc and ThrowImmutableFunc should be either both set or both unset");

if (IsReadOnlyFunc != null && IsReadOnlyFunc())
{
Debug.Assert(ThrowImmutableFunc != null);
ThrowImmutableFunc();
}
}
public bool IsReadOnly => IsLockedInstance;

public void Add(TItem item)
{
Expand All @@ -71,7 +56,7 @@ public void Add(TItem item)

VerifyMutable();
_list.Add(item);
OnElementAdded?.Invoke(item);
OnItemAdded(item);
}

public void Clear()
Expand Down Expand Up @@ -109,7 +94,7 @@ public void Insert(int index, TItem item)

VerifyMutable();
_list.Insert(index, item);
OnElementAdded?.Invoke(item);
OnItemAdded(item);
}

public bool Remove(TItem item)
Expand Down
2 changes: 1 addition & 1 deletion ....Text.Json/src/System/Text/Json/Serialization/Converters/JsonMetadataServicesConverter.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ internal override bool OnTryWrite(Utf8JsonWriter writer, T value, JsonSerializer
jsonTypeInfo is JsonTypeInfo<T> info &&
info.SerializeHandler != null &&
!state.CurrentContainsMetadata && // Do not use the fast path if state needs to write metadata.
info.Options.JsonSerializerContext?.CanUseSerializationLogic == true)
info.Options.SerializerContext?.CanUseSerializationLogic == true)
{
info.SerializeHandler(writer, value);
return true;
Expand Down
5 changes: 4 additions & 1 deletion src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Helpers.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ private static JsonTypeInfo GetTypeInfo(JsonSerializerOptions? options, Type run
Debug.Assert(runtimeType != null);

options ??= JsonSerializerOptions.Default;
options.EnsureInitializedForReflectionSerializer();
if (!options.IsInitializedForReflectionSerializer)
{
options.InitializeForReflectionSerializer();
}

return options.GetOrAddJsonTypeInfoForRootType(runtimeType);
}
Expand Down
5 changes: 4 additions & 1 deletion ...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -369,7 +369,10 @@ public static partial class JsonSerializer
}

options ??= JsonSerializerOptions.Default;
options.EnsureInitializedForReflectionSerializer();
if (!options.IsInitializedForReflectionSerializer)
{
options.InitializeForReflectionSerializer();
}

JsonTypeInfo jsonTypeInfo = options.GetOrAddJsonTypeInfoForRootType(typeof(TValue));
return CreateAsyncEnumerableDeserializer<TValue>(utf8Json, jsonTypeInfo, cancellationToken);
Expand Down
6 changes: 3 additions & 3 deletions ...aries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Helpers.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ private static void WriteUsingGeneratedSerializer<TValue>(Utf8JsonWriter writer,

if (jsonTypeInfo.HasSerialize &&
jsonTypeInfo is JsonTypeInfo<TValue> typedInfo &&
typedInfo.Options.JsonSerializerContext?.CanUseSerializationLogic == true)
typedInfo.Options.SerializerContext?.CanUseSerializationLogic == true)
{
Debug.Assert(typedInfo.SerializeHandler != null);
typedInfo.SerializeHandler(writer, value);
Expand All @@ -59,8 +59,8 @@ private static void WriteUsingSerializer<TValue>(Utf8JsonWriter writer, in TValu

Debug.Assert(!jsonTypeInfo.HasSerialize ||
jsonTypeInfo is not JsonTypeInfo<TValue> ||
jsonTypeInfo.Options.JsonSerializerContext == null ||
!jsonTypeInfo.Options.JsonSerializerContext.CanUseSerializationLogic,
jsonTypeInfo.Options.SerializerContext == null ||
!jsonTypeInfo.Options.SerializerContext.CanUseSerializationLogic,
"Incorrect method called. WriteUsingGeneratedSerializer() should have been called instead.");

WriteStack state = default;
Expand Down
18 changes: 6 additions & 12 deletions src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerContext.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,9 @@ public abstract partial class JsonSerializerContext : IJsonTypeInfoResolver
/// when instanciating the context, then a new instance is bound and returned.
/// </summary>
/// <remarks>
/// The instance cannot be mutated once it is bound with the context instance.
/// The instance cannot be mutated once it is bound to the context instance.
/// </remarks>
public JsonSerializerOptions Options => _options ??= new JsonSerializerOptions { JsonSerializerContext = this };

// Currently JsonSerializerContext starts falling back to DefaultJsonTypeInfoResolver when some methods are called.
internal IJsonTypeInfoResolver? FallbackResolver { get; set; }
public JsonSerializerOptions Options => _options ??= new JsonSerializerOptions { TypeInfoResolver = this };

/// <summary>
/// Indicates whether pre-generated serialization logic for types in the context
Expand Down Expand Up @@ -87,8 +84,7 @@ protected JsonSerializerContext(JsonSerializerOptions? options)
{
if (options != null)
{
options.JsonSerializerContext = this;
_options = options;
options.TypeInfoResolver = this;
}
}

Expand All @@ -101,15 +97,13 @@ protected JsonSerializerContext(JsonSerializerOptions? options)

JsonTypeInfo? IJsonTypeInfoResolver.GetTypeInfo(Type type, JsonSerializerOptions options)
{
if (_options != options)
if (options != null && _options != options)
{
Debug.Assert(_options != null, "_options are null");
// TODO is this the appropriate exception message to throw?
ThrowHelper.ThrowInvalidOperationException_SerializerContextOptionsImmutable();
}

JsonTypeInfo? typeInfo = GetTypeInfo(type);
typeInfo ??= FallbackResolver?.GetTypeInfo(type, options);
return typeInfo;
return GetTypeInfo(type);
}
}
}
46 changes: 5 additions & 41 deletions ...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ internal JsonTypeInfo GetOrAddJsonTypeInfo(Type type)
if (_cachingContext == null)
{
InitializeCachingContext();
Debug.Assert(_cachingContext != null);
}

return _cachingContext.GetOrAddJsonTypeInfo(type);
Expand Down Expand Up @@ -71,15 +70,11 @@ internal void ClearCaches()
_lastTypeInfo = null;
}

[MemberNotNull(nameof(_cachingContext))]
private void InitializeCachingContext()
{
_isLockedInstance = true;
_cachingContext = TrackedCachingContexts.GetOrCreate(this);
if (_typeInfoResolver != null)
{
Debug.Assert(_cachingContext.Options._typeInfoResolver != null || _typeInfoResolver == s_defaultTypeInfoResolver,
"EqualityComparer should guarantee that null is equivalent to s_defaultTypeInfoResolver");
_cachingContext.Options._typeInfoResolver ??= s_defaultTypeInfoResolver;
}
}

/// <summary>
Expand Down Expand Up @@ -131,6 +126,7 @@ internal static class TrackedCachingContexts

public static CachingContext GetOrCreate(JsonSerializerOptions options)
{
Debug.Assert(options._isLockedInstance, "Cannot create caching contexts for mutable JsonSerializerOptions instances");
ConcurrentDictionary<JsonSerializerOptions, WeakReference<CachingContext>> cache = s_cache;

if (cache.TryGetValue(options, out WeakReference<CachingContext>? wr) && wr.TryGetTarget(out CachingContext? ctx))
Expand Down Expand Up @@ -169,7 +165,6 @@ public static CachingContext GetOrCreate(JsonSerializerOptions options)
{
// Copy fields ignored by the copy constructor
// but are necessary to determine equivalence.
_serializerContext = options._serializerContext,
_typeInfoResolver = options._typeInfoResolver,
};
Debug.Assert(key._cachingContext == null);
Expand Down Expand Up @@ -273,25 +268,6 @@ public bool Equals(JsonSerializerOptions? left, JsonSerializerOptions? right)
{
Debug.Assert(left != null && right != null);

// We cache these values in this specific order enforcing default resolver to be read last
// this is done in case _typeInfoResolver or s_defaultTypeInfoResolver is not initialized yet
// the behavior will still be correct
IJsonTypeInfoResolver? leftResolver = left._typeInfoResolver;
IJsonTypeInfoResolver? rightResolver = right._typeInfoResolver;
IJsonTypeInfoResolver? defaultResolver = Volatile.Read(ref s_defaultTypeInfoResolver);

// we ensure null and s_defaultTypeInfoResolver mean the same thing
// we normalize to null so that calculating hash code is consistent
if (leftResolver == defaultResolver)
{
leftResolver = null;
}

if (rightResolver == defaultResolver)
{
rightResolver = null;
}

return
left._dictionaryKeyPolicy == right._dictionaryKeyPolicy &&
left._jsonPropertyNamingPolicy == right._jsonPropertyNamingPolicy &&
Expand All @@ -310,8 +286,7 @@ public bool Equals(JsonSerializerOptions? left, JsonSerializerOptions? right)
left._includeFields == right._includeFields &&
left._propertyNameCaseInsensitive == right._propertyNameCaseInsensitive &&
left._writeIndented == right._writeIndented &&
leftResolver == rightResolver &&
left._serializerContext == right._serializerContext &&
left._typeInfoResolver == right._typeInfoResolver &&
CompareLists(left._converters, right._converters) &&
#pragma warning disable CA2252 // This API requires opting into preview features
CompareLists(left._polymorphicTypeConfigurations, right._polymorphicTypeConfigurations);
Expand Down Expand Up @@ -339,16 +314,6 @@ static bool CompareLists<TValue>(ConfigurationList<TValue> left, ConfigurationLi

public int GetHashCode(JsonSerializerOptions options)
{
IJsonTypeInfoResolver? typeInfoResolver = options._typeInfoResolver;
IJsonTypeInfoResolver? defaultResolver = Volatile.Read(ref s_defaultTypeInfoResolver);

// we normalize s_defaultTypeInfoResolver to null so that hash code is the same in case s_defaultTypeInfoResolver
// is not initialized yet
if (typeInfoResolver == defaultResolver)
{
typeInfoResolver = null;
}

HashCode hc = default;

hc.Add(options._dictionaryKeyPolicy);
Expand All @@ -368,8 +333,7 @@ public int GetHashCode(JsonSerializerOptions options)
hc.Add(options._includeFields);
hc.Add(options._propertyNameCaseInsensitive);
hc.Add(options._writeIndented);
hc.Add(typeInfoResolver);
hc.Add(options._serializerContext);
hc.Add(options._typeInfoResolver);
GetHashCode(ref hc, options._converters);
#pragma warning disable CA2252 // This API requires opting into preview features
GetHashCode(ref hc, options._polymorphicTypeConfigurations);
Expand Down
32 changes: 5 additions & 27 deletions ...s/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,30 +24,8 @@ public sealed partial class JsonSerializerOptions
// The global list of built-in converters that override CanConvert().
private static JsonConverter[]? s_defaultFactoryConverters;

// Stores the JsonTypeInfo factory, which requires unreferenced code and must be rooted by the reflection-based serializer.
private static IJsonTypeInfoResolver? s_defaultTypeInfoResolver;

[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
[RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
[MemberNotNull(nameof(s_defaultTypeInfoResolver))]
private static void RootReflectionSerializerDependencies()
{
// We need to ensure that all threads use same instance of s_defaultTypeInfoResolver or
// otherwise EqualityComparer for CachingContext may fail even though
// two options might assign _typeInfoResolver to s_defaultTypeInfoResolver
Interlocked.CompareExchange(ref s_defaultTypeInfoResolver, new DefaultJsonTypeInfoResolver(mutable: false), null);

// constructor for DefaultJsonTypeInfoResolver calls RootConverters()
// and therefore both fields are initialized now
Debug.Assert(s_defaultSimpleConverters != null);
Debug.Assert(s_defaultFactoryConverters != null);
}

[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
[RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
// This method should be called only within DefaultJsonTypeInfoResolver constructor
// We need to do this in case DefaultJsonTypeInfoResolver is called standalone without
// serialization happening first
internal static void RootConverters()
{
// s_defaultFactoryConverters is the last field assigned.
Expand Down Expand Up @@ -236,7 +214,7 @@ public JsonConverter GetConverter(Type typeToConvert)
ThrowHelper.ThrowArgumentNullException(nameof(typeToConvert));
}

RootReflectionSerializerDependencies();
RootConverters();
return GetConverterInternal(typeToConvert);
}

Expand All @@ -256,7 +234,7 @@ internal JsonConverter GetConverterFromType(Type typeToConvert)
Debug.Assert(typeToConvert != null);

// Priority 1: If there is a JsonSerializerContext, fetch the converter from there.
JsonConverter? converter = _serializerContext?.GetTypeInfo(typeToConvert)?.Converter;
JsonConverter? converter = SerializerContext?.GetTypeInfo(typeToConvert)?.Converter;

// Priority 2: Attempt to get custom converter added at runtime.
// Currently there is not a way at runtime to override the [JsonConverter] when applied to a property.
Expand Down Expand Up @@ -386,9 +364,9 @@ private JsonConverter GetConverterFromAttribute(JsonConverterAttribute converter

internal bool TryGetDefaultSimpleConverter(Type typeToConvert, [NotNullWhen(true)] out JsonConverter? converter)
{
if (_serializerContext == null && // For consistency do not return any default converters for
// options instances linked to a JsonSerializerContext,
// even if the default converters might have been rooted.
if (SerializerContext is null && // For consistency do not return any default converters for
// options instances linked to a JsonSerializerContext,
// even if the default converters might have been rooted.
s_defaultSimpleConverters != null &&
s_defaultSimpleConverters.TryGetValue(typeToConvert, out converter))
{
Expand Down
Loading

0 comments on commit 055ce33

Please sign in to comment.