Implement new Codec that uses mem.BufferSlice instead of []byte
#7356
Conversation
This new codec, as outlined in grpc#6619 will allow the reuse of buffers to their fullest extent. Note that this deliberately does not (yet) implement support for stathandlers, however all the relevant APIs have been updated and both old and new Codec implementations are supported.
… the transport package
| // TODO: Can/should this still be preserved with the new BufferSlice API? Are | ||
| // there any actual benefits to allocating a single large buffer instead of | ||
| // multiple smaller ones? |
There was a problem hiding this comment.
Yes, it is massively more efficient. See #2635, golang/go#50774.
There was a problem hiding this comment.
I have absolutely no doubt that in the context of buffers not being reused, it's more efficient. But in this case where buffers are pooled and produce as little garbage as possible, it may not? I don't know what the default buffer size used by the compressor is, but if it roughly lines up with one of the sized pools used by the mem package, then I'm not sure it would be more efficient to allocate one large buffer instead of representing the data in a mem.BufferSlice. That being said, it's 100% something that can be benchmarked and tested.
There was a problem hiding this comment.
Apologies, I had not caught up with that point.
A quick profile with gRPC 1.67 suggests io.ReadAll is no longer causing excess garbage, thanks!.
There was a problem hiding this comment.
🎉 🎉 🎉
Awesome! That's really good to hear. Good to know this is a win for this new package. Do you think we would actually get a benefit from implementing this TODO? Or would you expect it to actually perform worse? If that's the case, maybe we can get rid of this commented block altogether instead
There was a problem hiding this comment.
Related to this function: #7653. io.Copy() makes lots of allocations - most for my use case.
There was a problem hiding this comment.
Great, I was hoping someone had the answer to that.
Do you think we would actually get a benefit from implementing this TODO?
I cannot think a major benefit right now.
This PR removes the following gRPC buffer pool flags: - `--grpc-server-recv-buffer-pool` - `--grpc-client-recv-buffer-pool` These flags are no longer necessary since gRPC introduced the `mem` package in version [1.66.0](https://github.com/grpc/grpc-go/releases/tag/v1.66.0). See: - https://github.com/grpc/grpc-go/releases/tag/v1.66.0 - grpc/grpc-go#7432 - grpc/grpc-go#7356
|
I think that this change should be more explicitly called out that the memory is being reused by default on the release notes as it can break some edge cases where the memory from the request is still being used even after the request finish. The only think i can see on the release notes about this is: Which does not call out that the feature is enabled by default now. |
This new codec, as outlined in #6619 will allow the reuse of buffers to their fullest extent. Note that this deliberately does not (yet) implement support for stathandlers, however all the relevant APIs have been updated and both old and new Codec implementations are supported.
RELEASE NOTES: