Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
gzip: Use sync.pool #1080
base: master
Are you sure you want to change the base?
gzip: Use sync.pool #1080
Changes from all commits
12fa00d
9e9222e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 Reset() failed below, are we sure we want to put this back in the pool?
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.
Hmm thats a good question. Im looking at the Reset code, and I think it just calls bufio.NewReader(r) to reset it back. So the put call is really just putting the base gzip.Reader{} object back and reset internally resets z.r which is the actual reader.
Does that check out with your understand of reset as well?
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.
I've never looked at the inside of Reset before today :D.
Looking at this code, I'd be a little concerned that this could result in us keeping
data
alive for longer than is necessary because the gzipReader will still have a reference to it.Given that your profile only shows the writer being a problem, I wonder if it would be better to start by just pooling the writer's and not the readers.
But don't feel a need to block on my feedback 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.
It's a good call out. The reader doesn't show up in this profile since I think the other things dominated it.
We're doing a lot of profile work on this now, so I can certainly test that as well. The new heap profiles after this pool change don't seem to point to a big issue of
data
living for longer than necessary. But it could also be that GC is running very aggressively due to other poorly managed allocations.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.
For a reference, here is a new CPU profile taken after teh pooling changes. Im highlighting over where the reader shows up which is much smaller that the writer. But certainly there are way more places to look a for bad allocations or things remaining on the heap leading to tha giant GC block still.