Adding STJ Polymorphism to Result Types #46008

brunolins16 · 2023-01-10T19:13:20Z

Fixes #44852
This PR enables support System.Text.Json polymorphism configuration (eg.: JsonDerivedAttribute) by calling the S.T.J.GetTypeInfo APIs to check if the declared type is polymorphic safe (value types, sealed types or has json polymorphism options), if not, will fall back to use the JsonTypeInfo obtained from the runtime type.

Also, this PR introduces new [Typed]Results APIs that allow users to provide JsonTypeInfo and JsonSerializerContext when using a JsonHttpResult (API Approved: #46252)

Contributes #45527
This PR resolves the following warnings:

ILC : Trim analysis warning IL2026: Microsoft.AspNetCore.Http.HttpResultsHelper.WriteResultAsJsonAsync<T>(HttpContext,ILogger,!!0,String,JsonSerializerOptions): Using member 'Microsoft.AspNetCore.Http.HttpResponseJsonExtensions.WriteAsJsonAsync<T>(HttpResponse,T,JsonSerializerOptions,String,CancellationToken)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. JSON serialization and deserialization might require types that cannot be statically analyzed. Use the overload that takes a JsonTypeInfo or JsonSerializerContext, or make sure all of the required types are preserved. [C:\Users\brolivei\source\repos\SampleTrim\SampleTrim.csproj]
ILC : AOT analysis warning IL3050: Microsoft.AspNetCore.Http.HttpResultsHelper.WriteResultAsJsonAsync<T>(HttpContext,ILogger,!!0,String,JsonSerializerOptions): Using member 'Microsoft.AspNetCore.Http.HttpResponseJsonExtensions.WriteAsJsonAsync<T>(HttpResponse,T,JsonSerializerOptions,String,CancellationToken)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. JSON serialization and deserialization might require types that cannot be statically analyzed and need runtime code generation. Use the overload that takes a JsonTypeInfo or JsonSerializerContext for native AOT applications. [C:\Users\brolivei\source\repos\SampleTrim\SampleTrim.csproj]

Benchmark results

Summary

Slightly reduced the memory allocation -> ~ 2%
Consistently slower (RPS) -> ~ 1% 😢 Related: Manually resolving JsonTypeInfo + Serialize methods slower than call Serializer directly runtime#80750
Scenario 3 (2 GetTypeInfo calls) does not show huge performance regressions: ~ 0.3%

1 - Fast path

Results

Sample

    app.MapGet("/ok", () => Results.Ok(new { message = "Hello, World!" }));

Results

application	main	PR
CPU Usage (%)	95	98	+3.16%
Cores usage (%)	1,140	1,180	+3.51%
Working Set (MB)	196	197	+0.51%
Private Memory (MB)	567	472	-16.75%
Build Time (ms)	3,354	1,421	-57.63%
Start Time (ms)	97	155	+59.79%
Published Size (KB)	90,388	90,400	+0.01%
Symbols Size (KB)	20	20	0.00%
.NET Core SDK Version	8.0.100-alpha.1.23063.11	8.0.100-alpha.1.23063.11
Max CPU Usage (%)	96	99	+2.58%
Max Working Set (MB)	202	208	+2.79%
Max GC Heap Size (MB)	95	94	-1.84%
Size of committed memory by the GC (MB)	127	132	+4.21%
Max Number of Gen 0 GCs / sec	4.00	4.00	0.00%
Max Number of Gen 1 GCs / sec	1.00	1.00	0.00%
Max Number of Gen 2 GCs / sec	1.00	1.00	0.00%
Max Time in GC (%)	1.00	0.00
Max Gen 0 Size (B)	584	584	0.00%
Max Gen 1 Size (B)	3,297,968	2,720,896	-17.50%
Max Gen 2 Size (B)	3,427,872	3,472,680	+1.31%
Max LOH Size (B)	85,512	85,512	0.00%
Max POH Size (B)	1,187,064	1,187,064	0.00%
Total Allocated Bytes	8,622,616,424	8,370,950,752	-2.92%
Max GC Heap Fragmentation	1	0	-40.68%
# of Assemblies Loaded	89	93	+4.49%
Max Exceptions (#/s)	361	339	-6.09%
Max Lock Contention (#/s)	661	604	-8.62%
Max ThreadPool Threads Count	22	23	+4.55%
Max ThreadPool Queue Length	144	171	+18.75%
Max ThreadPool Items (#/s)	675,591	675,196	-0.06%
Max Active Timers	1	1	0.00%
IL Jitted (B)	256,181	390,952	+52.61%
Methods Jitted	2,731	5,473	+100.40%

load	main	PR
CPU Usage (%)	93	93	0.00%
Cores usage (%)	1,120	1,117	-0.27%
Working Set (MB)	120	119	-0.83%
Private Memory (MB)	358	358	0.00%
Start Time (ms)	0	0
First Request (ms)	151	184	+21.85%
Requests/sec	568,826	567,100	-0.30%
Requests	8,588,707	8,561,175	-0.32%
Mean latency (ms)	0.48	0.49	+2.77%
Max latency (ms)	28.52	31.76	+11.36%
Bad responses	0	0
Socket errors	0	0
Read throughput (MB/s)	117.72	117.36	-0.31%
Latency 50th (ms)	0.40	0.40	-0.25%
Latency 75th (ms)	0.46	0.46	+0.22%
Latency 90th (ms)	0.56	0.56	+0.89%
Latency 99th (ms)	2.02	2.88	+42.57%

2 - Runtime type check (1 `GetTypeInfo` calls)

Sample

  public class Message
  {
      public string Text { get; set; }
  }

    app.MapGet("/ok", () => Results.Ok(new Message { Text = "Hello, World!" }));

Results

application	main	PR
CPU Usage (%)	98	99	+1.02%
Cores usage (%)	1,181	1,183	+0.17%
Working Set (MB)	192	196	+2.08%
Private Memory (MB)	580	575	-0.86%
Build Time (ms)	1,422	1,416	-0.42%
Start Time (ms)	98	97	-1.02%
Published Size (KB)	90,388	90,388	0.00%
Symbols Size (KB)	20	20	0.00%
.NET Core SDK Version	8.0.100-alpha.1.23067.2	8.0.100-alpha.1.23067.2
Max CPU Usage (%)	96	98	+1.87%
Max Working Set (MB)	201	207	+2.83%
Max GC Heap Size (MB)	94	93	-1.79%
Size of committed memory by the GC (MB)	125	133	+6.21%
Max Number of Gen 0 GCs / sec	4.00	4.00	0.00%
Max Number of Gen 1 GCs / sec	1.00	1.00	0.00%
Max Number of Gen 2 GCs / sec	1.00	1.00	0.00%
Max Time in GC (%)	1.00	0.00
Max Gen 0 Size (B)	584	584	0.00%
Max Gen 1 Size (B)	3,529,776	2,973,800	-15.75%
Max Gen 2 Size (B)	3,506,528	3,509,184	+0.08%
Max LOH Size (B)	85,512	183,872	+115.02%
Max POH Size (B)	1,187,064	1,187,064	0.00%
Total Allocated Bytes	8,629,069,992	8,256,430,648	-4.32%
Max GC Heap Fragmentation	1	0	-39.09%
# of Assemblies Loaded	89	89	0.00%
Max Exceptions (#/s)	342	338	-1.17%
Max Lock Contention (#/s)	650	593	-8.77%
Max ThreadPool Threads Count	24	23	-4.17%
Max ThreadPool Queue Length	138	204	+47.83%
Max ThreadPool Items (#/s)	680,406	674,336	-0.89%
Max Active Timers	1	1	0.00%
IL Jitted (B)	255,788	256,346	+0.22%
Methods Jitted	2,735	2,743	+0.29%

load	main	PR
CPU Usage (%)	94	93	-1.06%
Cores usage (%)	1,127	1,120	-0.62%
Working Set (MB)	120	120	0.00%
Private Memory (MB)	358	358	0.00%
Start Time (ms)	0	0
First Request (ms)	152	155	+1.97%
Requests/sec	572,212	566,318	-1.03%
Requests	8,640,202	8,551,477	-1.03%
Mean latency (ms)	0.48	0.48	+0.17%
Max latency (ms)	46.22	28.43	-38.49%
Bad responses	0	0
Socket errors	0	0
Read throughput (MB/s)	116.78	115.58	-1.03%
Latency 50th (ms)	0.39	0.40	+1.28%
Latency 75th (ms)	0.46	0.46	+1.54%
Latency 90th (ms)	0.55	0.56	+1.99%
Latency 99th (ms)	2.31	2.33	+0.87%

3 - Polymorphic with Runtime type check (2 `GetTypeInfo` calls)

Sample

    public class MessageBase
    {
        public string Text { get; set; }
    }
    
    public class Message : MessageBase
    {
    }

    app.MapGet("/ok", () => Results.Ok<MessageBase>(new Message { Text = "Hello, World!" }));

Results

application	main	PR
CPU Usage (%)	96	94	-2.08%
Cores usage (%)	1,146	1,134	-1.05%
Working Set (MB)	188	193	+2.66%
Private Memory (MB)	568	572	+0.70%
Build Time (ms)	1,482	2,329	+57.15%
Start Time (ms)	101	97	-3.96%
Published Size (KB)	90,389	90,389	0.00%
Symbols Size (KB)	20	20	0.00%
.NET Core SDK Version	8.0.100-alpha.1.23067.2	8.0.100-alpha.1.23067.2
Max CPU Usage (%)	98	95	-2.76%
Max Working Set (MB)	197	203	+3.07%
Max GC Heap Size (MB)	90	94	+5.14%
Size of committed memory by the GC (MB)	122	126	+3.52%
Max Number of Gen 0 GCs / sec	4.00	4.00	0.00%
Max Number of Gen 1 GCs / sec	1.00	1.00	0.00%
Max Number of Gen 2 GCs / sec	1.00	1.00	0.00%
Max Time in GC (%)	0.00	0.00
Max Gen 0 Size (B)	584	584	0.00%
Max Gen 1 Size (B)	3,324,536	3,299,616	-0.75%
Max Gen 2 Size (B)	3,528,288	3,482,944	-1.29%
Max LOH Size (B)	85,512	85,512	0.00%
Max POH Size (B)	1,187,064	1,187,064	0.00%
Total Allocated Bytes	8,731,022,216	8,345,579,104	-4.41%
Max GC Heap Fragmentation	1	1	+2.14%
# of Assemblies Loaded	89	89	0.00%
Max Exceptions (#/s)	342	328	-4.09%
Max Lock Contention (#/s)	660	613	-7.12%
Max ThreadPool Threads Count	23	23	0.00%
Max ThreadPool Queue Length	197	97	-50.76%
Max ThreadPool Items (#/s)	678,832	670,076	-1.29%
Max Active Timers	1	1	0.00%
IL Jitted (B)	256,022	258,246	+0.87%
Methods Jitted	2,735	2,772	+1.35%

load	main	PR
CPU Usage (%)	94	93	-1.06%
Cores usage (%)	1,128	1,121	-0.62%
Working Set (MB)	119	120	+0.84%
Private Memory (MB)	358	358	0.00%
Start Time (ms)	0	0
First Request (ms)	153	149	-2.61%
Requests/sec	573,460	565,594	-1.37%
Requests	8,659,317	8,540,528	-1.37%
Mean latency (ms)	0.48	0.47	-0.93%
Max latency (ms)	36.47	31.78	-12.86%
Bad responses	0	0
Socket errors	0	0
Read throughput (MB/s)	117.04	115.43	-1.38%
Latency 50th (ms)	0.39	0.40	+1.79%
Latency 75th (ms)	0.46	0.46	+1.53%
Latency 90th (ms)	0.56	0.56	+0.90%
Latency 99th (ms)	2.37	1.74	-26.58%

…44852-results

src/Http/Http.Results/src/HttpResultsWriter.cs

…44852-results

Removing the change from HttpResultsHelper to HttpResultsWriter to avoid polluting the git with not related changes and I will do it later.

…44852-results

JamesNK · 2023-01-27T04:50:20Z

This needs a rebase. My PR enables trimming/AOT analysis on this project. You may have new warnings.

…44852-results

src/Http/Http.Results/src/Results.cs

src/Http/Http.Results/src/HttpResultsHelper.cs

davidfowl · 2023-01-27T15:26:17Z

src/Http/Http.Results/src/JsonHttpResultOfT.cs

+    [RequiresDynamicCode(JsonHttpResultTrimmerWarning.SerializationRequiresDynamicCodeMessage)]
+    [RequiresUnreferencedCode(JsonHttpResultTrimmerWarning.SerializationUnreferencedCodeMessage)]


Remind me why this is unsafe after the other work we did to make the underlying JSON APIs that these call into safe?

This is unsafe because it is the scenario where the user provides the options, could be resolved by DI or not, so, we need to check and initialize for reflection.

src/Http/Http.Results/src/Results.cs

…44852-results

brunolins16 · 2023-02-01T00:20:32Z

@davidfowl after our conversation i tried to capture the feedback about the IsPolymorphicSafe method name and I came out with e39d168

brunolins16 · 2023-02-01T18:03:46Z

@davidfowl after our conversation i tried to capture the feedback about the IsPolymorphicSafe method name and I came out with e39d168

I'll take your silence as "OK" 😁😁😁😁 and mark it as auto-merge. We can follow-up with this in another PR if you want, ok?

src/Http/Http.Results/src/JsonHttpResultOfT.cs

brunolins16 added 5 commits January 9, 2023 11:29

Adding STJ Polymorphism to Result Types

2143f21

Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…

0c87d13

…44852-results

Renaming unittest

1e048d3

Adding unit tests

7485673

Adding more unit tests

af1eba8

ghost added the area-runtime label Jan 10, 2023

brunolins16 added area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-runtime labels Jan 10, 2023

eiriktsarpalis mentioned this pull request Jan 11, 2023

SystemTextJsonOutputFormatter calls the serializer with unexpected Type #44852

Closed

1 task

brunolins16 added 4 commits January 12, 2023 10:15

Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…

84e1c77

…44852-results

Setting DefaultTypeInfoResolver

4a63010

Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…

543c338

…44852-results

Removing ISTrimmable

cfbe2fd

davidfowl reviewed Jan 12, 2023

View reviewed changes

src/Http/Http.Results/src/HttpResultsWriter.cs Outdated Show resolved Hide resolved

brunolins16 added 3 commits January 12, 2023 15:24

Removing cache

8c10b8c

Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…

cabe9c5

…44852-results

Clean up

a4229b3

brunolins16 mentioned this pull request Jan 15, 2023

[AOT] Enable analysis and annotate Http.Results #46082

Merged

brunolins16 added 7 commits January 17, 2023 11:01

Avoiding multiple GetTypeInfo calls

2be89fc

Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…

c9ddbea

…44852-results

Fixing JsonResult

8c09b97

Clean up

52c3b9d

clean up

3d7a9f9

Adding Json apis proposal

891d909

Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…

1fb0405

…44852-results

brunolins16 marked this pull request as ready for review January 23, 2023 22:06

brunolins16 requested review from Tratcher, BrennanConroy and halter73 as code owners January 23, 2023 22:06

brunolins16 requested a review from a team January 23, 2023 22:06

brunolins16 added 2 commits January 25, 2023 10:51

Removing name change

e854667

Removing the change from HttpResultsHelper to HttpResultsWriter to avoid polluting the git with not related changes and I will do it later.

Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…

990498c

…44852-results

build-analysis bot mentioned this pull request Jan 25, 2023

Request failed "500 Internal Server Error" dotnet/arcade#11737

Open

5 tasks

Fixing bad merge

a2a972a

brunolins16 linked an issue Jan 26, 2023 that may be closed by this pull request

Adding Json aot/trimmer-safe overloads to [Typed]Results #46252

Closed

captainsafia linked an issue Jan 26, 2023 that may be closed by this pull request

Eliminate Trim/AOT Warnings related to System.Text.Json #45527

Closed

Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…

ae9c2a4

…44852-results

JamesNK approved these changes Jan 27, 2023

View reviewed changes

src/Http/Http.Results/src/Results.cs Outdated Show resolved Hide resolved

src/Http/Http.Results/src/Results.cs Outdated Show resolved Hide resolved

src/Http/Http.Results/src/Results.cs Outdated Show resolved Hide resolved

brunolins16 added 2 commits January 26, 2023 22:12

Fix build

c2768f4

PR review

610c7b1

davidfowl reviewed Jan 27, 2023

View reviewed changes

src/Http/Http.Results/src/HttpResultsHelper.cs Outdated Show resolved Hide resolved

davidfowl reviewed Jan 27, 2023

View reviewed changes

brunolins16 added 2 commits January 27, 2023 10:16

PR Feedback

1930364

Update for the approved API

3b54e1a

JamesNK reviewed Jan 30, 2023

View reviewed changes

src/Http/Http.Results/src/Results.cs Outdated Show resolved Hide resolved

brunolins16 and others added 4 commits January 30, 2023 16:08

PR review

205c6c6

Update TypedResultsTests.cs

fba0039

Changing IsPolymorphicSafe

e39d168

Merge remote-tracking branch 'upstream/main' into brunolins16/issues/…

baf9036

…44852-results

Fixing notnull annotation

9df633e

brunolins16 enabled auto-merge (squash) February 1, 2023 18:04

DamianEdwards reviewed Feb 1, 2023

View reviewed changes

src/Http/Http.Results/src/JsonHttpResultOfT.cs Outdated Show resolved Hide resolved

Remove blank line

a070575

brunolins16 merged commit 0d52a44 into dotnet:main Feb 1, 2023

brunolins16 deleted the brunolins16/issues/44852-results branch February 1, 2023 20:01

eerhardt mentioned this pull request Mar 20, 2023

Add source gen support for unspeakable types dotnet/runtime#82457

Closed

github-actions bot locked and limited conversation to collaborators Dec 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding STJ Polymorphism to Result Types #46008

Adding STJ Polymorphism to Result Types #46008

brunolins16 commented Jan 10, 2023 •

edited

Loading

JamesNK commented Jan 27, 2023

davidfowl Jan 27, 2023

brunolins16 Jan 27, 2023

brunolins16 commented Feb 1, 2023

brunolins16 commented Feb 1, 2023

		[RequiresDynamicCode(JsonHttpResultTrimmerWarning.SerializationRequiresDynamicCodeMessage)]
		[RequiresUnreferencedCode(JsonHttpResultTrimmerWarning.SerializationUnreferencedCodeMessage)]

Adding STJ Polymorphism to Result Types #46008

Adding STJ Polymorphism to Result Types #46008

Conversation

brunolins16 commented Jan 10, 2023 • edited Loading

Benchmark results

Summary

Results

Sample

Results

Sample

Results

Sample

Results

JamesNK commented Jan 27, 2023

davidfowl Jan 27, 2023

Choose a reason for hiding this comment

brunolins16 Jan 27, 2023

Choose a reason for hiding this comment

brunolins16 commented Feb 1, 2023

brunolins16 commented Feb 1, 2023

brunolins16 commented Jan 10, 2023 •

edited

Loading