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

Use exact size, if known, to allocate decompression buffer #3048

Merged
merged 5 commits into from Oct 4, 2019

Conversation

bboreham
Copy link
Contributor

Fixes #2635

For large messages this generates far less garbage than ioutil.ReadAll().

Implement for gzip - RFC1952 requires it, and the Go implementation checks it already (modulo 2^32).

For large messages this generates far less garbage than ioutil.ReadAll().

Implement for gzip - RFC1952 requires it, and the Go implementation
checks it already (modulo 2^32).

Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

Thanks for the new version, this is even better!

encoding/gzip/gzip.go Outdated Show resolved Hide resolved
encoding/encoding.go Outdated Show resolved Hide resolved
encoding/gzip/gzip.go Outdated Show resolved Hide resolved
Return -1 instead of an error in the case that we can't determine a size.

Use a LimitReader set to size+1 so we can detect cases like corrupt
messages and overflow of the 32-bit size field.

Signed-off-by: Bryan Boreham <bryan@weave.works>
Based on review feedback

Signed-off-by: Bryan Boreham <bryan@weave.works>
@bboreham
Copy link
Contributor Author

I pushed a couple more commits which I think cover all the review comments.

@dfawley dfawley self-assigned this Oct 3, 2019
rpc_util.go Outdated Show resolved Hide resolved
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Copy link
Member

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

LGTM. One last minor change to request for initializing buf first, though. Thanks!

rpc_util.go Outdated Show resolved Hide resolved
It's simpler.

Signed-off-by: Bryan Boreham <bryan@weave.works>
@dfawley
Copy link
Member

dfawley commented Oct 4, 2019

Thanks for the contribution!

@dfawley dfawley merged commit dcd1c97 into grpc:master Oct 4, 2019
bboreham added a commit to cortexproject/cortex that referenced this pull request Nov 8, 2019
This brings in some performance improvements, including
grpc/grpc-go#3048 which should make a big
difference when using gRPC gzip compression.

Plus some updated dependencies.

Signed-off-by: Bryan Boreham <bryan@weave.works>
@lock lock bot locked as resolved and limited conversation to collaborators Apr 2, 2020
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.

Use of ioutil.ReadAll in decompression generates lots of garbage
2 participants