Simplify and optimize async parameters in RequestDelegateFactory #42588

halter73 · 2022-07-06T01:17:47Z

Avoid object[] allocation for single BindAsync call
Remove redundant state and logic
Cleanup some names and nullability annotations
Split RequestDelegateFactory.Log class into its own RequestDelegateFactory.Log.cs file

- Avoid object[] for single BindAsync - Remove redundant state and logic - Cleanup some names and nullability annotations

JamesNK

No deep insight into the code but I noticed this 😄

JamesNK · 2022-07-06T01:19:37Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

-        var arguments = new Expression[factoryContext.ArgumentExpressions.Length + 1];
-        arguments[0] = HttpContextExpr;
-        factoryContext.ArgumentExpressions.CopyTo(arguments, 1);
+            for (int i = 0; i < methodParameters.Length; i++)


Suggested change

for (int i = 0; i < methodParameters.Length; i++)

for (var i = 0; i < methodParameters.Length; i++)

BrennanConroy · 2022-07-06T16:19:01Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

-            return Expression.New(constructor, arguments);
+            // For AOT platforms it's not possible to support the closed generic arguments that are based on the
+            // parameter arguments dynamically (for value types). In that case, fallback to boxing the argument list.
+            // new RouteHandlerInvocionContext(httpContext, (object)name_local, (object)int_local);


Suggested change

// new RouteHandlerInvocionContext(httpContext, (object)name_local, (object)int_local);

// new RouteHandlerInvocationContext(httpContext, (object)name_local, (object)int_local);

BrennanConroy · 2022-07-06T16:21:37Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

+        else
+        {
+            methodArgumentTypes = new Type[methodArguments.Length];
+            var boxedArgs = new Expression[methodArguments.Length];


super nit: this is only used in the if (!RuntimeFeature.IsDynamicCodeCompiled || !constructorType.IsGenericType) case, we could slightly restructure to avoid this startup cost

That’s what the code did before

Hence "super nit"

BrennanConroy · 2022-07-06T17:16:21Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

        }

-        // new RouteHandlerInvocationContext(httpContext, (object)name_local, (object)int_local);
-        return fallbackConstruction;
+        var constructor = constructorType.MakeGenericType(methodArgumentTypes).GetConstructors(BindingFlags.NonPublic | BindingFlags.Instance).Single();


Debug.Assert(constructor is not null)?

BrennanConroy · 2022-07-06T20:40:00Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

-                if (!successful)
+                var boundValues = new object?[count];
+
+                // Looping over arrays is faster


Comment was more for when we called .ToArray() on the binders collection, can remove

BrennanConroy · 2022-07-06T20:52:02Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

        {
-            return valueExpression;
+            return Expression.Condition(Expression.NotEqual(valueExpression, Expression.Default(parameter.ParameterType)),


Show code in a comment?

brunolins16 · 2022-07-06T21:23:32Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

-            args[i] = CreateArgument(parameters[i], factoryContext);
+            var parameter = parameters[i];
+
+            factoryContext.ParametersAndPropertiesAsParameters.Add(parameter);


It will change the order the metadata (for parameters from AsParameters) will be added, right? I don't know if that is a problem and, to be honest, it looks more correct with this change.

martincostello · 2022-07-08T13:46:52Z

As you're taking the big refactor-hammer to things as it is, I wonder if maybe it's also worth moving the other nested types into their own partial files too to get the main file down to under 2k LoC?

davidfowl · 2023-01-25T17:04:27Z

src/Http/Http.Extensions/src/RequestDelegateFactory.cs

@@ -466,24 +468,22 @@ private static Expression CreateRouteHandlerInvocationContextBase(FactoryContext
            _ => typeof(DefaultRouteHandlerInvocationContext)
        };

-        if (constructorType.IsGenericType)
+        if (!RuntimeFeature.IsDynamicCodeCompiled || !constructorType.IsGenericType)


I wonder if we should remove all of these checks now that we're going full source generator.

@halter73 Sounds like this needs to be updated after the introduction of RDG. @captainsafia might have thoughts?

captainsafia · 2023-04-24T17:39:46Z

@halter73 Thoughts on this PR? If I recall, it still needs a hearty rebase to get it in an up-to-date state.

There's also some goodness in here that is probably better to split up into a separate PR like:

Cleanup some names and nullability annotations
Split RequestDelegateFactory.Log class into its own RequestDelegateFactory.Log.cs file

halter73 · 2023-04-24T20:12:06Z

Thanks for the reminder. I'll work on rebasing this after I update my MapIdentityApi() PR (#47414) in response to API review.

Simplify and optimize async parameters in RequestDelegateFactory

2befe1c

- Avoid object[] for single BindAsync - Remove redundant state and logic - Cleanup some names and nullability annotations

halter73 requested review from davidfowl, captainsafia, BrennanConroy, brunolins16 and a team July 6, 2022 01:17

halter73 requested a review from Tratcher as a code owner July 6, 2022 01:17

ghost added the area-runtime label Jul 6, 2022

Remove outdated comment

0ca2144

JamesNK reviewed Jul 6, 2022

View reviewed changes

BrennanConroy reviewed Jul 6, 2022

View reviewed changes

brunolins16 reviewed Jul 6, 2022

View reviewed changes

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

davidfowl reviewed Jan 25, 2023

View reviewed changes

amcasey assigned captainsafia Apr 20, 2023

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023

wtgodbe removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 13, 2024

dotnet deleted a comment Feb 13, 2024

dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Feb 20, 2024

halter73 closed this Apr 16, 2024

dotnet-policy-service bot added this to the 9.0-preview4 milestone Apr 16, 2024

halter73 deleted the halter73/less-rdf-combos branch November 19, 2024 04:46

halter73 restored the halter73/less-rdf-combos branch November 19, 2024 04:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Simplify and optimize async parameters in RequestDelegateFactory #42588

Simplify and optimize async parameters in RequestDelegateFactory #42588

Uh oh!

halter73 commented Jul 6, 2022 •

edited

Loading

Uh oh!

JamesNK left a comment

Uh oh!

JamesNK Jul 6, 2022

Uh oh!

BrennanConroy Jul 6, 2022

Uh oh!

BrennanConroy Jul 6, 2022

Uh oh!

davidfowl Jul 6, 2022

Uh oh!

BrennanConroy Jul 6, 2022

Uh oh!

BrennanConroy Jul 6, 2022

Uh oh!

BrennanConroy Jul 6, 2022

Uh oh!

BrennanConroy Jul 6, 2022

Uh oh!

brunolins16 Jul 6, 2022 •

edited

Loading

Uh oh!

martincostello commented Jul 8, 2022

Uh oh!

davidfowl Jan 25, 2023

Uh oh!

amcasey Apr 15, 2023

Uh oh!

captainsafia commented Apr 24, 2023

Uh oh!

halter73 commented Apr 24, 2023

Uh oh!

Uh oh!

	for (int i = 0; i < methodParameters.Length; i++)
	for (var i = 0; i < methodParameters.Length; i++)

	// new RouteHandlerInvocionContext(httpContext, (object)name_local, (object)int_local);
	// new RouteHandlerInvocationContext(httpContext, (object)name_local, (object)int_local);

Simplify and optimize async parameters in RequestDelegateFactory #42588

Simplify and optimize async parameters in RequestDelegateFactory #42588

Uh oh!

Conversation

halter73 commented Jul 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JamesNK left a comment

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!

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!

brunolins16 Jul 6, 2022 • 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!

martincostello commented Jul 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

captainsafia commented Apr 24, 2023

Uh oh!

halter73 commented Apr 24, 2023

Uh oh!

Uh oh!

halter73 commented Jul 6, 2022 •

edited

Loading

brunolins16 Jul 6, 2022 •

edited

Loading