Skip to content

Add Header+Stream pooling #53

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

Closed
wants to merge 1 commit into from
Closed

Conversation

benaadams
Copy link
Contributor

Headers were previously reused for every request on a connection; now they are created for each request. This override restores the previous behaviour (though slightly different - as it is cross connection and runs with a lower idling memory set)

Ref aspnet/KestrelHttpServer#608

/cc @DamianEdwards

Headers were previously reused for every request on a connection; now they are created for each request. This override restores the previous behaviour (though slightly different)
@DamianEdwards
Copy link
Member

@halter73 @mikeharder Can we take a look at this one please. This might be the reason we've seen a slight drop-off in the results on the SmurfLab.

@mikeharder
Copy link
Contributor

@DamianEdwards Sure, I will compare plaintext RPS before and after this change.

@benaadams
Copy link
Contributor Author

The change to "Scenarios": "Plaintext,Json", is likely an oversight

@mikeharder
Copy link
Contributor

@benaadams The only change i see related to "Scenarios": "Plaintext,Json" is an added comma, which is necessary to make valid JSON. Are you talking about the added comma or something else?

@benaadams
Copy link
Contributor Author

Heh, yeah was just the comma

@mikeharder
Copy link
Contributor

@DamianEdwards @benaadams I'm not seeing a statistically significant change in RPS from this PR. RPS was 980k for dev, and 988k for this PR, with a STDEV of 15k. Is this PR expected to improve plaintext RPS?

@benaadams
Copy link
Contributor Author

It should revert a change which will reduce sustained perf (eg 5 mins), also you may have to set the numbers to be as high as the connections count

@mikeharder
Copy link
Contributor

@benaadams Is there a benchmark I can run to see the difference resulting from this change?

@benaadams
Copy link
Contributor Author

Is default now

@benaadams benaadams closed this Jul 27, 2016
@benaadams benaadams deleted the patch-6 branch July 27, 2016 01:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants