Skip to content

Use .NET Core SequenceReader#7003

Merged
halter73 merged 2 commits into
dotnet:masterfrom
JeremyKuhne:usesequencereader
Feb 20, 2019
Merged

Use .NET Core SequenceReader#7003
halter73 merged 2 commits into
dotnet:masterfrom
JeremyKuhne:usesequencereader

Conversation

@JeremyKuhne
Copy link
Copy Markdown
Member

@JeremyKuhne JeremyKuhne commented Jan 25, 2019

Remove BufferReader and use SequenceReader which now ships in CoreFX.

This is related to aspnet/KestrelHttpServer#3068 which builds on the functionality added to the reader.
I'll follow up with a port of said PR after this goes in.

Addresses #4693

cc: @danmosemsft, @davidfowl

@pakrym
Copy link
Copy Markdown
Contributor

pakrym commented Jan 25, 2019

We need to run a perf test with this to make sure we have no regressions.

cc @sebastienros

davidfowl
davidfowl reviewed Jan 25, 2019
View reviewed changes
Comment thread src/Servers/Kestrel/Core/src/Internal/Http/HttpParser.cs Outdated
@jkotalik
Copy link
Copy Markdown
Contributor

jkotalik commented Feb 7, 2019

/AzurePipelines run AspNetCore-benchmarking-pr

1 similar comment
@sebastienros
Copy link
Copy Markdown
Member

sebastienros commented Feb 7, 2019

/AzurePipelines run AspNetCore-benchmarking-pr

@sebastienros
Copy link
Copy Markdown
Member

sebastienros commented Feb 8, 2019

/AzurePipelines run AspNetCore-benchmarking-pr

3 similar comments
@sebastienros
Copy link
Copy Markdown
Member

sebastienros commented Feb 8, 2019

/AzurePipelines run AspNetCore-benchmarking-pr

@natemcmaster

This comment has been minimized.

Sign in to view
@natemcmaster
Copy link
Copy Markdown
Contributor

natemcmaster commented Feb 8, 2019

/AzurePipelines run AspNetCore-benchmarking-pr

@natemcmaster
Copy link
Copy Markdown
Contributor

natemcmaster commented Feb 8, 2019

Ok, i give up. The bot doesn't like us anymore.

@ahsonkhan
Copy link
Copy Markdown
Contributor

ahsonkhan commented Feb 8, 2019 •
edited
Loading

Ok, i give up. The bot doesn't like us anymore.

Let me give it a try, it always listens to me.

/AzurePipelines run ... away! (you are free)

:)

@jkotalik
Copy link
Copy Markdown
Contributor

jkotalik commented Feb 8, 2019

I'm just going to make a separate temp PR for perf validation.

@jkotalik jkotalik mentioned this pull request Feb 8, 2019
Closed
@jkotalik
Copy link
Copy Markdown
Contributor

jkotalik commented Feb 8, 2019

Results

Description RPS CPU (%) Memory (MB) Avg. Latency (ms) Startup (ms) First Request (ms) Latency (ms) Ratio
Baseline 2,165,914 91 385 1.8 212 36.6 0.5 1.00
SequenceReader 2,172,325 93 181 2.1 224 37 0.5 1.00

@halter73
Copy link
Copy Markdown
Member

halter73 commented Feb 13, 2019

@jkotalik Should we rerun the benchmarks for this? @pakrym reminded me there was an issue with the benchmarks server last week.

@sebastienros
Copy link
Copy Markdown
Member

sebastienros commented Feb 13, 2019

not only an issue but the baseline is using a different runtime, look at the memory usage

@sebastienros
Copy link
Copy Markdown
Member

sebastienros commented Feb 13, 2019

/AzurePipelines run AspNetCore-benchmarking-pr

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Feb 13, 2019

No pipelines are associated with this pull request.

@jkotalik
Copy link
Copy Markdown
Contributor

jkotalik commented Feb 13, 2019 via email

@sebastienros
Copy link
Copy Markdown
Member

sebastienros commented Feb 13, 2019

I will do it

@sebastienros
Copy link
Copy Markdown
Member

sebastienros commented Feb 13, 2019

@JeremyKuhne would you mind merging master into your branch so I can run benchmarks?

@JeremyKuhne
Use .NET Core SequenceReader
2c649b2 
Remove BufferReader and use SequenceReader<T> which now ships in CoreFX.

This is related to aspnet/KestrelHttpServer#3068 which builds on the functionality added to the reader.
@JeremyKuhne
Use var
a02ac20
@JeremyKuhne
Copy link
Copy Markdown
Member Author

JeremyKuhne commented Feb 14, 2019

@JeremyKuhne would you mind merging master into your branch so I can run benchmarks?

rebased

@pakrym
Copy link
Copy Markdown
Contributor

pakrym commented Feb 14, 2019

/AzurePipelines run AspNetCore-benchmarking-pr

@azure-pipelines
Copy link
Copy Markdown

azure-pipelines Bot commented Feb 14, 2019

No pipelines are associated with this pull request.

@halter73
Copy link
Copy Markdown
Member

halter73 commented Feb 20, 2019

@sebastienros confirmed there isn't a regression.

@halter73 halter73 merged commit 001b6ea into dotnet:master Feb 20, 2019
@danmoseley
Copy link
Copy Markdown
Member

danmoseley commented Feb 21, 2019

Yay.

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions 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

Reviewers

@davidfowl davidfowl davidfowl left review comments

@Tratcher Tratcher Awaiting requested review from Tratcher

+3 more reviewers

@pakrym pakrym pakrym approved these changes

@ahsonkhan ahsonkhan ahsonkhan approved these changes

@jkotalik jkotalik jkotalik approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.