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.

Sign up for GitHub

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

Jump to bottom

Use .NET Core SequenceReader #7003

Merged
merged 2 commits into from
Feb 20, 2019
Merged

Use .NET Core SequenceReader #7003

merged 2 commits into from
Feb 20, 2019

Conversation

JeremyKuhne
Copy link
Member

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
Contributor

pakrym commented Jan 25, 2019

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

cc @sebastienros

@jkotalik
Copy link
Contributor

jkotalik commented Feb 7, 2019

/AzurePipelines run AspNetCore-benchmarking-pr

1 similar comment
@sebastienros
Copy link
Member

/AzurePipelines run AspNetCore-benchmarking-pr

@sebastienros
Copy link
Member

/AzurePipelines run AspNetCore-benchmarking-pr

3 similar comments
@sebastienros
Copy link
Member

/AzurePipelines run AspNetCore-benchmarking-pr

@natemcmaster

This comment has been minimized.

Sign in to view
@natemcmaster
Copy link
Contributor

/AzurePipelines run AspNetCore-benchmarking-pr

@natemcmaster
Copy link
Contributor

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

@ahsonkhan
Copy link
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
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
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
Member

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

@sebastienros
Copy link
Member

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

@sebastienros
Copy link
Member

/AzurePipelines run AspNetCore-benchmarking-pr

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@jkotalik
Copy link
Contributor

jkotalik commented Feb 13, 2019 via email

@sebastienros
Copy link
Member

I will do it

@sebastienros
Copy link
Member

@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
Member Author

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

rebased

@pakrym
Copy link
Contributor

pakrym commented Feb 14, 2019

/AzurePipelines run AspNetCore-benchmarking-pr

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@halter73
Copy link
Member

@sebastienros confirmed there isn't a regression.

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

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

@pakrym pakrym pakrym approved these changes

@ahsonkhan ahsonkhan ahsonkhan approved these changes

@jkotalik jkotalik jkotalik approved these changes

@Tratcher Tratcher Awaiting requested review from Tratcher

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.