Set form options for form-based request types #50016

captainsafia · 2023-08-11T01:27:10Z

Part of #50008.

captainsafia · 2023-08-11T01:27:48Z

/benchmark plaintext aspnet-citrine-lin kestrel

captainsafia · 2023-08-11T01:36:05Z

/benchmarks

pr-benchmarks · 2023-08-11T01:46:30Z

Crank Pull Request Bot

/benchmark <benchmark[,...]> <profile[,...]> <component,[...]> <arguments>

Benchmarks:

plaintext: TechEmpower Plaintext Scenario - ASP.NET Platform implementation
json: TechEmpower JSON Scenario - ASP.NET Platform implementation
fortunes: TechEmpower Fortunes Scenario - ASP.NET Platform implementation
yarp: YARP - http-http with 10 bytes
mvcjsoninput2k: Sends 2Kb Json Body to an MVC controller

Profiles:

aspnet-perf-lin: INTEL/Linux 12 Cores
aspnet-perf-win: INTEL/Windows 12 Cores
aspnet-citrine-lin: INTEL/Linux 28 Cores
aspnet-citrine-win: INTEL/Windows 28 Cores
aspnet-citrine-ampere: Ampere/Linux 80 Cores
aspnet-citrine-amd: AMD/Linux 48 Cores

Components:

kestrel
mvc

Arguments: any additional arguments to pass through to crank, e.g. --variable name=value

captainsafia · 2023-08-11T02:16:44Z

/benchmark plaintext aspnet-citrine-lin kestrel

pr-benchmarks · 2023-08-11T02:26:57Z

Benchmark started for plaintext on aspnet-citrine-lin with kestrel. Logs: link

captainsafia · 2023-08-11T02:49:41Z

/benchmark plaintext_endpoint aspnet-citrine-lin kestrel

pr-benchmarks · 2023-08-11T03:06:23Z

Benchmark started for plaintext_endpoint on aspnet-citrine-lin with kestrel. Logs: link

BrennanConroy · 2023-08-11T04:46:14Z

@sebastienros Does the bot not write a comment with results anymore?

@captainsafia The "platform" benchmarks don't use Routing so won't show any difference here. The "mvcjsoninput2k" benchmark might show a small difference, ~615k on main -> ~625k a couple days ago; if I'm looking at the correct benchmark on the dashboard.

captainsafia · 2023-08-11T15:28:35Z

@captainsafia The "platform" benchmarks don't use Routing so won't show any difference here. The "mvcjsoninput2k" benchmark might show a small difference, ~615k on main -> ~625k a couple days ago; if I'm looking at the correct benchmark on the dashboard.

Yes, @sebastienros and I were discussing last night that there's currently no way for us to test PRs with changes in the routing layer. Using the mvcjsoninput2k build sounds like a good idea, but I still think it's worthwhile for us to get an "endpoint" component configured in the benchmark bot so we can profile changes to Microsoft.AspNetCore.Routing in PRs.

It's apparently straightforward to add support for a new component type so I'll try but in the meantime I'll also run them locally. Update: see aspnet/Benchmarks#1885.

davidfowl · 2023-08-11T15:35:33Z

src/Shared/HttpExtensions.cs

+        HttpMethods.IsPost(method) || HttpMethods.IsPut(method) || HttpMethods.IsPatch(method);
+
+    internal static bool IsValidContentTypeForForm(string? contentType) =>
+        MediaTypeHeaderValue.TryParse(contentType, out var mediaType) &&


This allocates, no?

Mmmm, yeah, and we would be paying the cost here for most requests (POST, PUT). I think I can cook up something better here.

src/Http/Routing/src/EndpointRoutingMiddleware.cs

captainsafia · 2023-08-11T17:56:30Z

application	plaintext_endpoint.base	plaintext_endpoint.pr
Max CPU Usage (%)	99	99	0.00%
Max Cores usage (%)	2,776	2,785	+0.32%
Max Working Set (MB)	513	86	-83.24%
Max Private Memory (MB)	1,271	593	-53.34%
Build Time (ms)	4,120	4,154	+0.83%
Start Time (ms)	179	178	-0.56%
Published Size (KB)	116,042	116,042	0.00%
Symbols Size (KB)	52	52	0.00%
.NET Core SDK Version	8.0.100-rc.1.23411.1	8.0.100-rc.1.23411.1

load	plaintext_endpoint.base	plaintext_endpoint.pr
Max CPU Usage (%)	41	46	+12.20%
Max Cores usage (%)	1,154	1,299	+12.56%
Max Working Set (MB)	48	48	0.00%
Max Private Memory (MB)	358	358	0.00%
Build Time (ms)	3,003	3,065	+2.06%
Start Time (ms)	0	0
Published Size (KB)	73,203	73,203	0.00%
Symbols Size (KB)	0	0
.NET Core SDK Version	7.0.400	7.0.400
First Request (ms)	129	123	-4.65%
Requests/sec	3,894,846	4,778,004	+22.68%
Requests	58,811,367	72,145,712	+22.67%
Mean latency (ms)	0.68	0.60	-12.40%
Max latency (ms)	37.83	44.75	+18.29%
Bad responses	0	0
Socket errors	0	0
Read throughput (MB/s)	605.45	742.74	+22.68%
Latency 50th (ms)	0.60	0.51	-16.36%
Latency 75th (ms)	0.88	0.73	-16.48%
Latency 90th (ms)	1.20	1.03	-14.17%
Latency 99th (ms)	0.00	0.00

captainsafia · 2023-08-11T17:57:06Z

/benchmark plaintext_endpoint aspnet-citrine-lin routing

pr-benchmarks · 2023-08-11T18:01:26Z

Benchmark started for plaintext_endpoint on aspnet-citrine-lin with routing. Logs: link

sebastienros · 2023-08-11T22:04:19Z

/benchmark plaintext_endpoint aspnet-citrine-lin routing

pr-benchmarks · 2023-08-11T22:05:30Z

Benchmark started for plaintext_endpoint on aspnet-citrine-lin with routing. Logs: link

sebastienros · 2023-08-11T22:15:38Z

/benchmark plaintext_endpoint aspnet-citrine-lin routing

pr-benchmarks · 2023-08-11T22:17:53Z

Benchmark started for plaintext_endpoint on aspnet-citrine-lin with routing. Logs: link

src/Shared/HttpExtensions.cs

captainsafia · 2023-08-14T14:12:20Z

/benchmark plaintext_endpoint aspnet-citrine-lin routing

pr-benchmarks · 2023-08-14T14:20:45Z

Benchmark started for plaintext_endpoint on aspnet-citrine-lin with routing. Logs: link

pr-benchmarks · 2023-08-14T14:28:56Z

plaintext_endpoint - aspnet-citrine-lin

application	plaintext_endpoint.base	plaintext_endpoint.pr
Max CPU Usage (%)	98	100	+2.04%
Max Cores usage (%)	2,752	2,790	+1.38%
Max Working Set (MB)	510	86	-83.14%
Max Private Memory (MB)	1,268	594	-53.15%
Build Time (ms)	5,181	4,423	-14.63%
Start Time (ms)	181	181	0.00%
Published Size (KB)	116,077	116,077	0.00%
Symbols Size (KB)	52	52	0.00%
.NET Core SDK Version	8.0.100-rc.1.23414.1	8.0.100-rc.1.23414.1

load	plaintext_endpoint.base	plaintext_endpoint.pr
Max CPU Usage (%)	41	45	+9.76%
Max Cores usage (%)	1,135	1,262	+11.19%
Max Working Set (MB)	48	48	0.00%
Max Private Memory (MB)	358	358	0.00%
Build Time (ms)	3,356	3,108	-7.39%
Start Time (ms)	0	0
Published Size (KB)	73,203	73,203	0.00%
Symbols Size (KB)	0	0
.NET Core SDK Version	7.0.400	7.0.400
First Request (ms)	125	128	+2.40%
Requests/sec	3,985,611	4,804,563	+20.55%
Requests	60,182,312	72,548,939	+20.55%
Mean latency (ms)	0.69	0.58	-15.38%
Max latency (ms)	33.21	53.23	+60.28%
Bad responses	0	0
Socket errors	0	0
Read throughput (MB/s)	501.73	604.82	+20.55%
Latency 50th (ms)	0.60	0.51	-15.61%
Latency 75th (ms)	0.88	0.74	-15.80%
Latency 90th (ms)	1.24	1.08	-12.90%
Latency 99th (ms)	0.00	0.00

captainsafia · 2023-08-14T16:56:04Z

Bump for reviews!

sebastienros

Perf numbers are back to before the regression

davidfowl · 2023-08-14T18:00:56Z

Not part of this PR but we need to understand the overhead of requests that POST requests that are not form and POST requests that are forms.

captainsafia · 2023-08-14T18:03:37Z

Not part of this PR but we need to understand the overhead of requests that POST requests that are not form and POST requests that are forms.

I'll take a stab at adding benchmarks in-repo for this.

sebastienros · 2023-08-14T18:05:21Z

We should probably track the perf of simple POST requests to detect regressions in the code that is completely excluded in GETs.

Set form options for form-based request types

791eb18

captainsafia requested a review from javiercn as a code owner August 11, 2023 01:27

ghost added the needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically label Aug 11, 2023

captainsafia added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares and removed needs-area-label Used by the dotnet-issue-labeler to label those issues which couldn't be triaged automatically labels Aug 11, 2023

davidfowl reviewed Aug 11, 2023

View reviewed changes

src/Http/Routing/src/EndpointRoutingMiddleware.cs Show resolved Hide resolved

Remove MediaTypeHeaderValue

2e6e1d5

martincostello reviewed Aug 11, 2023

View reviewed changes

src/Shared/HttpExtensions.cs Outdated Show resolved Hide resolved

Tweak

8cdd7ba

captainsafia requested review from BrennanConroy, davidfowl and sebastienros August 14, 2023 16:56

sebastienros approved these changes Aug 14, 2023

View reviewed changes

captainsafia merged commit fbe90bb into main Aug 14, 2023

captainsafia deleted the safia/formmapping-fix branch August 14, 2023 18:04

ghost added this to the 8.0-rc1 milestone Aug 14, 2023

captainsafia mentioned this pull request Aug 14, 2023

Update how IFormOptionsMetadata maps to FormOptions #50062

Closed

BrennanConroy mentioned this pull request Aug 14, 2023

Investigate performance of Antiforgery #50065

Open

4 tasks

campersau mentioned this pull request Sep 20, 2023

Avoid allocating FormFeature in HttpRequest.HasFormContentType #50830

Closed

Set form options for form-based request types #50016

Set form options for form-based request types #50016

Uh oh!

Conversation

captainsafia commented Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

captainsafia commented Aug 11, 2023

Uh oh!

captainsafia commented Aug 11, 2023

Uh oh!

pr-benchmarks bot commented Aug 11, 2023

Uh oh!

captainsafia commented Aug 11, 2023

Uh oh!

pr-benchmarks bot commented Aug 11, 2023

Uh oh!

captainsafia commented Aug 11, 2023

Uh oh!

pr-benchmarks bot commented Aug 11, 2023

Uh oh!

BrennanConroy commented Aug 11, 2023

Uh oh!

captainsafia commented Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

davidfowl Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

captainsafia Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

captainsafia commented Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

captainsafia commented Aug 11, 2023

Uh oh!

pr-benchmarks bot commented Aug 11, 2023

Uh oh!

sebastienros commented Aug 11, 2023

Uh oh!

pr-benchmarks bot commented Aug 11, 2023

Uh oh!

sebastienros commented Aug 11, 2023

Uh oh!

pr-benchmarks bot commented Aug 11, 2023

Uh oh!

Uh oh!

captainsafia commented Aug 14, 2023

Uh oh!

pr-benchmarks bot commented Aug 14, 2023

Uh oh!

pr-benchmarks bot commented Aug 14, 2023

Uh oh!

captainsafia commented Aug 14, 2023

Uh oh!

sebastienros left a comment

Choose a reason for hiding this comment

Uh oh!

davidfowl commented Aug 14, 2023

Uh oh!

captainsafia commented Aug 14, 2023

Uh oh!

sebastienros commented Aug 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

captainsafia commented Aug 11, 2023 •

edited

Loading

captainsafia commented Aug 11, 2023 •

edited

Loading

captainsafia commented Aug 11, 2023 •

edited

Loading