Skip to content

Add support for RouteHandlerInvocationContext<> overloads #41406

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

Merged
merged 6 commits into from
May 4, 2022

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Apr 27, 2022

Closes #40514.

  • Add T4 generator for RouteHandlerInvocationContext<> overloads of various arity (supports up to 10 arguments)
  • Update code generation to use new generic overloads when the number of handler arguments is < 10
  • Add GetParameter<T>(int index) API to RouteHandlerInvocationContext
  • Make HttpContext and Parameters properties on RouteHandlerInvocationContext virtual

API Changes

- public sealed class RouteHandlerInvocationContext
+ public class RouteHandlerInvocationContext
{
-  public HttpContext HttpContext { get; }
+ public virtual HttpContext HttpContext { get; }

- public List<object?> Parameters { get; }
+ public virtual List<object?> Parameters { get; }

+ public virtual T GetParameter<T>(int index) { }
}

Perf Impact

app.MapGet("/print/{name}/{age}", (string name, int age) => $"Hello, {name}! You're {age} years old.")
    .AddFilter(async (context, next) =>
    {
        var name = context.Parameters[0];
        var age = context.Parameters[1];
        return await next(context);
    });
> 1..1000 | % { curl http://localhost:5000/print/foo/27 }

Before:

Type Allocations
 - Microsoft.AspNetCore.Routing.Matching.CandidateState[1] 1,000
 - Microsoft.AspNetCore.Routing.RouteValueDictionary 1,000
 - Microsoft.AspNetCore.Http.RouteHandlerInvocationContext 1,000

Allocations for System.Object[] in non-generic RHIC

Type Allocations
 + System.Object[] 1,692
Function Name Allocations Bytes Module Name
 + dynamicClass.lambda_method2(00007FF9875983D8, object, 00007FF986FB5068) 1,000 40,000 anonymously hosted dynamicmethods assembly

After:

Type Allocations
 - Program.<>c.<<<Main>$>b__0_1>d 1,000
 - Microsoft.AspNetCore.Http.RouteHandlerInvocationContext<System.String, System.Int32> 1,000
 - Microsoft.AspNetCore.Routing.Matching.CandidateState[1] 1,000
 - Microsoft.AspNetCore.Routing.RouteValueDictionary 1,000
Type Allocations
 + System.Object[] 690

@ghost ghost added the area-runtime label Apr 27, 2022
@captainsafia captainsafia added old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels and removed area-runtime labels Apr 27, 2022
@captainsafia captainsafia marked this pull request as ready for review April 27, 2022 22:18
@captainsafia captainsafia requested review from davidfowl and a team April 27, 2022 22:18
@davidfowl
Copy link
Member

Where are the object[] allocations coming from?

@captainsafia
Copy link
Member Author

captainsafia commented Apr 28, 2022

Where are the object[] allocations coming from?

The ~1000 allocations before the change are from the object's array initialized in the invocation pipeline when we do the boxing. The rest are more dispersed.

Function Name Allocations Bytes Module Name
 - dynamicClass.lambda_method2(00007FF9875983D8, object, 00007FF986FB5068) 1,000 40,000 anonymously hosted dynamicmethods assembly
 - Microsoft.Extensions.DependencyInjection.ServiceLookup.CallSiteRuntimeResolver.VisitConstructor(Microsoft.Extensions.DependencyInjection.ServiceLookup.ConstructorCallSite, Microsoft.Extensions.DependencyInjection.ServiceLookup.RuntimeResolverContext) 4 160 microsoft.extensions.dependencyinjection
 - System.Collections.Generic.EnumerableHelpers.ToArray<T>(System.Collections.Generic.IEnumerable<T>) 2 80 system.linq.il
 - System.Collections.Generic.List<T>.ToArray() 1 40 System.Private.CoreLib.il
 - Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.UseMiddleware.AnonymousMethod__0(Microsoft.AspNetCore.Http.RequestDelegate) 1 40 microsoft.aspnetcore.http.abstractions
 - Microsoft.AspNetCore.Certificates.Generation.CertificateManager+CertificateManagerEventSource.ListCertificatesStart(System.Security.Cryptography.X509Certificates.StoreLocation, System.Security.Cryptography.X509Certificates.StoreName) 1 40 microsoft.aspnetcore.server.kestrel.core
 - System.ParamsArray.cctor() 1 40 System.Private.CoreLib.il

public void ProhibitsActionsThatModifyListSize()
{
var context = new RouteHandlerInvocationContext<string, int, bool>(new DefaultHttpContext(), "This is a test", 42, false);
Assert.Throws<NotSupportedException>(() => context.Add("string"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this, I think we should change the type of RouteHandlerInvocationContext.Arguments to IReadOnlyList<object?>.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so.

The type we use for Arguments will be used in both the generic and non-generic overloads. For the non-generic overload, we have to use IList to support in-place modification of items in the list.

One option is that we could change the behavior of the implementation so that if you're using the non-generic variant (more than 10 arguments) you're not able to make modifications to parameters. For scenarios where you're using ten arguments or less, you'll be able to modify in-place using the custom setter in the generic overloads.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, nevermind. We cannot do this anyway. The IReadOnlyList implementation does not support a setter on this[index] so we would be able to use it for generic variants anways.

</None>
</ItemGroup>

<ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can include this in the global directory.props

cc @dougbu

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean notice a project contains a .tt file and add the <Service /> element @davidfowl❔ That may work but VS would rewrite it the next time someone opens the project IIRC.

We only have 3 projects containing this <Service /> after this PR. How big an issue is this❔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not an issue, I would just like to clean it up as we plan to code generate more things.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's up to you @davidfowl whether to centralize things in this PR. I recommend testing if the code in our root Directory.Build.props prevents VS rewriting the project to list the <Service/>. If "Save All" is fine after adding a new .tt file, you're good to go. Otherwise, the extra code will become redundant and something we'll end up fighting.

The specific approach would be something like

<ItemGroup>
  <_GeneratorSources Include="$(MSBuildProjectDirectory)\**\*.tt" />
</ItemGroup>
<ItemGroup>
  <Service Condition=" '@(_GeneratorSources->Count()) ' != '0' " Include="{508349b6-6b84-4df5-91f0-309beebad82d}" />
</ItemGroup>

@@ -0,0 +1,129 @@
<#@ template language="C#" #>
<#@ output extension=".cs" #>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @mhutch I'm trying to resurrect t4

@captainsafia
Copy link
Member Author

🆙 📅 : Can I get another look at this?

/// Provides a default implementation for wrapping the <see cref="HttpContext"/> and parameters
/// provided to a route handler.
/// </summary>
public sealed class DefaultRouteHandlerInvocationContext : RouteHandlerInvocationContext
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this name discussed/approved?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@captainsafia captainsafia merged commit ba37a08 into dotnet:main May 4, 2022
@ghost ghost added this to the 7.0-preview5 milestone May 4, 2022
@davidfowl davidfowl added the Perf label Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Perf
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update RouteHandlerFilterContext.Parameters API
6 participants