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

Reuse server send buffers #6608

Closed
wants to merge 1 commit into from
Closed

Conversation

PapaCharlie
Copy link
Contributor

Under significant load, the server concurrently allocates a significant amount of buffers to serialize and compress the responses before sending them over the wire. This causes significant GC pressure and can cause large spikes in memory allocations.

Tests are benchmark results are pending.

Under significant load, the server concurrently allocates a significant amount
of buffers to serialize and compress the responses before sending them over the
wire. This causes significant GC pressure and can cause large spikes in memory
allocations.
@linux-foundation-easycla
Copy link

CLA Missing ID CLA Not Signed

@@ -152,6 +152,8 @@ type dataFrame struct {
// onEachWrite is called every time
// a part of d is written out.
onEachWrite func()
// onSent is called once all the bt
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment is incomplete?

Comment on lines 1774 to 1776
// prepareMsg returns the hdr, payload and data
// using the compressors passed or using the
// passed preparedmsg
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment could be updated

return err

var compData []byte
if shouldCompress(cp, comp) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could save the result of shouldCompress and reuse in the following lines

}
hdr, payload := msgHeader(data, compData)
// TODO(dfawley): should we be checking len(data) instead?
if len(payload) > s.opts.maxSendMessageSize {
s.opts.sendBufferPool.Put(dataBuf)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this be Put(data) instead? since data is the new buffer after encoding at line 1135.

return status.Errorf(codes.ResourceExhausted, "grpc: trying to send message larger than max (%d vs. %d)", len(payload), s.opts.maxSendMessageSize)
}
opts.OnSent = func() {
s.opts.sendBufferPool.Put(dataBuf)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, shouldn't this be Put(data)?

@arvindbr8
Copy link
Member

@PapaCharlie, seems like your github account has an issue with CLA. Please fix using this Help Article as mentioned in the comment above.

@arvindbr8
Copy link
Member

@PapaCharlie Also please consider filing an issue with us before sending the PR. Changes like this require more investigation and proof - which needs to happen in an issue.
Please open an issue with us here.
I'm closing this for now and we can reopen this once we have a solid idea on what the issue is and set in stone the fix (if required).

@arvindbr8 arvindbr8 closed this Sep 8, 2023
@PapaCharlie
Copy link
Contributor Author

Hey @arvindbr8, I didn't get a chance to link these issues when I opened the PR but here you go:
#2817 and #2816
This is also in response to pretty significant performance testing. I took some CPU and heap profiles for the same server code running on v1.58.0 and my fork under significant load (of the nature where the server is concurrently sending very large responses to many clients). Here are the results:

v1.58.0 CPU profile

old_cpu

v1.58.0 Heap profile (alloc_space)

old_heap

Fork CPU profile

new_cpu

Fork Heap profile (alloc_space)

new_heap

If you want me to repro this using simpler code (and/or a benchmark) I can give it a shot, it shouldn't be difficult. I just had these profile results handy. As you can see, the allocation overhead of not reusing the send buffers means the server is spending a lot its time collecting them instead of doing meaningful work. By reusing the send buffers, the performance drastically increases

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants