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.

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

Enable HTTP Keepalive's to reduce number of TLS handshakes #45

Merged
merged 1 commit into from
Jul 9, 2018

Conversation

takeyourhatoff
Copy link
Contributor

We found that when using Credhub with Concourse, there was a massive spike in
CPU usage. We profiled ATC (Concourse component that interacts with Credhub) and
found that >90% of samples were in TLS handshake methods. We did not profile
credhub, but presumably it was much the same.

This commit ensures HTTP response body's are read to completion, which is
required for net/http to reuse the connection and avoid a costly TLS handshake.
It also increases the maximum number of idle HTTP connections to 100, from the
default of 2, which is required to work around limitations in net/http:

golang/go#13801

Signed-off-by: Ife Runsewe [email protected]

We found that when using Credhub with Concourse, there was a massive spike in
CPU usage. We profiled ATC (Concourse component that interacts with Credhub) and
found that >90% of samples were in TLS handshake methods. We did not profile
credhub, but presumably it was much the same.

This commit ensures HTTP response body's are read to completion, which is
required for net/http to reuse the connection and avoid a costly TLS handshake.
It also increases the maximum number of idle HTTP connections to 100, from the
default of 2, which is required to work around limitations in net/http:

golang/go#13801

Signed-off-by: Ife Runsewe <[email protected]>
@cfdreddbot
Copy link

Hey takeyourhatoff!

Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA.

@cf-gitbot
Copy link

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/158865415

The labels on this github issue will be updated when the story is started.

@DanielJonesEB
Copy link

Hi CredHubbers! Just so you can see the impact, here's some profiling data before and after this PR:

Here you can see that most of the time is spent doing TLS handshakes:
screen shot 2018-07-06 at 13 27 30

...and here it's basically invisible because the ATC is now able to get on with it's business:
screen shot 2018-07-06 at 13 27 37

Here's a (under-represented, because it was previously CPU-starved) demonstration of the impact on the VM:
screen shot 2018-07-06 at 13 28 11

@mdelillo mdelillo merged commit 0887387 into cloudfoundry:master Jul 9, 2018
@mdelillo
Copy link
Contributor

mdelillo commented Jul 9, 2018

Looks good to me, thanks for submitting this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants