From f7fa8e746c6edbf373c30048ae37d4fe9441168d Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Thu, 2 Feb 2023 23:36:09 +0000 Subject: [PATCH 1/2] Use eager evaluation when configuring the JsonTypeInfo type graph. --- .../JsonSerializerOptions.Caching.cs | 23 ++-- .../Metadata/JsonPropertyInfo.cs | 33 ++--- .../Serialization/Metadata/JsonTypeInfo.cs | 124 +++++++----------- 3 files changed, 72 insertions(+), 108 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs index 6e67c8212d89c..aac4b744053d4 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs @@ -18,6 +18,15 @@ public sealed partial class JsonSerializerOptions /// Encapsulates all cached metadata referenced by the current instance. /// Context can be shared across multiple equivalent options instances. /// + 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. @@ -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(); @@ -140,13 +149,6 @@ internal void ClearCaches() _objectTypeInfo = null; } - private CachingContext? GetCachingContext() - { - Debug.Assert(IsReadOnly); - - return _cachingContext ??= TrackedCachingContexts.GetOrCreate(this); - } - /// /// Stores and manages all reflection caches for one or more instances. /// NB the type encapsulates the original options instance and only consults that one when building new types; @@ -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() { diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs index af5d97b009a18..5eb7539b5fc32 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs @@ -140,7 +140,7 @@ internal JsonIgnoreCondition? IgnoreCondition get => _ignoreCondition; set { - Debug.Assert(!_isConfigured); + Debug.Assert(!IsConfigured); ConfigureIgnoreCondition(value); _ignoreCondition = value; } @@ -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) { @@ -300,7 +288,9 @@ internal void Configure() } else { - _jsonTypeInfo ??= Options.GetTypeInfoInternal(PropertyType, ensureConfigured: false); + _jsonTypeInfo ??= Options.GetTypeInfoInternal(PropertyType); + _jsonTypeInfo.EnsureConfigured(); + DetermineEffectiveConverter(_jsonTypeInfo); DetermineNumberHandlingForProperty(); DetermineSerializationCapabilities(); @@ -330,6 +320,8 @@ internal void Configure() Debug.Assert(!IgnoreNullTokensOnRead); } + + IsConfigured = true; } private protected abstract void DetermineEffectiveConverter(JsonTypeInfo jsonTypeInfo); @@ -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},"); @@ -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 @@ -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; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs index 90f433fb64455..b592688fc696b 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs @@ -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.SerializeHandler is populated and is compatible with the associated Options instance. internal bool CanUseSerializeHandler { get; private protected set; } @@ -314,84 +311,47 @@ internal void ValidateCanBeUsedForMetadataSerialization() } } + internal Type? ElementType { get; } + internal Type? KeyType { get; } + /// /// Return the JsonTypeInfo for the element type, or null if the type is not an enumerable or dictionary. /// - /// - /// This should not be called during warm-up (initial creation of JsonTypeInfos) to avoid recursive behavior - /// which could result in a StackOverflowException. - /// 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; - /// /// Return the JsonTypeInfo for the key type, or null if the type is not a dictionary. /// - /// - /// This should not be called during warm-up (initial creation of JsonTypeInfos) to avoid recursive behavior - /// which could result in a StackOverflowException. - /// 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; /// /// Gets the value associated with the current instance. @@ -530,61 +490,59 @@ internal void VerifyMutable() } } - private volatile bool _isConfigured; - private readonly object _configureLock = new object(); + internal bool IsConfigured => _configureState == 2; + private volatile int _configureState; // 0: not configured, 1: configuring, 2: configured private ExceptionDispatchInfo? _cachedConfigureError; - internal bool IsConfigured => _isConfigured; - 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) { - if (_isConfigured) + if (_configureState != 0) + { + // The value of _configureState is either + // 1: recursive instance configured by this thread OR + // 2: instance already configured by another thread. return; + } _cachedConfigureError?.Throw(); try { + _configureState = 1; Configure(); - - IsReadOnly = true; - _isConfigured = true; + _configureState = 2; } catch (Exception e) { _cachedConfigureError = ExceptionDispatchInfo.Capture(e); + _configureState = 0; 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(); @@ -595,6 +553,18 @@ internal void Configure() } } + if (ElementType != null) + { + _elementTypeInfo ??= Options.GetTypeInfoInternal(ElementType); + _elementTypeInfo.EnsureConfigured(); + } + + if (KeyType != null) + { + _keyTypeInfo ??= Options.GetTypeInfoInternal(KeyType); + _keyTypeInfo.EnsureConfigured(); + } + if (PolymorphismOptions != null) { PolymorphicTypeResolver = new PolymorphicTypeResolver(this); @@ -947,7 +917,7 @@ internal void ConfigureProperties() } } - property.EnsureConfigured(); + property.Configure(); } NumberOfRequiredProperties = numberOfRequiredProperties; From bceae680d898995f465d2b8331b702cdb420edea Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Fri, 3 Feb 2023 13:03:11 +0000 Subject: [PATCH 2/2] Use enums to model JsonTypeInfo configuration state. --- .../Serialization/Metadata/JsonTypeInfo.cs | 21 +++++++++++-------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs index b592688fc696b..251d64e00e70a 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs @@ -490,8 +490,10 @@ internal void VerifyMutable() } } - internal bool IsConfigured => _configureState == 2; - private volatile int _configureState; // 0: not configured, 1: configuring, 2: configured + internal bool IsConfigured => _configurationState == ConfigurationState.Configured; + private volatile ConfigurationState _configurationState; + private enum ConfigurationState : byte { NotConfigured = 0, Configuring = 1, Configured = 2 }; + private ExceptionDispatchInfo? _cachedConfigureError; internal void EnsureConfigured() @@ -508,11 +510,12 @@ void ConfigureSynchronized() lock (Options.CacheContext) { - if (_configureState != 0) + if (_configurationState != ConfigurationState.NotConfigured) { - // The value of _configureState is either - // 1: recursive instance configured by this thread OR - // 2: instance already configured by another thread. + // 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; } @@ -520,14 +523,14 @@ void ConfigureSynchronized() try { - _configureState = 1; + _configurationState = ConfigurationState.Configuring; Configure(); - _configureState = 2; + _configurationState = ConfigurationState.Configured; } catch (Exception e) { _cachedConfigureError = ExceptionDispatchInfo.Capture(e); - _configureState = 0; + _configurationState = ConfigurationState.NotConfigured; throw; } }