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

crypto/tls: replace tls Conn's rawInput with a small buffer when Read timeout #48229

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cch123
Copy link
Contributor

@cch123 cch123 commented Sep 7, 2021

fixes #43563

@google-cla google-cla bot added the cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change. label Sep 7, 2021
@cch123
Copy link
Contributor Author

cch123 commented Sep 7, 2021

I'll add more benchmark data later

@gopherbot
Copy link

This PR (HEAD: c067980) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/347916 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@cch123
Copy link
Contributor Author

cch123 commented Sep 8, 2021

code demos is in #43563
server side read timeout value is 5 seconds

before optimization, 20k connections, 16k request size,500 qps:

  PID USER      PR  NI    VIRT    RES    SHR S  %CPU %MEM     TIME+ COMMAND
11397 root      20   0 1722816 1.181g   3712 S  16.3 15.5   0:29.03 c
11367 root      20   0 1937180 1.053g   2744 S  17.3 13.8   1:01.31 s -----> this is the server process

cost totally 1g+ memory

after optimization, 20k conns, 16k request size,500 qps:

  PID USER      PR  NI    VIRT    RES    SHR S  %CPU %MEM     TIME+ COMMAND
22618 root      20   0 1491180 1.135g   4276 S  16.9 14.9   0:26.76 c
22610 root      20   0 1528452 691064   3444 S  18.9  8.6   1:01.26 s_opt -----> this is the server process

about 700M memory

Since I don't have a good enough server for more connections to test now.... I can't provide data for more than 20k connections currently

@gopherbot
Copy link

This PR (HEAD: 9da3b34) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/347916 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@cch123 cch123 changed the title crypto/tls: shrink tls Conn's rawInput buffer when Read timeout crypto/tls: replace tls Conn's rawInput by a small buffer when Read timeout Sep 8, 2021
@cch123 cch123 changed the title crypto/tls: replace tls Conn's rawInput by a small buffer when Read timeout crypto/tls: replace tls Conn's rawInput with a small buffer when Read timeout Sep 8, 2021
This will reduce the rawInput buffer memory cost for large number of idle tls connections
@gopherbot
Copy link

This PR (HEAD: 51de0f8) has been imported to Gerrit for code review.

Please visit https://go-review.googlesource.com/c/go/+/347916 to see it.

Tip: You can toggle comments from me using the comments slash command (e.g. /comments off)
See the Wiki page for more info

@gopherbot
Copy link

Message from ChunHui Cao:

Patch Set 4:

(1 comment)


Please don’t reply on this GitHub thread. Visit golang.org/cl/347916.
After addressing review feedback, remember to publish your drafts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Used by googlebot to label PRs as having a valid CLA. The text of this label should not change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

crypto/tls: Conn.rawInput buffer has no chance to shrink on large number of idle conns
2 participants