-
Notifications
You must be signed in to change notification settings - Fork 10k
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.
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
Changing reflection-based json configuration #46303
Changing reflection-based json configuration #46303
Conversation
…n/reflection-json-options
src/Http/Routing/src/DependencyInjection/RoutingServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
This reverts commit 9e97941.
/benchmark |
…n/reflection-json-options
…n/reflection-json-options
…//github.com/brunolins16/aspnetcore into brunolins16/aot/json/reflection-json-options
…n/reflection-json-options
…n/reflection-json-options
…n/reflection-json-options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we undraft this? Has @eiriktsarpalis seen this?
|
||
if (markAsReadOnly) | ||
{ | ||
options.MakeReadOnly(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be simpler to just always mark as read-only? Every time it's false (everywhere but RDF), we're about to write anyway, correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer not set it everywhere and let the serializer call it when needed. If you want I can remove it from here and call MakeReadOnly
in RDF only.
Can you ensure that this doesn't regress size in the Native AOT benchmarks? |
@eerhardt I ran the Native AOT benchmarks (copied the command from the dashboard) and compared with a run using
Is this comparison enough? What else you usually do for comparison? |
@@ -51,36 +44,17 @@ public static class HttpRequestJsonExtensions | |||
/// <param name="options">The serializer options to use when deserializing the content.</param> | |||
/// <param name="cancellationToken">A <see cref="CancellationToken"/> used to cancel the operation.</param> | |||
/// <returns>The task object representing the asynchronous operation.</returns> | |||
[RequiresUnreferencedCode(RequiresUnreferencedCodeMessage)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recall @eerhardt mentioning that public ASP.NET APIs should preserve RUC/RDC annotations even if any reflection code is hidden behind a feature flag. Presumably library authors would still need to be warned if calling into these APIs.
This dotnet/runtime#83844 will fix everything 😂 |
Hi @brunolins16. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
Contributes #46490
Benchmark results
Summary
No negative impact detected.
1 - Request Delegate
Sample
Results
2 - Http Results
Sample
Results
3 - Type return
Sample
Results
4 - MVC Json
Sample
Results