Apply a variant of the visitor pattern for reflection-based generic type instantiation. #72373
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsThe System.Text.Json default metadata resolver has been using reflection to instantiate some of its generic metadata types such as This PR updates some of the existing reflection-based generic type instantiation logic so that type parameter reification concerns are delegated to a "type witness", and any domain-specific metadata instantiation is performed via a generic visitor that is accepted by the witness. This approach has a few benefits:
cc @stephentoub @GrabYourPitchforks @jkotas for feedback.
|
| { | ||
| return (JsonTypeInfo)methodInfo.MakeGenericMethod(type).Invoke(null, new[] { options })!; | ||
| } | ||
| catch (TargetInvocationException ex) |
There was a problem hiding this comment.
FYI there is BindingFlags.DoNotWrapExceptions that lets the raw exception throw.
There was a problem hiding this comment.
It's not available in netstandard2.0, hence this extra code. That being said, it is evident from this PR that we haven't been applying the flag consistently.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/TypeWitness.cs
Outdated
Show resolved
Hide resolved
| @@ -576,9 +590,7 @@ public static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions o | |||
| ThrowHelper.ThrowArgumentException_CannotSerializeInvalidType(nameof(type), type, null, null); | |||
| } | |||
|
|
|||
| s_createJsonTypeInfo ??= typeof(JsonTypeInfo).GetMethod(nameof(CreateJsonTypeInfo), new Type[] { typeof(JsonSerializerOptions) })!; | |||
| return (JsonTypeInfo)s_createJsonTypeInfo.MakeGenericMethod(type) | |||
There was a problem hiding this comment.
Just a note that this PR is changing the refactoring done in #67526.
In effect, replacing MethodInfo.MakeGenericMethod with Activor.CreateInstance()?
There was a problem hiding this comment.
It's using Type.MakeGenericType + Activator instead of MethodInfo.MakeGenericMethod, but everything is behind logic marked RUC so it shouldn't matter much.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/TypeWitness.cs
Outdated
Show resolved
Hide resolved
…ion/Metadata/TypeWitness.cs
I expect that this change has performance losses in static footprint. The runtime has to load number of extra generic types for each |
Given that these operations are typically executed just once, the static footprint is going to dominate the performance impact. My guess is that this change is (startup) performance regression for typical json serialization uses. |
How could I measure potential regressions in this space? We have a few cold-start serialization benchmarks in dotnet/performance but effectively they follow the same approach as the benchmark linked in this PR (creating metadata from scratch in the same process). |
|
https://gist.github.com/jkotas/aaaab624fbd034265cbecd6b6d8508ae Current win-x64 release main: ~700ms |
|
Nice and simple. I think I'll keep that for future checks. Thanks. |
ghost
locked as resolved and limited conversation to collaborators
The System.Text.Json default metadata resolver has been using reflection to instantiate some of its generic metadata types such as
JsonTypeInfo<T>andJsonPropertyInfo<T>. This of course is necessary since the value ofTcannot be statically determined in most cases.This PR updates some of the existing reflection-based generic type instantiation logic so that type parameter reification concerns are delegated to a "type witness", and any domain-specific metadata instantiation is performed via a generic visitor that is accepted by the witness. This approach has a few benefits:
TargetInvocationExceptionbeing surfaced to the user.System.Activatoror directMethodInfoinvocation on the micro level.cc @stephentoub @GrabYourPitchforks @jkotas for feedback.