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
Reset bytes.Buffer before returning it to sync.Pool #448
Conversation
Most of bytes.Buffer in stargz-snapshotter is unbounded in terms of size and sync.Pool's Get may choose to ignore the pool. Resizing a buffer before returning to the pool may alleviate the snapshotter's memory utilization issue. golang/go#23199 has a long discussion about the issue. Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
return nil | ||
}, | ||
} | ||
|
||
return memW, nil | ||
} | ||
|
||
func (dc *directoryCache) putBuffer(b *bytes.Buffer) { | ||
b.Reset() |
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.
Note that calling Reset() is the safest but may not be the most efficient solution.
Go's standard library has some more sophisticated examples.
@@ -406,11 +411,10 @@ func (sf *file) ReadAt(p []byte, offset int64) (int, error) { | |||
|
|||
// Use temporally buffer for aligning this chunk | |||
b := sf.gr.bufPool.Get().(*bytes.Buffer) | |||
b.Reset() |
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.
Could we remain Reset
before use for safety?
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.
Sure, but would it have real effects? Get() returns either a new Buffer, which is safe to use according to https://cs.opensource.google/go/go/+/refs/tags/go1.17:src/bytes/buffer.go;l=449
In most cases, new(Buffer) (or just declaring a Buffer variable) is sufficient to initialize a Buffer.
or a buffer which is put by putBuffer().
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.
Sorry, I think you are right. Merging.
Most of bytes.Buffer in stargz-snapshotter is unbounded in terms of
size and sync.Pool's Get may choose to ignore the pool.
Resizing a buffer before returning to the pool may alleviate the
snapshotter's memory utilization issue.
golang/go#23199 has a long discussion about
the issue.
Signed-off-by: Kazuyoshi Kato katokazu@amazon.com