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

Jump to bottom

Split Reflection and SourceGen TypeInfos #67526

Merged
merged 3 commits into from
Apr 7, 2022
Merged

Split Reflection and SourceGen TypeInfos #67526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
46a5687
Split Reflection and SourceGen TypeInfos
krwq Apr 4, 2022
08ca87b
Apply PR feedback
krwq Apr 5, 2022
a772f78
improve exception unwrapping
krwq Apr 6, 2022
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
3 changes: 2 additions & 1 deletion src/libraries/System.Text.Json/src/System.Text.Json.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ System.Text.Json.Nodes.JsonValue</PackageDescription>
<Compile Include="System\Text\Json\Serialization\JsonSerializer.Write.Node.cs" />
<Compile Include="System\Text\Json\Serialization\JsonSerializerContext.cs" />
<Compile Include="System\Text\Json\Serialization\JsonSerializerOptions.Caching.cs" />
<Compile Include="System\Text\Json\Serialization\Metadata\ReflectionJsonTypeInfoOfT.cs" />
<Compile Include="System\Text\Json\Serialization\PolymorphicSerializationState.cs" />
<Compile Include="System\Text\Json\Serialization\ReferenceEqualsWrapper.cs" />
<Compile Include="System\Text\Json\Serialization\ConverterStrategy.cs" />
Expand Down Expand Up @@ -242,7 +243,7 @@ System.Text.Json.Nodes.JsonValue</PackageDescription>
<Compile Include="System\Text\Json\Serialization\Metadata\JsonPropertyInfo.cs" />
<Compile Include="System\Text\Json\Serialization\Metadata\JsonPropertyInfoOfT.cs" />
<Compile Include="System\Text\Json\Serialization\Metadata\JsonPropertyInfoValuesOfT.cs" />
<Compile Include="System\Text\Json\Serialization\Metadata\JsonTypeInfoInternalOfT.cs" />
<Compile Include="System\Text\Json\Serialization\Metadata\SourceGenJsonTypeInfoOfT.cs" />
<Compile Include="System\Text\Json\Serialization\Metadata\JsonTypeInfoOfT.cs" />
<Compile Include="System\Text\Json\Serialization\Metadata\JsonTypeInfo.Cache.cs" />
<Compile Include="System\Text\Json\Serialization\Metadata\JsonTypeInfo.cs" />
Expand Down
24 changes: 1 addition & 23 deletions ....Text.Json/src/System/Text/Json/Serialization/Converters/JsonMetadataServicesConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,24 +50,7 @@ public JsonMetadataServicesConverter(Func<JsonConverter<T>> converterCreator!!,
}

internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, out T? value)
{
JsonTypeInfo jsonTypeInfo = state.Current.JsonTypeInfo;

if (_converterStrategy == ConverterStrategy.Object)
{
if (jsonTypeInfo.PropertyCache == null)
{
jsonTypeInfo.InitializePropCache();
}

if (jsonTypeInfo.ParameterCache == null && jsonTypeInfo.IsObjectWithParameterizedCtor)
{
jsonTypeInfo.InitializeParameterCache();
}
}

return Converter.OnTryRead(ref reader, typeToConvert, options, ref state, out value);
}
=> Converter.OnTryRead(ref reader, typeToConvert, options, ref state, out value);

internal override bool OnTryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions options, ref WriteStack state)
{
Expand All @@ -84,11 +67,6 @@ jsonTypeInfo is JsonTypeInfo<T> info &&
return true;
}

if (_converterStrategy == ConverterStrategy.Object && jsonTypeInfo.PropertyCache == null)
{
jsonTypeInfo.InitializePropCache();
}

return Converter.OnTryWrite(writer, value, options, ref state);
}

Expand Down
8 changes: 4 additions & 4 deletions ....Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,7 +287,7 @@ internal sealed override bool OnTryWrite(
{
// Remember the current property for JsonPath support if an exception is thrown.
state.Current.JsonPropertyInfo = jsonPropertyInfo;
state.Current.NumberHandling = jsonPropertyInfo.NumberHandling;
state.Current.NumberHandling = jsonPropertyInfo.EffectiveNumberHandling;

bool success = jsonPropertyInfo.GetMemberAndWriteJson(obj, ref state, writer);
// Converters only return 'false' when out of data which is not possible in fast path.
Expand All @@ -303,7 +303,7 @@ internal sealed override bool OnTryWrite(
{
// Remember the current property for JsonPath support if an exception is thrown.
state.Current.JsonPropertyInfo = dataExtensionProperty;
state.Current.NumberHandling = dataExtensionProperty.NumberHandling;
state.Current.NumberHandling = dataExtensionProperty.EffectiveNumberHandling;

bool success = dataExtensionProperty.GetMemberAndWriteJsonExtensionData(obj, ref state, writer);
Debug.Assert(success);
Expand Down Expand Up @@ -340,7 +340,7 @@ internal sealed override bool OnTryWrite(
if (jsonPropertyInfo.ShouldSerialize)
{
state.Current.JsonPropertyInfo = jsonPropertyInfo;
state.Current.NumberHandling = jsonPropertyInfo.NumberHandling;
state.Current.NumberHandling = jsonPropertyInfo.EffectiveNumberHandling;

if (!jsonPropertyInfo.GetMemberAndWriteJson(obj!, ref state, writer))
{
Expand Down Expand Up @@ -370,7 +370,7 @@ internal sealed override bool OnTryWrite(
{
// Remember the current property for JsonPath support if an exception is thrown.
state.Current.JsonPropertyInfo = dataExtensionProperty;
state.Current.NumberHandling = dataExtensionProperty.NumberHandling;
state.Current.NumberHandling = dataExtensionProperty.EffectiveNumberHandling;

if (!dataExtensionProperty.GetMemberAndWriteJsonExtensionData(obj, ref state, writer))
{
Expand Down
10 changes: 2 additions & 8 deletions ...Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,15 +52,9 @@ protected sealed override void InitializeConstructorArgumentCaches(ref ReadStack
{
JsonTypeInfo typeInfo = state.Current.JsonTypeInfo;

// Ensure property cache has been initialized.
Debug.Assert(typeInfo.PropertyCache != null);
Debug.Assert(typeInfo.ParameterCache != null);

if (typeInfo.ParameterCache == null)
{
typeInfo.InitializePropCache();
}

List<KeyValuePair<string, JsonParameterInfo?>> cache = typeInfo.ParameterCache!.List;
List<KeyValuePair<string, JsonParameterInfo?>> cache = typeInfo.ParameterCache.List;
object?[] arguments = ArrayPool<object>.Shared.Rent(cache.Count);

for (int i = 0; i < typeInfo.ParameterCount; i++)
Expand Down
8 changes: 6 additions & 2 deletions .../Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ internal sealed override bool OnTryRead(ref Utf8JsonReader reader, Type typeToCo

state.Current.JsonPropertyName = propertyNameArray;
state.Current.JsonPropertyInfo = jsonPropertyInfo;
state.Current.NumberHandling = jsonPropertyInfo.NumberHandling;
state.Current.NumberHandling = jsonPropertyInfo.EffectiveNumberHandling;

bool useExtensionProperty = dataExtKey != null;

Expand Down Expand Up @@ -505,7 +505,11 @@ private static bool HandlePropertyWithContinuation(
[MethodImpl(MethodImplOptions.AggressiveInlining)]
private void BeginRead(ref ReadStack state, ref Utf8JsonReader reader, JsonSerializerOptions options)
{
if (state.Current.JsonTypeInfo.ParameterCount != state.Current.JsonTypeInfo.ParameterCache!.Count)
JsonTypeInfo jsonTypeInfo = state.Current.JsonTypeInfo;

jsonTypeInfo.ValidateCanBeUsedForDeserialization();

if (jsonTypeInfo.ParameterCount != jsonTypeInfo.ParameterCache!.Count)
{
ThrowHelper.ThrowInvalidOperationException_ConstructorParameterIncompleteBinding(TypeToConvert);
}
Expand Down
2 changes: 1 addition & 1 deletion ...em.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ internal static JsonPropertyInfo LookupProperty(
}

state.Current.JsonPropertyInfo = jsonPropertyInfo;
state.Current.NumberHandling = jsonPropertyInfo.NumberHandling;
state.Current.NumberHandling = jsonPropertyInfo.EffectiveNumberHandling;
return jsonPropertyInfo;
}

Expand Down
1 change: 1 addition & 0 deletions ...raries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Helpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ public static partial class JsonSerializer
var reader = new Utf8JsonReader(utf8Json, isFinalBlock: true, readerState);

ReadStack state = default;
jsonTypeInfo.EnsureConfigured();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be moved to the JsonTypeInfo construction phase (to avoid making contention a concern?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No for 3 reasons:

  • this code path can be hit from APIs taking directly JsonTypeInfo which could not be preconfigured
  • putting that directly in JsonTypeInfo constructor can cause stack overflow for source gen
  • type returned from GetTypeInfo needs to not congiured as it needs to allow for editing as it's a common scenario. Configuring produces the cache

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, essentially it is accounting for instances not returned from the JsonSerializerOptions cache so cycles cannot be accounted for?

Copy link
Member Author

@krwq krwq Apr 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the first part of the question - yes - this accounts for source gen JsonTypeInfos. Yes it is done so that cycles cannot occur (that's a bit impl specific to source gen) but also so that JsonTypeInfo can be edited before this is called.

state.Initialize(jsonTypeInfo);

TValue? value;
Expand Down
5 changes: 4 additions & 1 deletion ...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ static async IAsyncEnumerable<TValue> CreateAsyncEnumerableDeserializer(
JsonConverter converter = QueueOfTConverter<Queue<TValue>, TValue>.Instance;
JsonTypeInfo jsonTypeInfo = CreateQueueJsonTypeInfo<TValue>(converter, options);
ReadStack readStack = default;
jsonTypeInfo.EnsureConfigured();
readStack.Initialize(jsonTypeInfo, supportContinuation: true);
var jsonReaderState = new JsonReaderState(options.GetReaderOptions());

Expand Down Expand Up @@ -334,7 +335,7 @@ static async IAsyncEnumerable<TValue> CreateAsyncEnumerableDeserializer(
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode",
Justification = "Workaround for https://github.com/mono/linker/issues/1416. All usages are marked as unsafe.")]
private static JsonTypeInfo CreateQueueJsonTypeInfo<TValue>(JsonConverter queueConverter, JsonSerializerOptions queueOptions) =>
new JsonTypeInfo(typeof(Queue<TValue>), queueConverter, queueOptions);
new ReflectionJsonTypeInfo<Queue<TValue>>(queueConverter, queueOptions);

internal static async ValueTask<TValue?> ReadAllAsync<TValue>(
Stream utf8Json,
Expand All @@ -344,6 +345,7 @@ private static JsonTypeInfo CreateQueueJsonTypeInfo<TValue>(JsonConverter queueC
JsonSerializerOptions options = jsonTypeInfo.Options;
var bufferState = new ReadBufferState(options.DefaultBufferSize);
ReadStack readStack = default;
jsonTypeInfo.EnsureConfigured();
readStack.Initialize(jsonTypeInfo, supportContinuation: true);
JsonConverter converter = readStack.Current.JsonPropertyInfo!.ConverterBase;
var jsonReaderState = new JsonReaderState(options.GetReaderOptions());
Expand Down Expand Up @@ -374,6 +376,7 @@ private static JsonTypeInfo CreateQueueJsonTypeInfo<TValue>(JsonConverter queueC
JsonSerializerOptions options = jsonTypeInfo.Options;
var bufferState = new ReadBufferState(options.DefaultBufferSize);
ReadStack readStack = default;
jsonTypeInfo.EnsureConfigured();
readStack.Initialize(jsonTypeInfo, supportContinuation: true);
JsonConverter converter = readStack.Current.JsonPropertyInfo!.ConverterBase;
var jsonReaderState = new JsonReaderState(options.GetReaderOptions());
Expand Down
1 change: 1 addition & 0 deletions ...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,7 @@ public static partial class JsonSerializer

private static TValue? ReadFromSpan<TValue>(ReadOnlySpan<char> json, JsonTypeInfo jsonTypeInfo)
{
jsonTypeInfo.EnsureConfigured();
byte[]? tempArray = null;

// For performance, avoid obtaining actual byte count unless memory usage is higher than the threshold.
Expand Down
1 change: 1 addition & 0 deletions ...System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Utf8JsonReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ public static partial class JsonSerializer
private static TValue? Read<TValue>(ref Utf8JsonReader reader, JsonTypeInfo jsonTypeInfo)
{
ReadStack state = default;
jsonTypeInfo.EnsureConfigured();
state.Initialize(jsonTypeInfo);

JsonReaderState readerState = reader.CurrentState;
Expand Down
1 change: 1 addition & 0 deletions ...aries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Helpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ jsonTypeInfo is not JsonTypeInfo<TValue> ||
"Incorrect method called. WriteUsingGeneratedSerializer() should have been called instead.");

WriteStack state = default;
jsonTypeInfo.EnsureConfigured();
state.Initialize(jsonTypeInfo, supportContinuation: false, supportAsync: false);

JsonConverter converter = jsonTypeInfo.PropertyInfoForTypeInfo.ConverterBase;
Expand Down
2 changes: 2 additions & 0 deletions ...raries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Stream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ private static async Task WriteStreamAsync<TValue>(
using (var writer = new Utf8JsonWriter(bufferWriter, writerOptions))
{
WriteStack state = new WriteStack { CancellationToken = cancellationToken };
jsonTypeInfo.EnsureConfigured();
JsonConverter converter = state.Initialize(jsonTypeInfo, supportContinuation: true, supportAsync: true);

bool isFinalBlock;
Expand Down Expand Up @@ -329,6 +330,7 @@ private static void WriteStream<TValue>(
using (var writer = new Utf8JsonWriter(bufferWriter, writerOptions))
{
WriteStack state = default;
jsonTypeInfo.EnsureConfigured();
JsonConverter converter = state.Initialize(jsonTypeInfo, supportContinuation: true, supportAsync: false);

bool isFinalBlock;
Expand Down
29 changes: 28 additions & 1 deletion ...s/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using System.Runtime.ExceptionServices;
using System.Text.Json.Reflection;
using System.Text.Json.Serialization;
using System.Text.Json.Serialization.Converters;
Expand Down Expand Up @@ -41,7 +42,33 @@ private static void RootReflectionSerializerDependencies()
}

[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options) => new JsonTypeInfo(type, options);
static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options)
{
JsonTypeInfo.ValidateType(type, null, null, options);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: probably fine for the scope of this PR, but eventually I'd like to see this method moved out of JsonSerializerOptions responsibility.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

most of the work this does is checking if the type is complete generic type so essentially I'd need to duplicate that logic elsewhere, we previously could have it as part of JsonTypeInfo because it wasn't generic but now we cannot instantiate it otherwise


MethodInfo methodInfo = typeof(JsonSerializerOptions).GetMethod(nameof(CreateReflectionJsonTypeInfo), BindingFlags.NonPublic | BindingFlags.Instance)!;
#if NETCOREAPP
return (JsonTypeInfo)methodInfo.MakeGenericMethod(type).Invoke(options, BindingFlags.NonPublic | BindingFlags.DoNotWrapExceptions, null, null, null)!;
#else
try
{
return (JsonTypeInfo)methodInfo.MakeGenericMethod(type).Invoke(options, null)!;
}
catch (TargetInvocationException ex)
{
// Some of the validation is done during construction (i.e. validity of JsonConverter, inner types etc.)
// therefore we need to unwrap TargetInvocationException for better user experience
ExceptionDispatchInfo.Capture(ex.InnerException).Throw();
throw ex.InnerException;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't need to emit a throw opcode here, since the previous method is guaranteed to throw.

Suggested change
throw ex.InnerException;
return null!;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix in the next PR

}
#endif
}
}

[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
private JsonTypeInfo<T> CreateReflectionJsonTypeInfo<T>()
{
return new ReflectionJsonTypeInfo<T>(this);
}

[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
Expand Down
15 changes: 8 additions & 7 deletions src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,21 +600,22 @@ internal void InitializeForReflectionSerializer()
private JsonTypeInfo GetJsonTypeInfoFromContextOrCreate(Type type)
{
JsonTypeInfo? info = _serializerContext?.GetTypeInfo(type);
if (info != null)
if (info == null && IsInitializedForReflectionSerializer)
{
return info;
Debug.Assert(
s_typeInfoCreationFunc != null,
"Reflection-based JsonTypeInfo creator should be initialized if IsInitializedForReflectionSerializer is true.");
info = s_typeInfoCreationFunc(type, this);
}

if (!IsInitializedForReflectionSerializer)
if (info == null)
{
ThrowHelper.ThrowNotSupportedException_NoMetadataForType(type);
return null!;
}

Debug.Assert(
s_typeInfoCreationFunc != null,
"Reflection-based JsonTypeInfo creator should be initialized if IsInitializedForReflectionSerializer is true.");
return s_typeInfoCreationFunc(type, this);
info.EnsureConfigured();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're calling EnsureConfigured here, do we really need to repeat the call in the serialization entrypoints? I suspect this means we could move EnsureConfigured responsibility to the func itself.

Copy link
Member Author

@krwq krwq Apr 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes because serialization is also hit through some APIs which directly pass in JsonType info (as a side note this call is very cheap, it's marked with aggressive inlining and it's a simple bool flag check in case where it's already configured)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to rename the method to something like EnsureInitialized() or EnsureLocked() to better communicate what it does to the instance?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually in my original prototype I called it "EnsureLocked" but here it's not really locking anything yet. What about I rename it to "EnsureLocked" once it starts ensuring no one tries to modify it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but here it's not really locking anything yet

I'm guessing though that it would eventually lock the public setters we'd be adding in the future? I think it's ok to introduce the name now even though it doesn't actually do exactly that since it signifies intent. Alternatively "EnsureInitialized" can be a generic enough description.

My issue with "EnsureConfigured" is that it's not really configuring anything (that happens on construction), it's just building caches based on configuration that has already been specified. Again though, that's me nit-picking on internal method names.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about BuildCache/EnsureCacheBuilt. I get similar notion with EnsureInitialized as you're getting with EnsureConfigured

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think EnsureInitialized is good enough and adequately communicates intent.

return info;
}

internal JsonDocumentOptions GetDocumentOptions()
Expand Down
Loading