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

Reimplement the concurrency limiter middleware to use the new abstractions & implementations #39040

Merged
merged 5 commits into from
Jan 4, 2022

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Dec 14, 2021

Contributes to #38306, not sure if we want to take it further with breaking API changes? Or do we obsolete it in favor of the new one?

This is a literal replacement of the implementations from the old middleware with the new RateLimiter components. It's not a perfect fit, but it's close enough.

Outstanding issue: dotnet/runtime#62817

Design mismatch: IQueuePolicy has an explicit release API, where ConcurrencyLimiter returns a lease that's disposable. Layering IQueuePolicy on ConcurrencyLimiter requires tracking the outstanding leases.

I was able to combine QueuePolicy and StackPolicy since the only difference is the order enum.

@Tratcher Tratcher added this to the 7.0-preview1 milestone Dec 14, 2021
@Tratcher Tratcher self-assigned this Dec 14, 2021
@ghost ghost added the area-runtime label Dec 14, 2021
Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

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

Seems reasonable - my feeling is we can start with this then make the breaking API change as a next stop (as opposed to obsoletion), but I'm open to persuasion

@halter73
Copy link
Member

How do the microbenchmarks look before and after? I'm expecting a big regression 😨

@halter73
Copy link
Member

Seems reasonable - my feeling is we can start with this then make the breaking API change as a next stop (as opposed to obsoletion), but I'm open to persuasion

I think we should pick a new namespace (Microsoft.AspNetCore.RateLimiting?) and move it into the shared framework. I don't see the point in breaking people who already have working code. And the newer middleware should be broader in that it supports both rate limiting, per-endpoint configuration, etc... beyond just the concurrency limiting of all requests, so Microsoft.AspNetCore.ConcurrencyLimiter will no longer make as much sense.

@Tratcher
Copy link
Member Author

int in breaking people who already have working code. And the newer middleware should be broader in that it supports both rate limiting, per-endpoint configuration, etc... beyond just the concurrency limiting of all requests, so Microsoft.AspNetCore.ConcurrencyLimiter will no longer make as much sense.

The new middleware is a separate topic (#37384). I expect to make minimal changes to the old one and gradually phase it out.

@Tratcher
Copy link
Member Author

1

Before:

Method YieldsThreadInternally Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated
Baseline False 3.556 ns 0.0053 ns 0.0044 ns 281,250,356.1 - - - -
WithEmptyQueueOverhead_QueuePolicy False 63.696 ns 0.6846 ns 0.6069 ns 15,699,455.3 - - - -
WithEmptyQueueOverhead_StackPolicy False 47.787 ns 0.0788 ns 0.0658 ns 20,926,140.5 - - - -
Baseline True 236.572 ns 4.6482 ns 4.5651 ns 4,227,040.0 0.0023 - - 112 B
WithEmptyQueueOverhead_QueuePolicy True 541.397 ns 10.7687 ns 17.0802 ns 1,847,071.9 0.0063 - - 280 B
WithEmptyQueueOverhead_StackPolicy True 513.893 ns 3.8522 ns 3.4149 ns 1,945,929.6 0.0063 - - 280 B

After:

Method YieldsThreadInternally Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated
Baseline False 2.625 ns 0.0042 ns 0.0033 ns 380,986,401.3 - - - -
WithEmptyQueueOverhead_QueuePolicy False 90.571 ns 0.7445 ns 0.6964 ns 11,041,012.3 0.0010 - - 40 B
WithEmptyQueueOverhead_StackPolicy False 90.391 ns 1.2970 ns 1.1498 ns 11,063,010.2 0.0010 - - 40 B
Baseline True 258.421 ns 5.0894 ns 11.7955 ns 3,869,661.5 0.0023 - - 112 B
WithEmptyQueueOverhead_QueuePolicy True 600.372 ns 11.8710 ns 19.1695 ns 1,665,633.9 0.0070 - - 320 B
WithEmptyQueueOverhead_StackPolicy True 589.859 ns 11.2993 ns 13.4510 ns 1,695,319.6 0.0070 0.0008 - 320 B

2

Before:

Method MaxConcurrentRequests Mean Error StdDev Op/s Gen 0 Gen 1 Gen 2 Allocated
Baseline 8 135.7 ns 2.71 ns 6.49 ns 7,367,679.7 - - - 120 B
QueueingAll_QueuePolicy 8 558.0 ns 14.83 ns 42.08 ns 1,792,009.4 - - - 328 B
QueueingAll_StackPolicy 8 655.0 ns 23.58 ns 66.50 ns 1,526,779.6 - - - 493 B

After:

Method MaxConcurrentRequests Mean Error StdDev Median Op/s Gen 0 Gen 1 Gen 2 Allocated
Baseline 8 140.4 ns 2.80 ns 4.67 ns 140.6 ns 7,122,436.7 - - - 120 B
QueueingAll_QueuePolicy 8 1,169.1 ns 108.56 ns 320.09 ns 1,285.5 ns 855,346.8 - - - 580 B
QueueingAll_StackPolicy 8 797.8 ns 21.69 ns 61.54 ns 779.2 ns 1,253,416.6 - - - 569 B

3
Before:

Method Mean Error StdDev Median Op/s Gen 0 Gen 1 Gen 2 Allocated
Baseline 576.6 ns 11.39 ns 29.82 ns 577.0 ns 1,734,166.2 - - - 1 KB
RejectingRapidly_QueuePolicy 752.8 ns 68.16 ns 200.96 ns 663.9 ns 1,328,370.2 - - - 1 KB
RejectingRapidly_StackPolicy 745.6 ns 55.41 ns 163.39 ns 659.2 ns 1,341,177.3 - - - 1 KB

After:

Method Mean Error StdDev Median Op/s Gen 0 Gen 1 Gen 2 Allocated
Baseline 612.5 ns 10.69 ns 11.43 ns 613.6 ns 1,632,626.4 - - - 1 KB
RejectingRapidly_QueuePolicy 1,094.1 ns 21.40 ns 25.47 ns 1,094.8 ns 913,974.9 - - - 1 KB
RejectingRapidly_StackPolicy 629.3 ns 27.53 ns 73.01 ns 595.6 ns 1,589,029.6 - - - 1 KB

Note RejectingRapidly_StackPolicy is misleading because of dotnet/runtime#62817

@Tratcher Tratcher force-pushed the tratcher/rate_limit_concurrency branch from ef3fd3d to b5de7cd Compare January 4, 2022 18:17
@Tratcher Tratcher enabled auto-merge (squash) January 4, 2022 18:55
@Tratcher Tratcher merged commit dae7ed1 into dotnet:main Jan 4, 2022
@Tratcher Tratcher deleted the tratcher/rate_limit_concurrency branch January 4, 2022 19:55
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlesware
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants