Added ControllerRequestDelegateFactory #27773

davidfowl · 2020-11-12T14:56:55Z

Today there's lots of indirection to figure out which invoker is responsible for executing a particular request based on the ActionContext. The IActionInvokerFactory calls through IActionInvokerProviders and an IActionInvoker is picked as a result of this. The default implementations based this decision on the type of action descriptor (though that's not required) which means its possible to avoid this per request indirection and select the invoker based on the descriptor. The ControllerRequestDelegateFactory creates a RequestDelegate for the endpoint don't go through indirection to find the ControllerActionInvoker.

API

public class MvcOptions
{
+   /// <summary>
+   /// Gets or sets the flag that determines if MVC should use action invoker extensibility. This will allow
+   /// custom <see cref="IActionInvokerFactory"/> and <see cref="IActionInvokerProvider"/> execute during the request pipeline.
+   /// </summary>
+   /// <remarks>This only applies when <see cref="EnableEndpointRouting"/> is true.</remarks>
+   public bool EnableActionInvokers { get; set; }
}

src/Mvc/Mvc.Core/src/DependencyInjection/MvcCoreServiceCollectionExtensions.cs

- Today there's lots of indirection to figure out which invoker is responsible for executing a particular request based on the `ActionContext`. The `IActionInvokerFactory` calls through `IActionInvokerProviders` and an `IActionInvoker` is picked as a result of this. The default implementations based this decision on the type of action descriptor (though that's not required) which means its possible to avoid this per request indirection and select the invoker based on the descriptor. The `ControllerRequestDelegateFactory` creates a `RequestDelegate` for the endpoint don't go through indirection to find the ControllerActionInvoker. PS: This code path breaks extensibility for IActionInvoker* types that want to take over invocation of existing ControllerActionDescriptor since it no longer calls them in the endpoint routing code path. This may warrant a compatibility switch

- This will allow people to turn off the request delegate factory if they were using that extensiblity point before.

ghost · 2020-11-13T12:04:10Z

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

rynowak · 2020-11-16T20:24:40Z

PS: This code path breaks extensibility for IActionInvoker* types that want to take over invocation of existing ControllerActionDescriptor since it no longer calls them in the endpoint routing code path. This may warrant a compatibility switch (this is why it's a draft)

FWIW - I'm not aware of anyone using this for extensiblity for anything real (along with filter providers). There were some projects using it back when our implementation of the invoker was public. Now that this requires some really complicated work, it's just not realistic for someone to extend.

rynowak · 2020-11-16T20:28:30Z

src/Mvc/Mvc.Core/src/Routing/ControllerRequestDelegateFactory.cs

+
+                    controllerContext.ModelState.MaxAllowedErrors = _maxModelValidationErrors;
+
+                    var (cacheEntry, filters) = _controllerActionInvokerCache.GetCachedResult(controllerContext);


Another thing to investigate. This is a cache lookup that hits a concurrent dictionary (keyed on ActionDescriptor). Since you're creating a request delegate per-action there's no need for additional caches and all that stuff can get closed over here.

rynowak · 2020-11-16T20:29:51Z

src/Mvc/Mvc.Core/src/Routing/ControllerRequestDelegateFactory.cs

+
+namespace Microsoft.AspNetCore.Mvc.Routing
+{
+    internal class ControllerRequestDelegateFactory : IRequestDelegateFactory


Have you tried implementing this yet for Pages? I suspect it's more complex

I have a stash for pages and it is more complex.

src/Mvc/Mvc.Core/src/MvcOptions.cs

Co-authored-by: Pranav K <[email protected]>

ghost · 2020-11-23T22:09:08Z

Hello @davidfowl!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

davidfowl · 2020-11-24T20:09:41Z

/azp run

azure-pipelines · 2020-11-24T20:10:00Z

Azure Pipelines successfully started running 2 pipeline(s).

halter73 · 2020-11-25T00:21:17Z

You said you were close to having an IRequestDelegateFactory implementation ready for Razor Pages. Can that be included in this PR?

halter73

After some offline discussion, I'm fine with looking at the Razor Page change in a separate PR.

halter73 · 2020-11-25T01:07:47Z

FWIW, here are the benchmark results for an early version of this PR hitting the following action using this crank config:

    public class Person
    {
        public string Name { get; set; }
    }

    [ApiController]
    public class EchoController : ControllerBase
    {
        [HttpPost("/EchoController")]
        public Person Post(Person person) => person;

### MapController (master)

`> crank --config .\echo.yml --scenario mapcontroller --profile aspnet-perf-lin --application.options.outputFiles C:\dev\dotnet\aspnetcore\artifacts\bin\Microsoft.AspNetCore.Mvc.Core\Release\net6.0\`
| application           |                       |
| --------------------- | --------------------- |
| CPU Usage (%)         |                    98 |
| Raw CPU Usage (%)     |              1,176.68 |
| Working Set (MB)      |                   190 |
| Build Time (ms)       |                 3,614 |
| Start Time (ms)       |                   380 |
| Published Size (KB)   |                94,141 |
| .NET Core SDK Version | 5.0.100-rc.2.20479.15 |

| load                   |           |
| ---------------------- | --------- |
| CPU Usage (%)          |        39 |
| Raw CPU Usage (%)      |    471.89 |
| Working Set (MB)       |        51 |
| Build Time (ms)        |     4,072 |
| Start Time (ms)        |       107 |
| Published Size (KB)    |    77,080 |
| .NET Core SDK Version  | 3.1.403   |
| First Request (ms)     |       189 |
| Requests               | 1,827,246 |
| Bad responses          |         0 |
| Mean latency (us)      |     2,096 |
| Max latency (us)       |    52,615 |
| Requests/sec           |   122,043 |
| Requests/sec (max)     |   132,081 |
| Read throughput (MB/s) |     20.56 |

### MapControler (davidfowl/fast-path)

`> crank --config .\echo.yml --scenario mapcontroller --profile aspnet-perf-lin --application.options.outputFiles C:\dev\dotnet\aspnetcore\artifacts\bin\Microsoft.AspNetCore.Mvc.Core\Release\net6.0\`

| application           |                       |
| --------------------- | --------------------- |
| CPU Usage (%)         |                    99 |
| Raw CPU Usage (%)     |              1,185.04 |
| Working Set (MB)      |                   186 |
| Build Time (ms)       |                 3,578 |
| Start Time (ms)       |                   372 |
| Published Size (KB)   |                94,141 |
| .NET Core SDK Version | 5.0.100-rc.2.20479.15 |

| load                   |           |
| ---------------------- | --------- |
| CPU Usage (%)          |        42 |
| Raw CPU Usage (%)      |    507.19 |
| Working Set (MB)       |        51 |
| Build Time (ms)        |     4,105 |
| Start Time (ms)        |       106 |
| Published Size (KB)    |    77,080 |
| .NET Core SDK Version  | 3.1.403   |
| First Request (ms)     |       213 |
| Requests               | 2,061,249 |
| Bad responses          |         0 |
| Mean latency (us)      |     1,857 |
| Max latency (us)       |    63,930 |
| Requests/sec           |   137,556 |
| Requests/sec (max)     |   148,030 |
| Read throughput (MB/s) |     23.20 |

A ~7.5% RPS improvement for this action with a small JSON payload.

halter73 · 2020-11-25T20:51:02Z

@davidfowl pointed out what I measured before removed a lot more stuff than this PR ultimately did. Here are the numbers for the actual PR:

### post-echo-controller (pre-PR)

| application           |          |
| --------------------- | -------- |
| CPU Usage (%)         | 99       |
| Raw CPU Usage (%)     | 1,186.47 |
| Working Set (MB)      | 187      |
| Build Time (ms)       | 3,596    |
| Start Time (ms)       | 383      |
| Published Size (KB)   | 94,119   |
| .NET Core SDK Version | 5.0.100  |


| load                   |           |
| ---------------------- | --------- |
| CPU Usage (%)          | 39        |
| Raw CPU Usage (%)      | 468.89    |
| Working Set (MB)       | 51        |
| Build Time (ms)        | 4,118     |
| Start Time (ms)        | 108       |
| Published Size (KB)    | 77,077    |
| .NET Core SDK Version  | 3.1.404   |
| First Request (ms)     | 249       |
| Requests               | 1,845,112 |
| Bad responses          | 0         |
| Mean latency (us)      | 2,075     |
| Max latency (us)       | 79,749    |
| Requests/sec           | 123,451   |
| Requests/sec (max)     | 133,755   |
| Read throughput (MB/s) | 20.76     |

### post-echo-controller (post-PR)

| application           |          |
| --------------------- | -------- |
| CPU Usage (%)         | 99       |
| Raw CPU Usage (%)     | 1,182.20 |
| Working Set (MB)      | 188      |
| Build Time (ms)       | 3,617    |
| Start Time (ms)       | 384      |
| Published Size (KB)   | 94,119   |
| .NET Core SDK Version | 5.0.100  |


| load                   |           |
| ---------------------- | --------- |
| CPU Usage (%)          | 39        |
| Raw CPU Usage (%)      | 472.26    |
| Working Set (MB)       | 50        |
| Build Time (ms)        | 4,104     |
| Start Time (ms)        | 106       |
| Published Size (KB)    | 77,077    |
| .NET Core SDK Version  | 3.1.404   |
| First Request (ms)     | 242       |
| Requests               | 1,873,259 |
| Bad responses          | 0         |
| Mean latency (us)      | 2,043     |
| Max latency (us)       | 62,319    |
| Requests/sec           | 125,211   |
| Requests/sec (max)     | 134,867   |
| Read throughput (MB/s) | 21.08     |

It looks like there might be a small perf improvement, but it's within the margin of error.

ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Nov 12, 2020

davidfowl requested review from pranavkm and javiercn November 12, 2020 14:57

pranavkm reviewed Nov 12, 2020

View reviewed changes

src/Mvc/Mvc.Core/src/DependencyInjection/MvcCoreServiceCollectionExtensions.cs Show resolved Hide resolved

davidfowl changed the base branch from davidfowl/request-delegate-factory to master November 13, 2020 01:30

davidfowl mentioned this pull request Nov 13, 2020

Added IRequestDelegateFactory #27764

Closed

davidfowl force-pushed the davidfowl/controller-delegate-factory branch from 29ebb0c to e1bedc9 Compare November 13, 2020 01:57

Added compatibility switch to enable action invokers

040046b

- This will allow people to turn off the request delegate factory if they were using that extensiblity point before.

davidfowl added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Nov 13, 2020

davidfowl marked this pull request as ready for review November 13, 2020 12:04

davidfowl requested a review from halter73 November 13, 2020 17:46

rynowak reviewed Nov 16, 2020

View reviewed changes

pranavkm reviewed Nov 23, 2020

View reviewed changes

src/Mvc/Mvc.Core/src/MvcOptions.cs Show resolved Hide resolved

pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Nov 23, 2020

pranavkm and others added 3 commits November 23, 2020 10:44

Merge branch 'master' into davidfowl/controller-delegate-factory

5289443

Update src/Mvc/Mvc.Core/src/MvcOptions.cs

271d0bf

Co-authored-by: Pranav K <[email protected]>

Add public API

9a00957

davidfowl added the auto-merge label Nov 23, 2020

davidfowl added auto-merge and removed auto-merge labels Nov 24, 2020

davidfowl added this to the 6.0-preview1 milestone Nov 24, 2020

davidfowl closed this Nov 24, 2020

davidfowl reopened this Nov 24, 2020

halter73 approved these changes Nov 25, 2020

View reviewed changes

ghost merged commit 584d928 into master Nov 25, 2020

ghost deleted the davidfowl/controller-delegate-factory branch November 25, 2020 00:32

davidfowl added the breaking-change This issue / pr will introduce a breaking change, when resolved / merged. label Dec 14, 2022

davidfowl mentioned this pull request Dec 14, 2022

IActionInvokerProvider & UseStatusCodePagesWithReExecute 3.1 to net 6 difference #45589

Open

1 task

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Added ControllerRequestDelegateFactory #27773

Added ControllerRequestDelegateFactory #27773

Uh oh!

davidfowl commented Nov 12, 2020 •

edited

Loading

Uh oh!

Uh oh!

ghost commented Nov 13, 2020

Uh oh!

rynowak commented Nov 16, 2020

Uh oh!

rynowak Nov 16, 2020

Uh oh!

davidfowl Nov 16, 2020

Uh oh!

rynowak Nov 16, 2020

Uh oh!

davidfowl Nov 17, 2020

Uh oh!

Uh oh!

ghost commented Nov 23, 2020

Uh oh!

davidfowl commented Nov 24, 2020

Uh oh!

azure-pipelines bot commented Nov 24, 2020

Uh oh!

halter73 commented Nov 25, 2020

Uh oh!

halter73 left a comment

Uh oh!

halter73 commented Nov 25, 2020

Uh oh!

halter73 commented Nov 25, 2020

Uh oh!

Uh oh!


		controllerContext.ModelState.MaxAllowedErrors = _maxModelValidationErrors;

		var (cacheEntry, filters) = _controllerActionInvokerCache.GetCachedResult(controllerContext);

Added ControllerRequestDelegateFactory #27773

Added ControllerRequestDelegateFactory #27773

Uh oh!

Conversation

davidfowl commented Nov 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ghost commented Nov 13, 2020

Uh oh!

rynowak commented Nov 16, 2020

Uh oh!

rynowak Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

davidfowl Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

rynowak Nov 16, 2020

Choose a reason for hiding this comment

Uh oh!

davidfowl Nov 17, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Nov 23, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

davidfowl commented Nov 24, 2020

Uh oh!

azure-pipelines bot commented Nov 24, 2020

Uh oh!

halter73 commented Nov 25, 2020

Uh oh!

halter73 left a comment

Choose a reason for hiding this comment

Uh oh!

halter73 commented Nov 25, 2020

Uh oh!

halter73 commented Nov 25, 2020

Uh oh!

Uh oh!

davidfowl commented Nov 12, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.