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

Use eager evaluation when configuring the JsonTypeInfo type graph. #81576

Merged
merged 2 commits into from
Feb 3, 2023
Merged

Use eager evaluation when configuring the JsonTypeInfo type graph. #81576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f7fa8e7
Use eager evaluation when configuring the JsonTypeInfo type graph.
eiriktsarpalis Feb 2, 2023
bceae68
Use enums to model JsonTypeInfo configuration state.
eiriktsarpalis Feb 3, 2023
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
23 changes: 13 additions & 10 deletions ...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,15 @@ public sealed partial class JsonSerializerOptions
/// Encapsulates all cached metadata referenced by the current <see cref="JsonSerializerOptions" /> instance.
/// Context can be shared across multiple equivalent options instances.
/// </summary>
internal CachingContext CacheContext
{
get
{
Debug.Assert(IsReadOnly);
return _cachingContext ??= TrackedCachingContexts.GetOrCreate(this);
}
}

private CachingContext? _cachingContext;

// Simple LRU cache for the public (de)serialize entry points that avoid some lookups in _cachingContext.
Expand Down Expand Up @@ -59,7 +68,7 @@ internal JsonTypeInfo GetTypeInfoInternal(Type type, bool ensureConfigured = tru

if (IsReadOnly)
{
typeInfo = GetCachingContext()?.GetOrAddJsonTypeInfo(type);
typeInfo = CacheContext.GetOrAddTypeInfo(type);
if (ensureConfigured)
{
typeInfo?.EnsureConfigured();
Expand Down Expand Up @@ -140,13 +149,6 @@ internal void ClearCaches()
_objectTypeInfo = null;
}

private CachingContext? GetCachingContext()
{
Debug.Assert(IsReadOnly);

return _cachingContext ??= TrackedCachingContexts.GetOrCreate(this);
}

/// <summary>
/// Stores and manages all reflection caches for one or more <see cref="JsonSerializerOptions"/> instances.
/// NB the type encapsulates the original options instance and only consults that one when building new types;
Expand Down Expand Up @@ -175,14 +177,15 @@ public CachingContext(JsonSerializerOptions options, int hashCode)
// If changing please ensure that src/ILLink.Descriptors.LibraryBuild.xml is up-to-date.
public int Count => _jsonTypeInfoCache.Count;

public JsonTypeInfo? GetOrAddJsonTypeInfo(Type type) =>
public JsonTypeInfo? GetOrAddTypeInfo(Type type) =>
#if NETCOREAPP
_jsonTypeInfoCache.GetOrAdd(type, static (type, options) => options.GetTypeInfoNoCaching(type), Options);
#else
_jsonTypeInfoCache.GetOrAdd(type, _jsonTypeInfoFactory);
#endif

public bool TryGetJsonTypeInfo(Type type, [NotNullWhen(true)] out JsonTypeInfo? typeInfo) => _jsonTypeInfoCache.TryGetValue(type, out typeInfo);
public bool TryGetJsonTypeInfo(Type type, [NotNullWhen(true)] out JsonTypeInfo? typeInfo)
=> _jsonTypeInfoCache.TryGetValue(type, out typeInfo);

public void Clear()
{
Expand Down
33 changes: 12 additions & 21 deletions ...ibraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ internal JsonIgnoreCondition? IgnoreCondition
get => _ignoreCondition;
set
{
Debug.Assert(!_isConfigured);
Debug.Assert(!IsConfigured);
ConfigureIgnoreCondition(value);
_ignoreCondition = value;
}
Expand Down Expand Up @@ -272,24 +272,12 @@ private protected void VerifyMutable()
}
}

internal bool IsConfigured => _isConfigured;
private volatile bool _isConfigured;

internal void EnsureConfigured()
{
if (_isConfigured)
{
return;
}

Configure();

_isConfigured = true;
}
internal bool IsConfigured { get; private set; }

internal void Configure()
{
Debug.Assert(ParentTypeInfo != null);
Debug.Assert(!IsConfigured);

if (IsIgnored)
{
Expand All @@ -300,7 +288,9 @@ internal void Configure()
}
else
{
_jsonTypeInfo ??= Options.GetTypeInfoInternal(PropertyType, ensureConfigured: false);
_jsonTypeInfo ??= Options.GetTypeInfoInternal(PropertyType);
_jsonTypeInfo.EnsureConfigured();

DetermineEffectiveConverter(_jsonTypeInfo);
DetermineNumberHandlingForProperty();
DetermineSerializationCapabilities();
Expand Down Expand Up @@ -330,6 +320,8 @@ internal void Configure()

Debug.Assert(!IgnoreNullTokensOnRead);
}

IsConfigured = true;
}

private protected abstract void DetermineEffectiveConverter(JsonTypeInfo jsonTypeInfo);
Expand Down Expand Up @@ -553,7 +545,7 @@ internal string GetDebugInfo(int indent = 0)
sb.AppendLine($"{ind}{{");
sb.AppendLine($"{ind} Name: {Name},");
sb.AppendLine($"{ind} NameAsUtf8.Length: {(NameAsUtf8Bytes?.Length ?? -1)},");
sb.AppendLine($"{ind} IsConfigured: {_isConfigured},");
sb.AppendLine($"{ind} IsConfigured: {IsConfigured},");
sb.AppendLine($"{ind} IsIgnored: {IsIgnored},");
sb.AppendLine($"{ind} CanSerialize: {CanSerialize},");
sb.AppendLine($"{ind} CanDeserialize: {CanDeserialize},");
Expand Down Expand Up @@ -799,8 +791,7 @@ internal JsonTypeInfo JsonTypeInfo
get
{
Debug.Assert(IsConfigured);
Debug.Assert(_jsonTypeInfo != null);
_jsonTypeInfo.EnsureConfigured();
Debug.Assert(_jsonTypeInfo?.IsConfigured == true);
return _jsonTypeInfo;
}
set
Expand Down Expand Up @@ -878,13 +869,13 @@ internal int RequiredPropertyIndex
{
get
{
Debug.Assert(_isConfigured);
Debug.Assert(IsConfigured);
Debug.Assert(IsRequired);
return _index;
}
set
{
Debug.Assert(!_isConfigured);
Debug.Assert(!IsConfigured);
_index = value;
}
}
Expand Down
127 changes: 50 additions & 77 deletions src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,9 +295,6 @@ public JsonPolymorphismOptions? PolymorphismOptions

internal PolymorphicTypeResolver? PolymorphicTypeResolver { get; private set; }

// If enumerable or dictionary, the JsonTypeInfo for the element type.
private JsonTypeInfo? _elementTypeInfo;

// Flag indicating that JsonTypeInfo<T>.SerializeHandler is populated and is compatible with the associated Options instance.
internal bool CanUseSerializeHandler { get; private protected set; }

Expand All @@ -314,84 +311,47 @@ internal void ValidateCanBeUsedForMetadataSerialization()
}
}

internal Type? ElementType { get; }
internal Type? KeyType { get; }

/// <summary>
/// Return the JsonTypeInfo for the element type, or null if the type is not an enumerable or dictionary.
/// </summary>
/// <remarks>
/// This should not be called during warm-up (initial creation of JsonTypeInfos) to avoid recursive behavior
/// which could result in a StackOverflowException.
/// </remarks>
internal JsonTypeInfo? ElementTypeInfo
{
get
{
if (_elementTypeInfo == null)
{
if (ElementType != null)
{
// GetOrAddJsonTypeInfo already ensures JsonTypeInfo is configured
// also see comment on JsonPropertyInfo.JsonTypeInfo
_elementTypeInfo = Options.GetTypeInfoInternal(ElementType);
}
}
else
{
_elementTypeInfo.EnsureConfigured();
}

Debug.Assert(IsConfigured);
return _elementTypeInfo;
}
set
{
// Set by JsonMetadataServices.
Debug.Assert(_elementTypeInfo == null);
Debug.Assert(!IsReadOnly);
Debug.Assert(value is null || value.Type == ElementType);
_elementTypeInfo = value;
}
}

internal Type? ElementType { get; }

// If dictionary, the JsonTypeInfo for the key type.
private JsonTypeInfo? _keyTypeInfo;

/// <summary>
/// Return the JsonTypeInfo for the key type, or null if the type is not a dictionary.
/// </summary>
/// <remarks>
/// This should not be called during warm-up (initial creation of JsonTypeInfos) to avoid recursive behavior
/// which could result in a StackOverflowException.
/// </remarks>
internal JsonTypeInfo? KeyTypeInfo
{
get
{
if (_keyTypeInfo == null)
{
if (KeyType != null)
{
Debug.Assert(Kind == JsonTypeInfoKind.Dictionary);

// GetOrAddJsonTypeInfo already ensures JsonTypeInfo is configured
// also see comment on JsonPropertyInfo.JsonTypeInfo
_keyTypeInfo = Options.GetTypeInfoInternal(KeyType);
}
}
else
{
_keyTypeInfo.EnsureConfigured();
}

Debug.Assert(IsConfigured);
return _keyTypeInfo;
}
set
{
// Set by JsonMetadataServices.
Debug.Assert(_keyTypeInfo == null);
Debug.Assert(!IsReadOnly);
Debug.Assert(value is null || value.Type == KeyType);
_keyTypeInfo = value;
}
}

internal Type? KeyType { get; }
private JsonTypeInfo? _elementTypeInfo;
private JsonTypeInfo? _keyTypeInfo;

/// <summary>
/// Gets the <see cref="JsonSerializerOptions"/> value associated with the current <see cref="JsonTypeInfo" /> instance.
Expand Down Expand Up @@ -530,61 +490,62 @@ internal void VerifyMutable()
}
}

private volatile bool _isConfigured;
private readonly object _configureLock = new object();
private ExceptionDispatchInfo? _cachedConfigureError;
internal bool IsConfigured => _configurationState == ConfigurationState.Configured;
private volatile ConfigurationState _configurationState;
private enum ConfigurationState : byte { NotConfigured = 0, Configuring = 1, Configured = 2 };

internal bool IsConfigured => _isConfigured;
private ExceptionDispatchInfo? _cachedConfigureError;

internal void EnsureConfigured()
{
Debug.Assert(!Monitor.IsEntered(_configureLock), "recursive locking detected.");

if (!_isConfigured)
ConfigureLocked();
if (!IsConfigured)
ConfigureSynchronized();

void ConfigureLocked()
void ConfigureSynchronized()
{
Options.MakeReadOnly();
MakeReadOnly();

_cachedConfigureError?.Throw();

lock (_configureLock)
lock (Options.CacheContext)
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
{
if (_isConfigured)
if (_configurationState != ConfigurationState.NotConfigured)
{
// The value of _configurationState is either
// 'Configuring': recursive instance configured by this thread or
// 'Configured' : instance already configured by another thread.
// We can safely yield the configuration operation in both cases.
return;
}

_cachedConfigureError?.Throw();

try
{
_configurationState = ConfigurationState.Configuring;
Configure();

IsReadOnly = true;
_isConfigured = true;
_configurationState = ConfigurationState.Configured;
}
catch (Exception e)
{
_cachedConfigureError = ExceptionDispatchInfo.Capture(e);
_configurationState = ConfigurationState.NotConfigured;
throw;
}
}
}
}

internal void Configure()
private void Configure()
{
Debug.Assert(Monitor.IsEntered(_configureLock), "Configure called directly, use EnsureConfigured which locks this method");

if (!Options.IsReadOnly)
{
Options.MakeReadOnly();
}

PropertyInfoForTypeInfo.EnsureConfigured();
Debug.Assert(Monitor.IsEntered(Options.CacheContext), "Configure called directly, use EnsureConfigured which synchronizes access to this method");
Debug.Assert(Options.IsReadOnly);
Debug.Assert(IsReadOnly);

PropertyInfoForTypeInfo.Configure();
CanUseSerializeHandler &= Options.CanUseFastPathSerializationLogic;

Debug.Assert(PropertyInfoForTypeInfo.EffectiveConverter.ConverterStrategy == Converter.ConverterStrategy,
$"ConverterStrategy from PropertyInfoForTypeInfo.EffectiveConverter.ConverterStrategy ({PropertyInfoForTypeInfo.EffectiveConverter.ConverterStrategy}) does not match converter's ({Converter.ConverterStrategy})");
if (Kind == JsonTypeInfoKind.Object)
{
ConfigureProperties();
Expand All @@ -595,6 +556,18 @@ internal void Configure()
}
}

if (ElementType != null)
{
_elementTypeInfo ??= Options.GetTypeInfoInternal(ElementType);
_elementTypeInfo.EnsureConfigured();
}

if (KeyType != null)
{
_keyTypeInfo ??= Options.GetTypeInfoInternal(KeyType);
_keyTypeInfo.EnsureConfigured();
}
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved

if (PolymorphismOptions != null)
{
PolymorphicTypeResolver = new PolymorphicTypeResolver(this);
Expand Down Expand Up @@ -947,7 +920,7 @@ internal void ConfigureProperties()
}
}

property.EnsureConfigured();
property.Configure();
}

NumberOfRequiredProperties = numberOfRequiredProperties;
Expand Down