Support STJ Polymorphism #45405
Support STJ Polymorphism #45405
Conversation
|
/benchmark |
|
Crank Pull Request Bot
Benchmarks:
Profiles:
Components:
Arguments: any additional arguments to pass through to crank, e.g. |
|
/benchmark json aspnet-perf-win mvc |
|
Benchmark started for json on aspnet-perf-win with mvc. Logs: link |
json - aspnet-perf-win
|
|
|
||
| namespace Microsoft.AspNetCore.Http; | ||
|
|
||
| public sealed class HttpJsonOptionsSetup : IConfigureOptions<JsonOptions> |
There was a problem hiding this comment.
This should be:
| public sealed class HttpJsonOptionsSetup : IConfigureOptions<JsonOptions> | |
| public sealed class HttpJsonOptionsSetup : IPostConfigureOptions<JsonOptions> |
There was a problem hiding this comment.
@davidfowl I changed to set the DefaultJsonTypeResolver directly when the JsonOptions is created. Are you ok with that?
There was a problem hiding this comment.
What happens if somebody sets it to null?
There was a problem hiding this comment.
Right now, we are failing back to use the serializer with runtime type. In Native AOT probably we will need to throw.
There was a problem hiding this comment.
I just changed the behavior after all discussion, and it will throw when we call JsonOptions.MakeReadOnly().
|
|
||
| if (declaredType is not null && | ||
| runtimeType != declaredType && | ||
| SerializerOptions.TypeInfoResolver != null && |
There was a problem hiding this comment.
TypeInfoResolver should always be non-null at this point.
There was a problem hiding this comment.
The serializer could not be called yet, so, null still valid, no?
There was a problem hiding this comment.
null resolvers only get accepted by the serialization methods that are marked RequiredUnreferencedCode. Given that you plan on replacing these with the linker-safe overloads, your code will sooner or later need to enforce that invariant.
There was a problem hiding this comment.
Given that you plan on replacing these with the linker-safe overloads, your code will sooner or later need to enforce that invariant.
That is true for native AOT (Trim safe)-only. When the app is not configured to trim it could fall back to the current call to the overloaded marked with RequiredUnreferencedCode.
There was a problem hiding this comment.
I think I finally got your point 😂. If the TypeInfoResolver is null at this point is an indication of:
- It was explicitly set to
nullby the users, since we are setting it toDefault...in a non-trimmed(aot) apps, or; - It is a trimmed/aot app and the source gen context was not configured correctly.
If I understand correctly, in both case a call to MakeReadOnly will throw because it is misconfigured, that really make sense, and that is why you are asking for make the call very early. Is that correct?
If so, I believe you are correct, and we should only make calls to linker-safe overloads both here and RDF (doesn't matter if in AOT/Trimmed app or not) and add the MakeReadOnly as early as possible.
src/Mvc/Mvc.Core/src/Formatters/SystemTextJsonOutputFormatter.cs
Outdated
Show resolved
Hide resolved
src/Http/Http.Extensions/src/Microsoft.AspNetCore.Http.Extensions.WarningSuppressions.xml
Show resolved
Hide resolved
| [RequiresUnreferencedCode("System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver might require types that cannot be statically analyzed and might need runtime code generation.")] | ||
| [RequiresDynamicCode("System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver might require types that cannot be statically analyzed and might need runtime code generation. Enable EnsureJsonTrimmability feature switch for native AOT applications.")] |
There was a problem hiding this comment.
Am I correct in understanding that after we have dotnet/sdk#29719, we'll update the JsonOptions static constructor to stop calling this method when the "EnsureJsonTrimmability" feature switch is set?
If we're going to mention the feature switch, we'd probably also want to include the full name, "Microsoft.AspNetCore.EnsureJsonTrimmability", and link to https://learn.microsoft.com/dotnet/core/runtime-config/#runtimeconfigjson, but I think picking one of the msbuild properties and telling people to add it their csproj would be simpler.
Setting any one of "EnsureAspNetCoreJsonTrimmability", "PublishTrimmed", "IsTrimmable" or "PublishAot" properties to true should be sufficient, right? Will warning fire for unreferenced or dynamic code when these properties are not set?
I don't think mentioning System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver is relevant since the user is not directly calling it and probably has no idea why it's getting called. If you remove these attributes, do warnings still propagate from the DefaultJsonTypeInfoResolver? If so, we might as well just leave the original warning, since it's not actionable just yet.
There was a problem hiding this comment.
Am I correct in understanding that after we have dotnet/sdk#29719, we'll update the
JsonOptionsstatic constructor to stop calling this method when the "EnsureJsonTrimmability" feature switch is set?
Exactly and the private method will be trimmed. Something like:
TypeInfoResolver = TrimmingAppContextSwitches.EnsureJsonTrimmability ? null : CreateDefaultTypeResolver()There was a problem hiding this comment.
If we're going to mention the feature switch, we'd probably also want to include the full name, "Microsoft.AspNetCore.EnsureJsonTrimmability", and link to https://learn.microsoft.com/dotnet/core/runtime-config/#runtimeconfigjson, but I think picking one of the msbuild properties and telling people to add it their csproj would be simpler.
Setting any one of "EnsureAspNetCoreJsonTrimmability", "PublishTrimmed", "IsTrimmable" or "PublishAot" properties to true should be sufficient, right?
Right, the idea is when EnsureAspNetCoreJsonTrimmability is set to true, that would be possible by setting directly or using the PublishTrimmed, PublishAot properties, the RuntimeHostConfigurationOption (Microsoft.AspNetCore.EnsureJsonTrimmability) will be set and the linker will be able to do the substitutions.
https://github.com/dotnet/aspnetcore/pull/45886/files
Will warning fire for unreferenced or dynamic code when these properties are not set?
Yes.
There was a problem hiding this comment.
Also, once you have EnsureAspNetCoreJsonTrimmability set to true the TypeInfoResolver will default null when run (F5) or publish, so, developers can understand the behavior during the app development.
There was a problem hiding this comment.
I don't think mentioning System.Text.Json.Serialization.Metadata.DefaultJsonTypeInfoResolver is relevant since the user is not directly calling it and probably has no idea why it's getting called. If you remove these attributes, do warnings still propagate from the DefaultJsonTypeInfoResolver? If so, we might as well just leave the original warning, since it's not actionable just yet.
You are right and it will propagate. I will leave the original warning for now. Thanks.
|
Are we calling a new API that isn't public? The source generator needs to copy this pattern as well. |
I just merged #45593 that makes the APIs public. |
|
@eiriktsarpalis I believe I covered almost all the feedback. Do you have any additional concern before I merge it? |
ghost
added this to the 
Overview
Contributes to #44852
This PR change
RDFandJsonOutputFormatterto, when aJsonPolymorphismOptionsis detected, uses the declared type's JsonTypeInfo to call the serializer.Benchmark results
MapXXX
Summary:
JsonTypeInfo)JsonDerivedfaster than runtime type usage.Fast path
Sample
Results
Runtime type check + TypeInfo serializer
Sample
Results
Polymorphic
Sample
Results
Controllers
Summary:
JsonTypeInfonew apisGetTypeInfowill happen.Controllers
Sample
Results