-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Add short circuit in routing #46713
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
Add short circuit in routing #46713
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
859b841
Add route short circuit
Kahbazi 67939ca
Add benchmark
Kahbazi ba36f70
Review feedback
Kahbazi f420070
remove log
Kahbazi a412348
Reuse Short Circuit metadata
Kahbazi 8abecae
Review Feedback
Kahbazi a3823d6
Set the order
Kahbazi baaabf5
Set display name, add tests
Kahbazi 4c5c1d4
Minor edits
adityamandaleeka f40f14b
Spelling
adityamandaleeka 91ef740
Update security metadata throw test to include cases without a status.
adityamandaleeka File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
113 changes: 113 additions & 0 deletions
113
src/Http/Routing/perf/Microbenchmarks/EndpointRoutingShortCircuitBenchmark.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,113 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using System.Diagnostics; | ||
using BenchmarkDotNet.Attributes; | ||
using Microsoft.AspNetCore.Builder; | ||
using Microsoft.AspNetCore.Http; | ||
using Microsoft.AspNetCore.Routing.Matching; | ||
using Microsoft.AspNetCore.Routing.ShortCircuit; | ||
using Microsoft.Extensions.Logging.Abstractions; | ||
using Microsoft.Extensions.Options; | ||
using Microsoft.Extensions.Primitives; | ||
|
||
namespace Microsoft.AspNetCore.Routing; | ||
|
||
public class EndpointRoutingShortCircuitBenchmark | ||
{ | ||
private EndpointRoutingMiddleware _normalEndpointMiddleware; | ||
private EndpointRoutingMiddleware _shortCircuitEndpointMiddleware; | ||
|
||
[GlobalSetup] | ||
public void Setup() | ||
{ | ||
var normalEndpoint = new Endpoint(context => Task.CompletedTask, new EndpointMetadataCollection(), "normal"); | ||
|
||
_normalEndpointMiddleware = new EndpointRoutingMiddleware( | ||
new BenchmarkMatcherFactory(normalEndpoint), | ||
NullLogger<EndpointRoutingMiddleware>.Instance, | ||
new BenchmarkEndpointRouteBuilder(), | ||
new BenchmarkEndpointDataSource(), | ||
new DiagnosticListener("benchmark"), | ||
Options.Create(new RouteOptions()), | ||
context => Task.CompletedTask); | ||
|
||
var shortCircuitEndpoint = new Endpoint(context => Task.CompletedTask, new EndpointMetadataCollection(new ShortCircuitMetadata(200)), "shortcircuit"); | ||
|
||
_shortCircuitEndpointMiddleware = new EndpointRoutingMiddleware( | ||
new BenchmarkMatcherFactory(shortCircuitEndpoint), | ||
NullLogger<EndpointRoutingMiddleware>.Instance, | ||
new BenchmarkEndpointRouteBuilder(), | ||
new BenchmarkEndpointDataSource(), | ||
new DiagnosticListener("benchmark"), | ||
Options.Create(new RouteOptions()), | ||
context => Task.CompletedTask); | ||
|
||
} | ||
|
||
[Benchmark] | ||
public async Task NormalEndpoint() | ||
{ | ||
var context = new DefaultHttpContext(); | ||
await _normalEndpointMiddleware.Invoke(context); | ||
} | ||
|
||
[Benchmark] | ||
public async Task ShortCircuitEndpoint() | ||
{ | ||
var context = new DefaultHttpContext(); | ||
await _shortCircuitEndpointMiddleware.Invoke(context); | ||
} | ||
} | ||
|
||
internal class BenchmarkMatcherFactory : MatcherFactory | ||
{ | ||
private readonly Endpoint _endpoint; | ||
|
||
public BenchmarkMatcherFactory(Endpoint endpoint) | ||
{ | ||
_endpoint = endpoint; | ||
} | ||
|
||
public override Matcher CreateMatcher(EndpointDataSource dataSource) | ||
{ | ||
return new BenchmarkMatcher(_endpoint); | ||
} | ||
|
||
internal class BenchmarkMatcher : Matcher | ||
{ | ||
private Endpoint _endpoint; | ||
|
||
public BenchmarkMatcher(Endpoint endpoint) | ||
{ | ||
_endpoint = endpoint; | ||
} | ||
|
||
public override Task MatchAsync(HttpContext httpContext) | ||
{ | ||
httpContext.SetEndpoint(_endpoint); | ||
return Task.CompletedTask; | ||
} | ||
} | ||
} | ||
|
||
internal class BenchmarkEndpointRouteBuilder : IEndpointRouteBuilder | ||
{ | ||
public IServiceProvider ServiceProvider => throw new NotImplementedException(); | ||
|
||
public ICollection<EndpointDataSource> DataSources => new List<EndpointDataSource>(); | ||
|
||
public IApplicationBuilder CreateApplicationBuilder() | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
} | ||
internal class BenchmarkEndpointDataSource : EndpointDataSource | ||
{ | ||
public override IReadOnlyList<Endpoint> Endpoints => throw new NotImplementedException(); | ||
|
||
public override IChangeToken GetChangeToken() | ||
{ | ||
throw new NotImplementedException(); | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,8 @@ | ||
#nullable enable | ||
Microsoft.AspNetCore.Routing.RouteHandlerServices | ||
static Microsoft.AspNetCore.Routing.RouteHandlerServices.Map(Microsoft.AspNetCore.Routing.IEndpointRouteBuilder! endpoints, string! pattern, System.Delegate! handler, System.Collections.Generic.IEnumerable<string!>! httpMethods, System.Func<System.Reflection.MethodInfo!, Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions?, Microsoft.AspNetCore.Http.RequestDelegateMetadataResult!>! populateMetadata, System.Func<System.Delegate!, Microsoft.AspNetCore.Http.RequestDelegateFactoryOptions!, Microsoft.AspNetCore.Http.RequestDelegateMetadataResult?, Microsoft.AspNetCore.Http.RequestDelegateResult!>! createRequestDelegate) -> Microsoft.AspNetCore.Builder.RouteHandlerBuilder! | ||
Microsoft.AspNetCore.Builder.RouteShortCircuitEndpointConventionBuilderExtensions | ||
Microsoft.AspNetCore.Routing.RouteShortCircuitEndpointRouteBuilderExtensions | ||
static Microsoft.AspNetCore.Builder.RouteShortCircuitEndpointConventionBuilderExtensions.ShortCircuit(this Microsoft.AspNetCore.Builder.IEndpointConventionBuilder! builder, int? statusCode = null) -> Microsoft.AspNetCore.Builder.IEndpointConventionBuilder! | ||
static Microsoft.AspNetCore.Routing.RouteShortCircuitEndpointRouteBuilderExtensions.MapShortCircuit(this Microsoft.AspNetCore.Routing.IEndpointRouteBuilder! builder, int statusCode, params string![]! routePrefixes) -> Microsoft.AspNetCore.Builder.IEndpointConventionBuilder! | ||
static Microsoft.Extensions.DependencyInjection.RoutingServiceCollectionExtensions.AddRoutingCore(this Microsoft.Extensions.DependencyInjection.IServiceCollection! services) -> Microsoft.Extensions.DependencyInjection.IServiceCollection! |
39 changes: 39 additions & 0 deletions
39
src/Http/Routing/src/ShortCircuit/RouteShortCircuitEndpointConventionBuilderExtensions.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
// Licensed to the .NET Foundation under one or more agreements. | ||
// The .NET Foundation licenses this file to you under the MIT license. | ||
|
||
using Microsoft.AspNetCore.Routing.ShortCircuit; | ||
|
||
namespace Microsoft.AspNetCore.Builder; | ||
|
||
/// <summary> | ||
/// Short circuit extension methods for <see cref="IEndpointConventionBuilder"/>. | ||
/// </summary> | ||
public static class RouteShortCircuitEndpointConventionBuilderExtensions | ||
{ | ||
private static readonly ShortCircuitMetadata _200ShortCircuitMetadata = new ShortCircuitMetadata(200); | ||
private static readonly ShortCircuitMetadata _401ShortCircuitMetadata = new ShortCircuitMetadata(401); | ||
private static readonly ShortCircuitMetadata _404ShortCircuitMetadata = new ShortCircuitMetadata(404); | ||
private static readonly ShortCircuitMetadata _nullShortCircuitMetadata = new ShortCircuitMetadata(null); | ||
Tratcher marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/// <summary> | ||
/// Short circuit the endpoint(s). | ||
/// The execution of the endpoint will happen in UseRouting middleware instead of UseEndpoint. | ||
/// </summary> | ||
/// <param name="builder">The endpoint convention builder.</param> | ||
/// <param name="statusCode">The status code to set in the response.</param> | ||
/// <returns>The original convention builder parameter.</returns> | ||
public static IEndpointConventionBuilder ShortCircuit(this IEndpointConventionBuilder builder, int? statusCode = null) | ||
{ | ||
var metadata = statusCode switch | ||
{ | ||
200 => _200ShortCircuitMetadata, | ||
401 => _401ShortCircuitMetadata, | ||
404 => _404ShortCircuitMetadata, | ||
null => _nullShortCircuitMetadata, | ||
_ => new ShortCircuitMetadata(statusCode) | ||
}; | ||
|
||
builder.Add(b => b.Metadata.Add(metadata)); | ||
return builder; | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the result
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's NormalEndpoints benchmark without your changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it's with my changes. I will add another benchmark with previous implementation too. I assume we don't need to have two different implementation in the code base, right? I just run it in my local?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, we just want to confirm the before-and-after to make sure this doesn't regress anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1,518,082.2 vs 1,563,187.5? That's less than 3% on a microbenchmark. I doubt it would even show up on the end-to-end. Adding the endpoint execution middleware to this benchmark would make it more representative in all cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I looked at the wrong results.
We don't have to guess, we can just run it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this so much worse?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hitting the new code path that would normally not be hit until the EndpointMiddleware. If we include the EndpointMiddleware in the benchmark you shouldn't notice a difference.
https://github.com/dotnet/aspnetcore/pull/46713/files#diff-5019372fafa72e521b12fab1c28fd96919cb21c5294003b7777f5a46322cca54R113-R175
https://github.com/dotnet/aspnetcore/blob/main/src/Http/Routing/src/EndpointMiddleware.cs#L34-L78
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's the result with both endpoint routing middleware and endpoint middleware together.