Add parameters to adjust grpc wirte/read buffer size #12596
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. ℹ️ Googlers: Go here for more info. |
googlebot
added
the
cla: no
|
Hi @yangdihangN. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
11 |
|
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
googlebot
added
cla: yes
and removed
cla: no
pkg/keepalive/options.go
Outdated
| @@ -53,6 +61,8 @@ func DefaultOption() *Options { | |||
| Timeout: 10 * time.Second, | |||
| MaxServerConnectionAge: Infinity, | |||
| MaxServerConnectionAgeGrace: 10 * time.Second, | |||
| WriteBufferSize: 32 * 1024, | |||
There was a problem hiding this comment.
What is the gRPC default?
There was a problem hiding this comment.
default is 32k both, in google.golang.org/grpc/clientconn.go
const (
// ...
// http2IOBufSize specifies the buffer size for sending frames.
defaultWriteBufSize = 32 * 1024
defaultReadBufSize = 32 * 1024
)
pkg/keepalive/options.go
Outdated
| // WriteBufferSize determines how much data can be batched before doing a write on the wire. | ||
| // The corresponding memory allocation for this buffer will be twice the size to keep syscalls low. | ||
| // The default value for this buffer is 32KB. | ||
| WriteBufferSize int |
There was a problem hiding this comment.
These two option does not belong to keepalive. How about changing the pkg to grpc? And also move all grpc related options in.
There was a problem hiding this comment.
I am not quite sure about the usage of some other parameters in grpc options. Can I just change the package name and add a todo tag?
|
This provides a little memory improvement. But compared to pilot other caches, this quantity of memory is too little. So the total elevation is tiny. But overall i am not against this. |
pkg/keepalive/options.go
Outdated
| cmd.PersistentFlags().IntVar(&o.WriteBufferSize, "writeBufferSize", o.WriteBufferSize, | ||
| "WriteBufferSize determines how much data can be batched before doing a write on the wire.") | ||
| cmd.PersistentFlags().IntVar(&o.ReadBufferSize, "readBufferSize", o.ReadBufferSize, | ||
| "ReadBufferSize lets you set the size of read buffer, this determines how much data can be read at "+ |
There was a problem hiding this comment.
can you please provide guidance on how this should be tuned by the user in the description here? I think this desp is going to our reference doc.
There was a problem hiding this comment.
Definitely very few users may need adjust these values. The desc should be more clearly.
There was a problem hiding this comment.
I will add some suggested usages for these two parameters here, after I run some relevant grpc benchmark.
There was a problem hiding this comment.
Here is some benchmark for WB/RB.
I think the scene similar to istio is that there are more Connections, but each Connection has fewer concurrent calls (envoy doesn't seem to harass pilot often). From the results, the size of ReadBuffer has almost no effect on the qps of Server (pilot). Can we reduce the default value of ReadBuffer and do related verification? In addition, I added some suggestions at the end of the article, which may be used in our references.
|
@yangdihangN: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
istio-testing
added
the
needs-rebase
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: yangdihangN The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
When the number of xds connections is too large, grpc write buffer/read buffer will consume a lot of memory (96k per-xdsConnection). This pr adds a parameter to adjust the size of the grpc buffer.
xds-connections = 4000
default (writebuffer/readbuffer = 32k) :
writebuffer/readbuffer =16k :
