Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Make benchmarks closer to what a client should send #1471

Merged
merged 2 commits into from
Mar 9, 2017
Merged

Make benchmarks closer to what a client should send #1471

merged 2 commits into from
Mar 9, 2017

Conversation

natemcmaster
Copy link
Contributor

We shouldn't benchmark a "bad client". Absolute-form requests should only be sent to proxies, not end-servers.

Make benchmarks closer to what a client should send
3549577 
Absolute-form requests should only be sent to proxies, not end-servers.
@natemcmaster natemcmaster mentioned this pull request Mar 9, 2017
Merged
@davidfowl
Copy link
Member

Sneaky, fix the benchmark 😄. I'd also run the end to end tests.

@natemcmaster
Copy link
Contributor Author

hehe, path of least resistance :)

@natemcmaster
Copy link
Contributor Author

To clarify:


   To allow for transition to the absolute-form for all requests in some
   future version of HTTP, a server MUST accept the absolute-form in
   requests, even though HTTP/1.1 clients will only send them in
   requests to proxies.

https://tools.ietf.org/html/rfc7230#section-5.3.2

@natemcmaster
Copy link
Contributor Author

Essentially these benchmarks are testing a scenario that clients should never do.

@natemcmaster
Copy link
Contributor Author

🆙 📅

@natemcmaster natemcmaster merged commit 3a1f3d1 into dev Mar 9, 2017
@natemcmaster natemcmaster deleted the namc/bench-targets branch March 9, 2017 01:21
benaadams
benaadams reviewed Mar 9, 2017
View reviewed changes
test/Microsoft.AspNetCore.Server.Kestrel.Performance/RequestParsing.cs
@@ -42,6 +42,16 @@ public void PlaintextTechEmpower()
}
}

[Benchmark(Baseline = true, OperationsPerInvoke = RequestParsingData.InnerLoopCount)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't have two benchmarks with Baseline = true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops. sorry, i'm the new here. will fix it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
3 more reviewers

@benaadams benaadams benaadams left review comments

@davidfowl davidfowl davidfowl approved these changes

@cesarblum cesarblum cesarblum approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@natemcmaster @davidfowl @cesarblum @benaadams @dnfclas