Skip to content

Do not emit unused code with Request Delegate Generator #48381

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

Closed
1 task done
martincostello opened this issue May 23, 2023 · 6 comments · Fixed by #48426
Closed
1 task done

Do not emit unused code with Request Delegate Generator #48381

martincostello opened this issue May 23, 2023 · 6 comments · Fixed by #48426
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-rdg help wanted Up for grabs. We would accept a PR to help resolve this issue NativeAOT
Milestone

Comments

@martincostello
Copy link
Member

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

While looking at the code for RequestDelegateGeneratorSources to see if I could see the source of the bug for #48379 I noticed that private static constants are emitted for all HTTP methods, whether or not they are actually used:

private static readonly string[] GetVerb = new[] { global::Microsoft.AspNetCore.Http.HttpMethods.Get };
private static readonly string[] PostVerb = new[] { global::Microsoft.AspNetCore.Http.HttpMethods.Post };
private static readonly string[] PutVerb = new[] { global::Microsoft.AspNetCore.Http.HttpMethods.Put };
private static readonly string[] DeleteVerb = new[] { global::Microsoft.AspNetCore.Http.HttpMethods.Delete };
private static readonly string[] PatchVerb = new[] { global::Microsoft.AspNetCore.Http.HttpMethods.Patch };

In the following generated code, only GetVerb and PostVerb are used:

[System.CodeDom.Compiler.GeneratedCodeAttribute("Microsoft.AspNetCore.Http.RequestDelegateGenerator, Version=8.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60", "8.0.0.0")]
internal static class GenerateRouteBuilderEndpoints
{
    private static readonly string[] GetVerb = new[] { global::Microsoft.AspNetCore.Http.HttpMethods.Get };
    private static readonly string[] PostVerb = new[] { global::Microsoft.AspNetCore.Http.HttpMethods.Post };
    private static readonly string[] PutVerb = new[]  { global::Microsoft.AspNetCore.Http.HttpMethods.Put };
    private static readonly string[] DeleteVerb = new[] { global::Microsoft.AspNetCore.Http.HttpMethods.Delete };
    private static readonly string[] PatchVerb = new[] { global::Microsoft.AspNetCore.Http.HttpMethods.Patch };

    internal static global::Microsoft.AspNetCore.Builder.RouteHandlerBuilder MapGet(
        this global::Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints,
        [global::System.Diagnostics.CodeAnalysis.StringSyntax("Route")] string pattern,
        global::System.Func<global::System.Collections.Generic.IEnumerable<global::MartinCostello.AdventOfCode.PuzzleAttribute>, global::Microsoft.AspNetCore.Http.IResult> handler,
        [global::System.Runtime.CompilerServices.CallerFilePath] string filePath = "",
        [global::System.Runtime.CompilerServices.CallerLineNumber]int lineNumber = 0)
    {
        return global::Microsoft.AspNetCore.Http.Generated.GeneratedRouteBuilderExtensionsCore.MapCore(
            endpoints,
            pattern,
            handler,
            GetVerb,
            filePath,
            lineNumber);
    }
    internal static global::Microsoft.AspNetCore.Builder.RouteHandlerBuilder MapPost(
        this global::Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints,
        [global::System.Diagnostics.CodeAnalysis.StringSyntax("Route")] string pattern,
        global::System.Func<global::System.Int32, global::System.Int32, global::Microsoft.AspNetCore.Http.HttpRequest, global::Microsoft.AspNetCore.Http.IFormFile?, global::MartinCostello.AdventOfCode.PuzzleFactory, global::Microsoft.Extensions.Logging.ILogger<global::MartinCostello.AdventOfCode.Puzzle>, global::System.Threading.CancellationToken, global::System.Threading.Tasks.Task<global::Microsoft.AspNetCore.Http.IResult>> handler,
        [global::System.Runtime.CompilerServices.CallerFilePath] string filePath = "",
        [global::System.Runtime.CompilerServices.CallerLineNumber]int lineNumber = 0)
    {
        return global::Microsoft.AspNetCore.Http.Generated.GeneratedRouteBuilderExtensionsCore.MapCore(
            endpoints,
            pattern,
            handler,
            PostVerb,
            filePath,
            lineNumber);
    }
    internal static global::Microsoft.AspNetCore.Builder.RouteHandlerBuilder MapGet(
        this global::Microsoft.AspNetCore.Routing.IEndpointRouteBuilder endpoints,
        [global::System.Diagnostics.CodeAnalysis.StringSyntax("Route")] string pattern,
        global::System.Func<global::Microsoft.AspNetCore.Http.IResult> handler,
        [global::System.Runtime.CompilerServices.CallerFilePath] string filePath = "",
        [global::System.Runtime.CompilerServices.CallerLineNumber]int lineNumber = 0)
    {
        return global::Microsoft.AspNetCore.Http.Generated.GeneratedRouteBuilderExtensionsCore.MapCore(
            endpoints,
            pattern,
            handler,
            GetVerb,
            filePath,
            lineNumber);
    }
}

The memory and IL size would be ever so slightly smaller if the fields weren't emitted (assuming the compiler isn't removing them anyway). This is of course a micro-optimisation, so feel free to close this suggestion if the complexity of the generator code isn't worth changing it to detect which of the arrays are actually needed.

Describe the solution you'd like

The source generator only emits the string arrays that are needed by the endpoints used in the compilation.

Additional context

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label May 23, 2023
@mitchdenny mitchdenny added NativeAOT feature-rdg area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels May 23, 2023
@mitchdenny
Copy link
Member

It is a complexity trade off but we've definitely got logic already where we omit certain types entirely based on the endpoints they are defined.

@mitchdenny mitchdenny added this to the Backlog milestone May 24, 2023
@ghost
Copy link

ghost commented May 24, 2023

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

@danmoseley
Copy link
Member

@mitchdenny is this the sort of issue where we'd take a community PR? I'm hoping we can apply the 'help wanted' label whenever we'd take a community PR and believe we would have time to review and accept one (which of course is not always the case, eg., if something would likely need a lot of collaboration)

@mitchdenny
Copy link
Member

I think we would absolutely take a PR on this.

@mitchdenny mitchdenny added the help wanted Up for grabs. We would accept a PR to help resolve this issue label May 25, 2023
@martincostello
Copy link
Member Author

I'll see if I can come up with a PR to address this that doesn't make things too complicated.

@martincostello
Copy link
Member Author

Proposed fix in #48426.

martincostello added a commit to martincostello/aspnetcore that referenced this issue Jul 7, 2023
Do not emit unused private fields for HTTP verbs that are not used by any of the user-code endpoints.
Resolves dotnet#48381.
captainsafia pushed a commit that referenced this issue Jul 10, 2023
* Do not emit unused fields in RDG source

Do not emit unused private fields for HTTP verbs that are not used by any of the user-code endpoints.
Resolves #48381.

* Move verbs to incremental pipeline

Use another incremental pipeline to produce the HTTP verbs.

* Add custom comparer

Add custom comparer for endpoints by their HTTP method.
@ghost ghost locked as resolved and limited conversation to collaborators Aug 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc feature-rdg help wanted Up for grabs. We would accept a PR to help resolve this issue NativeAOT
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants
@mitchdenny @martincostello @danmoseley and others