Pre-create larger buffers when uncompressing, to reduce garbage. #3035
Conversation
Buffer is created as a multiple of the size of the compressed data. Add CompressionFactor options on call and server. Signed-off-by: Bryan Boreham <bryan@weave.works>
| @@ -677,11 +698,36 @@ func recvAndDecompress(p *parser, s *transport.Stream, dc Decompressor, maxRecei | |||
| return d, nil | |||
| } | |||
|
|
|||
| // Copied from go/src/io/ioutil/ioutil.go | |||
There was a problem hiding this comment.
This could be problematic, as it will have legal (license) concerns. At the least, we would need to add their license to our repo, which I would rather avoid. Is there another way to implement this?
| // Read from LimitReader with limit max+1. So if the underlying | ||
| // reader is over limit, the result will be bigger than max. | ||
| d, err = ioutil.ReadAll(io.LimitReader(dcReader, int64(maxReceiveMessageSize)+1)) | ||
| d, err = readAll(io.LimitReader(dcReader, int64(maxReceiveMessageSize)+1), expectedSize) |
There was a problem hiding this comment.
If expectedSize > maxReceiveMessageSize, then there would be no reason to allocate a buffer that large.
| @@ -323,6 +324,25 @@ func (o MaxSendMsgSizeCallOption) before(c *callInfo) error { | |||
| } | |||
| func (o MaxSendMsgSizeCallOption) after(c *callInfo) {} | |||
|
|
|||
| // CallCompressionFactor returns a CallOption which sets the predicted compression factor, | |||
| // used in sizing buffers | |||
There was a problem hiding this comment.
Please add:
//
// This API is EXPERIMENTAL.We should call out where this applies (i.e. it's only when decompressing), and how it works (a reasonable interpretation of compression factor could be: "0.5" indicates that compression would halve the input).
| @@ -323,6 +324,25 @@ func (o MaxSendMsgSizeCallOption) before(c *callInfo) error { | |||
| } | |||
| func (o MaxSendMsgSizeCallOption) after(c *callInfo) {} | |||
|
|
|||
| // CallCompressionFactor returns a CallOption which sets the predicted compression factor, | |||
| // used in sizing buffers | |||
| func CallCompressionFactor(f int) CallOption { | |||
There was a problem hiding this comment.
float32/64? To allow more precise values like 1.5?
(I'm also uncertain if the proposed default of 2x is acceptable, given that it means we are now allocating 3x what the client sent, which could potentially be a vulnerability. I'll ask some others for input.)
|
Thank you for the thoughtful review. Thinking around the problem, I realised the gzip protocol sends the uncompressed size as the last four bytes of the message, so all this guesswork is unnecessary. Since that size value could be forged by a malicious sender, and since you mention a potential vulnerability of allocating 3x the input data, I will note that it is fairly straightforward to create a gzip message which expands 100x or more, so gRPC is already vulnerable to such attacks up to |
This is one of the reasons we have this setting, so I'm not too concerned. I think mostly I was concerned that this allocation was ignoring |
|
Replaced by #3048 |
Fixes #2635
The basic idea is that, instead of starting at a fixed size of 512 bytes, the buffer is created as a multiple of the size of the compressed data.
Add
CompressionFactoroptions on call and server for the user to control this multiple.