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

store-gateway: reduce garbage from index header #10715

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bboreham
Copy link
Contributor

What this PR does

This one call to io.ReadAll was generating 6% of all garbage in some production store-gateways recently.

Since gzip puts the size at the end of the compressed data, we can use that size to pre-allocate the buffer and avoid resizing.
Code is adapted from some that used to be inside gRPC, from grpc/grpc-go#3048

It is annoying to add 16 lines of code for this benefit, so I looked at whether we could have a utility function doing it.

  • pkg/util/http.go has similar code but also handles LZ4.
  • pkg/distributor/otel.go also handles gzip and LZ4, and pools the buffers.

Both of the above, and some other similar places, have a maximum size they will read.

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

This one call to io.ReadAll was generating 6% of all garbage in some
production store-gateways recently.

Since gzip puts the size at the end of the compressed data, we can use
that size to pre-allocate the buffer and avoid resizing.
@bboreham bboreham requested a review from a team as a code owner February 21, 2025 11:55
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

this looks good in general. Can't we change util.ParseProtoReader to do this estimation? It currently takes expectedSize I guess we can take -1 and let it try to estimate the size based on the method here. Do you want to try that or should I?

@jhalterman
Copy link
Member

jhalterman commented Feb 21, 2025

I don't have an opinion on having a separate function vs updating the existing one, but if we do have a new function, can it also live in the util package for better reuse?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants