-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Use pooled gzip.{Writer,Reader} in gzip{Compressor,Decompressor} #1217
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed, please reply here (e.g.
|
I signed it! |
CLAs look good, thanks! |
On a side note, this fixed the GC issues we had in production. Processes went from pretty much doing GC all the time back to a normal pattern. Scheduling was also significantly improved as traces showed up to 800 runable goroutines all the time down to ~20 during GC, and 0 most of the time. |
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.
Thanks for the contribution.
LGTM
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.
Whoops, I just noticed the test failure. TestCompress is instantiating its own compressor, so it should be changed to use NewGZIPCompressor() instead.
This change saves a lot of memory by reusing the underlying gzip.{Writer,Reader}, which allocates up to 1.4mb at every instanciation according to [1]. This was fixed by adding a Reset method by to the object at [2]. The amount of memory (and GC time) saved is pretty high, as reported by pprof: flat flat% sum% cum cum% 28.33GB 85.70% 85.70% 32.74GB 99.05% compress/flate.NewWriter flat flat% sum% cum cum% 19.39MB 16.74% 16.74% 22.07MB 19.05% compress/flate.NewWriter And the benchmarks: benchmark old ns/op new ns/op delta BenchmarkGZIPCompressor1B-4 215170 22291 -89.64% BenchmarkGZIPCompressor1KiB-4 225971 27213 -87.96% BenchmarkGZIPCompressor8KiB-4 246696 54785 -77.79% BenchmarkGZIPCompressor64KiB-4 444851 286924 -35.50% BenchmarkGZIPCompressor512KiB-4 2279043 2115863 -7.16% BenchmarkGZIPCompressor1MiB-4 4412989 4258635 -3.50% benchmark old allocs new allocs delta BenchmarkGZIPCompressor1B-4 17 0 -100.00% BenchmarkGZIPCompressor1KiB-4 17 0 -100.00% BenchmarkGZIPCompressor8KiB-4 17 0 -100.00% BenchmarkGZIPCompressor64KiB-4 17 0 -100.00% BenchmarkGZIPCompressor512KiB-4 17 0 -100.00% BenchmarkGZIPCompressor1MiB-4 17 0 -100.00% benchmark old bytes new bytes delta BenchmarkGZIPCompressor1B-4 813872 8 -100.00% BenchmarkGZIPCompressor1KiB-4 813872 16 -100.00% BenchmarkGZIPCompressor8KiB-4 813875 27 -100.00% BenchmarkGZIPCompressor64KiB-4 813918 190 -99.98% BenchmarkGZIPCompressor512KiB-4 814928 1871 -99.77% BenchmarkGZIPCompressor1MiB-4 820889 9735 -98.81% [1] golang/go#6138 [2] golang/go@db12f9d Signed-off-by: Steeve Morin <[email protected]>
Seems it was not pushed, fixed |
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.
Thanks for the fix!
This change saves a lot of memory by reusing the underlying
gzip.{Writer,Reader}
, which allocates up to 1.4mb at every instanciationaccording to [1]. This was fixed by adding a Reset method by to the
object at [2].
The amount of memory (and GC time) saved is pretty high, as reported by
pprof
for the benchmarks below:And the benchmarks:
[1] golang/go#6138
[2] golang/go@db12f9d