Avoid allocating FormFeature in HttpRequest.HasFormContentType #50830

captainsafia · 2023-09-20T00:43:44Z

public class FormFeatureBenchmark
{
    private HttpRequest _request;

    [IterationSetup]
    public void Setup()
    {
        _request = new DefaultHttpRequest(new DefaultHttpContext());
    }

    [Benchmark]
    public void Get_HttpRequestHasFormContentType()
    {
        _ = _request.HasFormContentType;
    }
}

Baseline

Method	Mean	Error	StdDev	Median	Op/s	Gen 0	Gen 1	Gen 2	Allocated
Get_HttpRequestHasFormContentType	10.69 us	1.879 us	5.390 us	9.208 us	93,577.6	-	-	-	776 B

With Changeset

Method	Mean	Error	StdDev	Op/s	Gen 0	Gen 1	Gen 2	Allocated
Get_HttpRequestHasFormContentType	8.959 us	2.259 us	6.445 us	111,616.9	-	-	-	672 B

JamesNK · 2023-09-20T04:34:14Z

src/Http/Http/src/Features/FormFeature.cs

            {
                return true;
            }

-            var contentType = ContentType;
-            return HasApplicationFormContentType(contentType) || HasMultipartFormContentType(contentType);
+            return HttpExtensions.HasApplicationFormContentType(_request.ContentType) || HttpExtensions.HasMultipartFormContentType(_request.ContentType);


This is done a couple of times.

Add a HasFormContentType(...) to HttpExtensions that calls these methods for you? Eliminates the chance of logic going out of sync.

JamesNK · 2023-09-20T04:35:11Z

src/Http/Http/src/Features/FormFeature.cs

            {
-                var encoding = FilterEncoding(contentType.Encoding);
+                var encoding = FilterEncoding(ContentType!.Encoding);


nit: I don't like that the ContentType property allocates a MediaTypeHeaderValue. IMO it should be a method.

The type is more reliable, methods just re-invent the parsers.

JamesNK · 2023-09-20T04:35:53Z

src/Http/Http/src/Internal/DefaultHttpRequest.cs

-    {
-        get { return FormFeature.HasFormContentType; }
-    }
+    public override bool HasFormContentType => HttpExtensions.HasApplicationFormContentType(ContentType) || HttpExtensions.HasMultipartFormContentType(ContentType);


Use new HasFormContentType(...) method here.

This is a behavior change mentioned here: #50016 (comment)
Before if the FormFeature.Form was set manually this would also return true independent of the headers.
Now it would return false because only the headers are checked.
DefaultHttpRequest.HasFormContentType needs to be exactly the same as FormFeature.HasFormContentType for backward compatibility.

I guess this should also check if the form is already set e.g.

_features.Cache.Form?._form is not null || ...

Now it would return false because only the headers are checked.
DefaultHttpRequest.HasFormContentType needs to be exactly the same as FormFeature.HasFormContentType for backward compatibility.

Yeah, this is a total drag. I almost considered a breaking change here. However, it seems like there's a few components in MVC's model binding that rely on the behavior of form set = form content type is true. At best, here we can avoid constructing a new FormFeature if it has already been set.

ghost · 2023-09-28T03:00:32Z

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

Tratcher · 2023-09-28T15:36:38Z

src/Shared/HttpExtensions.cs

+    internal static bool HasApplicationFormContentType([NotNullWhen(true)] string? contentType)
+    {
+        // Content-Type: application/x-www-form-urlencoded; charset=utf-8
+        return contentType != null && contentType.Contains(UrlEncodedFormContentType, StringComparison.OrdinalIgnoreCase);


This is less precise / robust than the prior implementation. It's ok in the normal scenario, but it can cause strange random bugs for other inputs. E.g. this now matches "web-application/x-www-form-urlencoded-foo".

We have tools that can get the same precision as before without allocating:
https://github.com/dotnet/aspnetcore/blob/main/src/Shared/MediaType/ReadOnlyMediaTypeHeaderValue.cs

We have tools that can get the same precision as before without allocating:

Ah, perfect! This is exactly what I was looking for.

Tratcher · 2023-09-28T15:45:38Z

src/Http/Http/src/Internal/DefaultHttpRequest.cs

-        get { return FormFeature.HasFormContentType; }
-    }
+    public override bool HasFormContentType => HttpExtensions.HasAnyFormContentType(ContentType)
+        || _features.Fetch(ref _features.Cache.Form, _features.Collection, collection => collection.Get<IFormFeature>()) != null;


Isn't this just checking for the presence of the form feature? If present, it should check the form feature's HasFormContentType.

The current form feature should also take precedence if it exists.

Avoid allocating FormFeature in HttpRequest.HasFormContentType

76ed8e0

captainsafia requested review from Tratcher, BrennanConroy and halter73 as code owners September 20, 2023 00:43

ghost added the area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions label Sep 20, 2023

JamesNK approved these changes Sep 20, 2023

View reviewed changes

Address feedback and fix back-compat

5e33d72

build-analysis bot mentioned this pull request Sep 21, 2023

Microsoft.DotNet.Build.Tasks.Workloads.Tests.MsiTests.ItCanBuildAManifestMsi test failure in Arcade dotnet/arcade#14046

Closed

ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Sep 28, 2023

Tratcher requested changes Sep 28, 2023

View reviewed changes

Tratcher reviewed Sep 28, 2023

View reviewed changes

captainsafia closed this Nov 27, 2023

captainsafia deleted the safia/form-contenttype-check branch November 29, 2023 00:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Avoid allocating FormFeature in HttpRequest.HasFormContentType #50830

Avoid allocating FormFeature in HttpRequest.HasFormContentType #50830

Uh oh!

captainsafia commented Sep 20, 2023 •

edited

Loading

Uh oh!

JamesNK Sep 20, 2023 •

edited

Loading

Uh oh!

JamesNK Sep 20, 2023

Uh oh!

Tratcher Sep 28, 2023

Uh oh!

JamesNK Sep 20, 2023

Uh oh!

campersau Sep 20, 2023

Uh oh!

captainsafia Sep 20, 2023

Uh oh!

ghost commented Sep 28, 2023

Uh oh!

Tratcher Sep 28, 2023

Uh oh!

captainsafia Sep 28, 2023

Uh oh!

Tratcher Sep 28, 2023

Uh oh!

Uh oh!

Avoid allocating FormFeature in HttpRequest.HasFormContentType #50830

Avoid allocating FormFeature in HttpRequest.HasFormContentType #50830

Uh oh!

Conversation

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

Uh oh!

Uh oh!

JamesNK Sep 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost commented Sep 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

captainsafia commented Sep 20, 2023 •

edited

Loading

JamesNK Sep 20, 2023 •

edited

Loading