-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Overhead Benchmark for Request Throttling #10907
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
Conversation
|
||
private async Task NextDelay(HttpContext context) | ||
{ | ||
for (int i = 0; i < 1000; i++) { } |
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.
I think you need a better way to represent work. spoopy.
Current runs:
|
3922dfc
to
8a55fc9
Compare
8a55fc9
to
a9337ff
Compare
@halter73 am I good to merge here? Also I've snuck a |
src/Middleware/RequestThrottling/perf/Microbenchmarks/QueueFullOverhead.cs
Show resolved
Hide resolved
src/Middleware/RequestThrottling/perf/Microbenchmarks/QueueFullOverhead.cs
Outdated
Show resolved
Hide resolved
|
||
namespace Microsoft.AspNetCore.RequestThrottling.Internal | ||
{ | ||
internal class TailDrop : RequestQueue |
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.
We shouldn't just change the default to TailDrop. Keep all the existing tests fro the base RequestQueue and add special tests for TailDrop.
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.
Hopefully will have multiple queue implementations soon, will run the existing tests on all of them.
RequestQueueLimit = requestQueueLimit | ||
}; | ||
|
||
options.ServerAlwaysBlocks = maxConcurrentRequests == 0; |
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.
This check should be in options or the middleware. Right now this only work if you use TestUtils.
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.
I think I understand now. What you have is fine, just use a different method for the ServerAlwaysBlocks case.
src/Middleware/RequestThrottling/perf/Microbenchmarks/QueueFullOverhead.cs
Outdated
Show resolved
Hide resolved
@@ -15,6 +15,8 @@ namespace RequestThrottlingSample | |||
{ | |||
public class Startup | |||
{ | |||
private int totalLoads = 0; |
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.
private int totalLoads = 0; | |
private int _totalLoads = 0; |
_requestThrottlingOptions.MaxConcurrentRequests.Value, | ||
_requestThrottlingOptions.RequestQueueLimit); | ||
|
||
if (_requestThrottlingOptions.ServerAlwaysBlocks) |
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.
Nit: comment here too that this is for testing only
This comment was made automatically. If there is a problem contact [email protected]. I've triaged the above build. I've created/commented on the following issue(s) |
There's a benchmark project!
The single included benchmark captures the overhead of requests passing directly through the middleware (which happens the server has extra room). The baseline represents calling the rest of the pipeline directly without passing through the middleware. Comparing these two numbers, we can get an estimate for the overhead.