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

fix: reset frame when read to EOF to release buffer in Block #220

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

Conversation

aiquestion
Copy link

@aiquestion aiquestion commented Aug 19, 2024

We use reader to decompress with no pooling object, it's some code like:

func Decompress(compressed []byte) ([]byte, error) {
	if len(compressed) == 0 {
		return compressed, nil
	}
	destBuffer := bytes.Buffer{}
	bytesReader := bytes.NewReader(compressed)
	lz4Reader := lz4.NewReader(bytesReader)
	if _, err := destBuffer.ReadFrom(lz4Reader); err != nil {
		return nil, err
	}
	return destBuffer.Bytes(), nil
}

and we found a big performace regression when we upgrade to v4.1.21 (from v4.0.2), after some test we narrow down the change from v4.1.0 -> v4.1.1.

It's seems that the code do BlockSizeIndex.Get() twice and only call Put() to return the buffer 1 time. It look like that the buffer from this stack is not released.

lz4block.BlockSizeIndex.Get (blocks.go:54) github.com/pierrec/lz4/v4/internal/lz4block
lz4stream.NewFrameDataBlock (block.go:197) github.com/pierrec/lz4/v4/internal/lz4stream
lz4stream.(*Blocks).initR (block.go:99) github.com/pierrec/lz4/v4/internal/lz4stream
lz4stream.(*Frame).InitR (frame.go:114) github.com/pierrec/lz4/v4/internal/lz4stream
lz4.(*Reader).init (reader.go:91) github.com/pierrec/lz4/v4
lz4.(*Reader).Read (reader.go:111) github.com/pierrec/lz4/v4
bytes.(*Buffer).ReadFrom (buffer.go:211) bytes

So here i added a Reset when read to EOF, to release the underlying buffer in Block.
Added a Benchmark test:

Before:
BenchmarkReaderNoReset-12    	  258943	      4354 ns/op	   70309 B/op	      12 allocs/op

After:
BenchmarkReaderNoReset-12    	 1136524	      1031 ns/op	    4226 B/op	      11 allocs/op

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.

1 participant