-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Use sync.Pool to share per-connection transport write buffer. #6309
Conversation
internal/transport/http_util.go
Outdated
} | ||
writeBufferPoolMap[size] = pool | ||
} | ||
writeBufferMutex.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory, we can get rid of this mutex and pool map if we store sync.Pool per ClientConnection and per server. However, this will require modifying a lot of internal interface to pass the pool all the way down to the transport layer. This also will prevent us from sharing the pool between different ClientConnections and servers, so I decided to take the mutex approach.
As far as I understand, this mutex shouldn't really impact the performance, as it got called only on new connection creation, which happens not very often in http2.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When do entries from this map ever get deleted? Can this map grow in a unbounded fashion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right now - never. In theory it can grow unbounded if the app creates multiple servers and/or ClientConnections with different buffer sizes. I can't think about any use-case where this could be useful, but it is possible. I am not sure if it worth adding more complicated logic to protect against this, because my understanding is that go runtime will eventually deallocate all memory from unused pools, so we will be waisting memory only on storing pointers to empty pools, which should be neglectable in practise.
Still I can fix this if you prefer:
- I can add a goroutine that removes unused pools. I'll probably need to store something like lastModifedTime for each pool to do that. The downsides are some additional complexity + the fact that we need to take the lock in the goroutine.
- I can store a pool per Server and ClientConnection and propagate it all the way down to the transport layer. The downsides are the fact that we need to extend some internal interfaces + we won't be able to reuse pools between different connections that uses the same buffer size (which I assume is a very common use-case) But in this case we can get rid of the lock completely.
Do you have any other ideas? Let me know what I should do here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In theory it can grow unbounded if the app creates multiple servers and/or ClientConnections with different buffer sizes. I can't think about any use-case where this could be useful, but it is possible.
I agree. I don't expect applications to create multiple grpc.ClientConn
s or grpc.Server
s and pass them different values for the send and receive buffer sizes. But given that we are going to have a dial option and a server option to enable the sync.Pool on the client and server respectively, we could document it there, just so no one complains saying a few bytes are leaked.
@dfawley : Thoughts/Objections?
I was trying to test whether this change causes any significant CPU/latency regressions and run benchmarks on a dedicated 8 CPU 32GB virtual machine. I used #6299 with the following parameters
Here are the results. |
Unfortunately with @arvindbr8 and @easwars out of town we are a bit short staffed right now. Will resume in ~2 weeks if that's okay, thanks! |
Sounds good! Thanks for the update. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are your thoughts about making this an opt-in feature, i.e, we could have a dial option which the user would have to set for them to get this behavior. If the dial option is not set, the user would get the old behavior.
At a later point in time, we could get rid of the dial option completely and make this the default behavior, or continue to have the dial option (but make the use of the sync.Pool the default one, and the user would have to use the dial option to explicitly disable it).
internal/transport/http_util.go
Outdated
} | ||
writeBufferPoolMap[size] = pool | ||
} | ||
writeBufferMutex.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When do entries from this map ever get deleted? Can this map grow in a unbounded fashion?
Sure, will do that. If we are not enabling this by default, what do you think if I add a similar fix for the read buffer in the same PR? I can use a single option to enable both. |
Yes, totally. And if we are going to have it for the server as well, we need a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a small vet failure with the docstring not starting with the name of the function. Could you please take care of that. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! Just a couple minor things before we can merge it.
https://pkg.go.dev/google.golang.org/grpc#WithSharedWriteBuffer
not release, but reuse ? |
release, so it can be reused by other connections. |
Fixes siderolabs#7576 See grpc/grpc-go#6309 Signed-off-by: Andrey Smirnov <[email protected]>
Fixes siderolabs#7576 See grpc/grpc-go#6309 Signed-off-by: Andrey Smirnov <[email protected]>
func getWriteBufferPool(writeBufferSize int) *sync.Pool { | ||
writeBufferMutex.Lock() | ||
defer writeBufferMutex.Unlock() | ||
size := writeBufferSize * 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the significance of the * 2
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC this matches the legacy behavior, which always doubled for reasons nobody knows but we don't want to change now due to it affecting existing applications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair. Do you mind expanding on the legacy behavior you are describing? It seems like if there is no shared buffer pool, the buffer is allocated per the input size. It only seems to be in the case of the buffer pool that we're allocating 2x the size (and this seems to be the PR the write buffer pool was added)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may be right.. The old code multiplied by 2 when doing this allocation, so it seems we have changed the behavior for legacy users already:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - it just felt somewhat bizarre because this is the very PR that also removed that same * 2
in the default path (see line 321 above), but added the * 2
for the buffer pool path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think it's worth updating, I created #6983
This is an attempt to fix #5759.
The use-case where this fix could be especially useful is a server with a lot of mostly idle connections (like connections that handle only healthchecks) In our infrastructure this use-case is very common, as we have some servers that handle request from thousands of clients, all of which generate relatively small volume of requests but has an active healthcheck setup, which forces every client connect to every server. Some kind of subsetting would be useful in such scenario, but gRPC doesn't really implement subsetting. Grpc-go isn't great at handling memory in such use-case. Right now a lot of our users put envoy in front of grpc server just because it handles memory per connection a lot more efficiently (in one case we were able to reduce the memory consumed by an app from 1.5 GB to 500 MB just by putting envoy in front of the grpc server)
This PR adds sync.Pool to the transport layer to share the write buffer between connections (I plan to provide a similar fix for the read buffer if this PR will be accepted) The buffer is claimed in the Write method and released on Flush, with an additional optimization that preserves the buffer if a single Write results in multiple Flushes.
Testing
I submitted #6299 so that I can reproduce the case described above with benchmarks. I used the following comnand to demonstrate the impact of this change
As you can see, I used tiny requests with a combination of large delays between requests to minimize the impact of other allocations on memory stats.
Here are the results that I got:
Without the optimization:
With the optimization:
As you can see, in the unoptimized benchmark run
newBufWriter
method allocates 59.04MB or 54.39% of the whole memory. In the optimized run we now see this line instead0.53MB 0.96% 91.88% 0.53MB 0.96% google.golang.org/grpc/internal/transport.newFramer.func1
which corresponds to the memory allocated by the sync.Pool that I added to the newFramer function.I also have some results that demonstrates that this change doesn't negatively impact performance in the more realistic benchmark setups, but I will provide those results a bit later.
RELEASE NOTES:
[With]SharedWriteBuffer
to improve performance by reducing allocations when sending RPC messages. (Disabled by default.)