Skip to content

compress/flate: flate.NewReader allocates too much memory when decoding small buffers #7950

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.

Sign up for GitHub

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

Jump to bottom
Closed
gopherbot opened this issue May 7, 2014 · 6 comments
Closed

compress/flate: flate.NewReader allocates too much memory when decoding small buffers #7950

gopherbot opened this issue May 7, 2014 · 6 comments
Labels
FrozenDueToAge
Milestone
Go1.4

Comments

@gopherbot
Copy link
Contributor

by jamesr.gatech:

I have a program that processes git repositories which are composed of git objects
(commits, files, metadata) stored as deflate compressed buffers.  Many common operations
require decompressing lots of small buffers, around a few hundred bytes.  For example,
performing the operation 'git log --first-parent 779792a5f2' on the main git repository,
which shows a linearized history of the repo, requires inflating 13480 buffers with an
average size of 535.9 bytes each (inflated).

The current flate.NewReader() implementation allocates just over 40kb per reader most of
which is for the history buffer which is 32768 bytes allocated here:
https://code.google.com/p/go/source/browse/src/pkg/compress/flate/inflate.go#689

Allocating 40kb to inflate 500 bytes is pretty inefficient.  In my program, allocating
these readers and the GCs these allocations that causes are responsible for the majority
of CPU time of the overall program and 25% of the total execution time (the program
should be I/O bound and mostly is other than this).  I'm testing on a 2.3 GHz Core i7
Mac Book Pro with go 1.2.1 darwin/amd64.
@ianlancetaylor
Copy link
Member

Comment 1:

Labels changed: added repo-main, release-go1.4.

@gopherbot
Copy link
Contributor Author

Comment 2 by jamesr.gatech:

Patch that reuses buffers posted here: https://golang.org/cl/97140043/
Other approaches I thought of were dynamically resizing the history buffer as needed,
although that runs a greater risk of hurting perf in the large buffer case, or coming up
with a way for the caller to reuse readers or at least reuse memory for this large
buffer.

@gopherbot
Copy link
Contributor Author

Comment 3:

CL https://golang.org/cl/97140043 mentions this issue.

@gopherbot
Copy link
Contributor Author

Comment 4 by jamesr.gatech:

issue #6317 appears to be closely related.  Based on the discussion in issue #6086, seems
like the best approach will be to have a Reset() on flate.Reader and use that to manage
whatever buffer pooling is appropriate for different users of the interface.

@nigeltao
Copy link
Contributor

Comment 5:

This issue was closed by revision 193d09a.

Status changed to Fixed.

@gopherbot
Copy link
Contributor Author

Comment 6 by jamesr.gatech:

Thanks for the help, Nigel and everyone else on the review!  My program now takes 331ms
instead of 819ms to go through git.git's commit history.

@rsc rsc added this to the Go1.4 milestone Apr 14, 2015
@rsc rsc removed the release-go1.4 label Apr 14, 2015
@golang golang locked and limited conversation to collaborators Jun 25, 2016
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jun 25, 2018
@jamesr @wheatman
compress/flate: add Reset() to allow reusing large buffers to compres…
c2ac1ab 
…s multiple buffers

This adds a Reset() to compress/flate's decompressor and plumbs that through
to compress/zlib and compress/gzip's Readers so callers can avoid large
allocations when performing many inflate operations. In particular this
preserves the allocation of the decompressor.hist buffer, which is 32kb and
overwritten as needed while inflating.

On the benchmark described in issue 6317, produces the following speedup on
my 2.3ghz Intel Core i7 MBP with go version devel +6b696a34e0af Sun Aug 03
15:14:59 2014 -0700 darwin/amd64:

blocked.text w/out patch vs blocked.text w/ patch:
benchmark           old ns/op      new ns/op      delta
BenchmarkGunzip     8371577533     7927917687     -5.30%

benchmark           old allocs     new allocs     delta
BenchmarkGunzip     176818         148519         -16.00%

benchmark           old bytes     new bytes     delta
BenchmarkGunzip     292184936     12739528      -95.64%

flat.text vs blocked.text w/patch:
benchmark           old ns/op      new ns/op      delta
BenchmarkGunzip     7939447827     7927917687     -0.15%

benchmark           old allocs     new allocs     delta
BenchmarkGunzip     90702          148519         +63.74%

benchmark           old bytes     new bytes     delta
BenchmarkGunzip     9959528       12739528      +27.91%

Similar speedups to those bradfitz saw in  https://golang.org/cl/13416045.

Fixes golang#6317.
Fixes golang#7950.

LGTM=nigeltao
R=golang-codereviews, bradfitz, dan.kortschak, adg, nigeltao, jamesr
CC=golang-codereviews
https://golang.org/cl/97140043
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jun 26, 2018
@jamesr @wheatman
compress/flate: add Reset() to allow reusing large buffers to compres…
add7a71 
…s multiple buffers

This adds a Reset() to compress/flate's decompressor and plumbs that through
to compress/zlib and compress/gzip's Readers so callers can avoid large
allocations when performing many inflate operations. In particular this
preserves the allocation of the decompressor.hist buffer, which is 32kb and
overwritten as needed while inflating.

On the benchmark described in issue 6317, produces the following speedup on
my 2.3ghz Intel Core i7 MBP with go version devel +6b696a34e0af Sun Aug 03
15:14:59 2014 -0700 darwin/amd64:

blocked.text w/out patch vs blocked.text w/ patch:
benchmark           old ns/op      new ns/op      delta
BenchmarkGunzip     8371577533     7927917687     -5.30%

benchmark           old allocs     new allocs     delta
BenchmarkGunzip     176818         148519         -16.00%

benchmark           old bytes     new bytes     delta
BenchmarkGunzip     292184936     12739528      -95.64%

flat.text vs blocked.text w/patch:
benchmark           old ns/op      new ns/op      delta
BenchmarkGunzip     7939447827     7927917687     -0.15%

benchmark           old allocs     new allocs     delta
BenchmarkGunzip     90702          148519         +63.74%

benchmark           old bytes     new bytes     delta
BenchmarkGunzip     9959528       12739528      +27.91%

Similar speedups to those bradfitz saw in  https://golang.org/cl/13416045.

Fixes golang#6317.
Fixes golang#7950.

LGTM=nigeltao
R=golang-codereviews, bradfitz, dan.kortschak, adg, nigeltao, jamesr
CC=golang-codereviews
https://golang.org/cl/97140043
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jul 9, 2018
@jamesr @wheatman
compress/flate: add Reset() to allow reusing large buffers to compres…
3457bc6 
…s multiple buffers

This adds a Reset() to compress/flate's decompressor and plumbs that through
to compress/zlib and compress/gzip's Readers so callers can avoid large
allocations when performing many inflate operations. In particular this
preserves the allocation of the decompressor.hist buffer, which is 32kb and
overwritten as needed while inflating.

On the benchmark described in issue 6317, produces the following speedup on
my 2.3ghz Intel Core i7 MBP with go version devel +6b696a34e0af Sun Aug 03
15:14:59 2014 -0700 darwin/amd64:

blocked.text w/out patch vs blocked.text w/ patch:
benchmark           old ns/op      new ns/op      delta
BenchmarkGunzip     8371577533     7927917687     -5.30%

benchmark           old allocs     new allocs     delta
BenchmarkGunzip     176818         148519         -16.00%

benchmark           old bytes     new bytes     delta
BenchmarkGunzip     292184936     12739528      -95.64%

flat.text vs blocked.text w/patch:
benchmark           old ns/op      new ns/op      delta
BenchmarkGunzip     7939447827     7927917687     -0.15%

benchmark           old allocs     new allocs     delta
BenchmarkGunzip     90702          148519         +63.74%

benchmark           old bytes     new bytes     delta
BenchmarkGunzip     9959528       12739528      +27.91%

Similar speedups to those bradfitz saw in  https://golang.org/cl/13416045.

Fixes golang#6317.
Fixes golang#7950.

LGTM=nigeltao
R=golang-codereviews, bradfitz, dan.kortschak, adg, nigeltao, jamesr
CC=golang-codereviews
https://golang.org/cl/97140043
wheatman pushed a commit to wheatman/go-akaros that referenced this issue Jul 30, 2018
@jamesr @wheatman
compress/flate: add Reset() to allow reusing large buffers to compres…
8acaa45 
…s multiple buffers

This adds a Reset() to compress/flate's decompressor and plumbs that through
to compress/zlib and compress/gzip's Readers so callers can avoid large
allocations when performing many inflate operations. In particular this
preserves the allocation of the decompressor.hist buffer, which is 32kb and
overwritten as needed while inflating.

On the benchmark described in issue 6317, produces the following speedup on
my 2.3ghz Intel Core i7 MBP with go version devel +6b696a34e0af Sun Aug 03
15:14:59 2014 -0700 darwin/amd64:

blocked.text w/out patch vs blocked.text w/ patch:
benchmark           old ns/op      new ns/op      delta
BenchmarkGunzip     8371577533     7927917687     -5.30%

benchmark           old allocs     new allocs     delta
BenchmarkGunzip     176818         148519         -16.00%

benchmark           old bytes     new bytes     delta
BenchmarkGunzip     292184936     12739528      -95.64%

flat.text vs blocked.text w/patch:
benchmark           old ns/op      new ns/op      delta
BenchmarkGunzip     7939447827     7927917687     -0.15%

benchmark           old allocs     new allocs     delta
BenchmarkGunzip     90702          148519         +63.74%

benchmark           old bytes     new bytes     delta
BenchmarkGunzip     9959528       12739528      +27.91%

Similar speedups to those bradfitz saw in  https://golang.org/cl/13416045.

Fixes golang#6317.
Fixes golang#7950.

LGTM=nigeltao
R=golang-codereviews, bradfitz, dan.kortschak, adg, nigeltao, jamesr
CC=golang-codereviews
https://golang.org/cl/97140043
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.4
Development

No branches or pull requests
4 participants
@rsc @ianlancetaylor @nigeltao @gopherbot