Skip to content
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

[AOT] Add expression free request filter pipeline for RequestDelegate #46020

Merged
merged 9 commits into from
Jan 13, 2023

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Jan 11, 2023

Fixes #46014

The goal here is to remove System.Linq.Expressions dependency from mapping RequestDelegate endpoints (i.e. basic, non-minimal API endpoints). This allows apps that map basic endpoints to avoid warnings from expressions, and reduce publish size by trimming away expressions assembly.

Changes:

  • Add InferMetadataFunc and CreateHandlerRequestDelegateFunc to RouteEntry.
  • RequestDelegate now registers its own func for building request filter factory pipeline. It doesn't use System.Linq.Expressions.
  • There is some messiness in sharing code because mapping RequestDelegate routes lives in Microsoft.AspNetCore.Routing. All of RequestDelegateFactory lives in Microsoft.AspNetCore.Http.Extensions. I tried to share code using source file linking, but a cleaner solution would be to add a public RequestDelegateFactory.Create(RequestDelegate, ...) overload that Microsoft.AspNetCore.Routing can call. Thoughts? Edit: API proposal - [API] RequestDelegateFactory.Create RequestDelegate overload #46024

@halter73 @davidfowl This is a start to the changes you discussed making. The next step after this PR is to make public API to configure InferMetadataFunc and CreateHandlerRequestDelegateFunc. Then source generated code can inject its own logic.

@JamesNK JamesNK added the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Jan 11, 2023
@JamesNK JamesNK changed the title Add explicit request filter path for RequestDelegate [AOT] Add explicit request filter path for RequestDelegate Jan 11, 2023
@JamesNK JamesNK requested review from eerhardt and davidfowl January 11, 2023 08:15
@JamesNK JamesNK changed the title [AOT] Add explicit request filter path for RequestDelegate [AOT] Add expression free request filter pipeline for RequestDelegate Jan 11, 2023
@JamesNK JamesNK self-assigned this Jan 12, 2023
@JamesNK JamesNK force-pushed the jamesnk/requestdelegate-filter branch from 5a9c74b to 690b124 Compare January 12, 2023 04:22
@JamesNK
Copy link
Member Author

JamesNK commented Jan 12, 2023

Numbers!

  • Before: 17316 KB
  • After: (referencing EmptyHttpResult): 16052 KB
  • After (not referencing EmptyHttpResult): 15316 KB

Test app:

var builder = WebApplication.CreateBuilder(args);
var app = builder.Build();
app.MapGet("/", (HttpContext c) => Task.CompletedTask);
app.Run();

Good news is System.Linq.Expressions warnings are gone, and there is precisely a 2 MB reduction in published Windows AOT size 🚀

There are two after results. I noticed that the hack to get the empty HTTP result brought in a lot of Microsoft.AspNet.Core.Http.Results. The difference between it being used or not is a large publish size increase and a lot of warnings from the results assembly. I'm not sure why Type.GetType("Microsoft.AspNetCore.Http.HttpResults.EmptyHttpResult, Microsoft.AspNetCore.Http.Results") appears to bring all result types from that assembly 🤷‍♂️

A simple fix could be to move EmptyHttpResult to Microsoft.AspNetCore.Http.Abstractions and type forward from Http.Results. Then Microsoft.AspNetCore.Routing an easily reference it. IMO it is common for an "empty" result to be part of the abstraction.

@davidfowl
Copy link
Member

I think we should move empty result down as well

@DamianEdwards
Copy link
Member

@JamesNK typo in the numbers? Should those have a decimal point in them somewhere or just be bytes instead if MB?

@JamesNK
Copy link
Member Author

JamesNK commented Jan 12, 2023

Ah, yeah, KB

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I'm definitely no expert in this area.

MethodInfo = requestDelegate.Method,
ApplicationServices = options.EndpointBuilder.ApplicationServices
};
var jsonTypeInfo = (JsonTypeInfo<object>)jsonSerializerOptions.GetReadOnlyTypeInfo(typeof(object));
Copy link
Member

Choose a reason for hiding this comment

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

How does this work with source generation, if we always get the JsonTypeInfo for object?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

This is weird, but OK I guess.

Copy link
Member

Choose a reason for hiding this comment

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

Very weird.

@JamesNK JamesNK merged commit d27c95d into main Jan 13, 2023
@JamesNK JamesNK deleted the jamesnk/requestdelegate-filter branch January 13, 2023 01:10
@ghost ghost added this to the 8.0-preview1 milestone Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[AOT] Remove usage of System.Linq.Expressions
5 participants