Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(7)

Issue 97140043: code review 97140043: compress/flate: add Reset() to allow reusing large buffers to compress multipl

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 8 months ago by james.robinson
Modified:
5 years, 3 months ago
Reviewers:
nigeltao, rsc
CC:
golang-codereviews, bradfitz, kortschak, adg, nigeltao, jamesr1
Visibility:
Public.

Description

compress/flate: add Reset() to allow reusing large buffers to compress 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://codereview.appspot.com/13416045. Fixes issue 6317. Fixes issue 7950.

Patch Set 1 #

Patch Set 2 : diff -r e473e77e84ff https://code.google.com/p/go #

Patch Set 3 : diff -r e473e77e84ff https://code.google.com/p/go #

Patch Set 4 : diff -r e473e77e84ff https://code.google.com/p/go #

Patch Set 5 : diff -r 6b696a34e0af https://code.google.com/p/go #

Patch Set 6 : diff -r 6b696a34e0af https://code.google.com/p/go #

Patch Set 7 : diff -r 6b696a34e0af https://code.google.com/p/go #

Total comments: 1

Patch Set 8 : diff -r e1a081e6ddf8a77b267eb0d2b9747b2181524106 https://code.google.com/p/go #

Patch Set 9 : diff -r e1a081e6ddf8a77b267eb0d2b9747b2181524106 https://code.google.com/p/go #

Total comments: 23

Patch Set 10 : diff -r e1a081e6ddf8a77b267eb0d2b9747b2181524106 https://code.google.com/p/go #

Total comments: 7

Patch Set 11 : diff -r e1a081e6ddf8a77b267eb0d2b9747b2181524106 https://code.google.com/p/go #

Total comments: 2

Patch Set 12 : diff -r e1a081e6ddf8a77b267eb0d2b9747b2181524106 https://code.google.com/p/go #

Total comments: 14

Patch Set 13 : diff -r e1a081e6ddf8a77b267eb0d2b9747b2181524106 https://code.google.com/p/go #

Patch Set 14 : diff -r b91b31b57771240403a02a9257fbf8f0c6ea5f09 https://code.google.com/p/go #

Total comments: 6

Patch Set 15 : diff -r b91b31b57771240403a02a9257fbf8f0c6ea5f09 https://code.google.com/p/go #

Unified diffs Side-by-side diffs Delta from patch set Stats (+125 lines, -27 lines) Patch
M src/compress/flate/inflate.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +27 lines, -0 lines 0 comments Download
M src/compress/flate/inflate_test.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +39 lines, -0 lines 0 comments Download
M src/compress/gzip/gunzip.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +5 lines, -1 line 0 comments Download
M src/compress/zlib/reader.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +54 lines, -26 lines 0 comments Download

Messages

Total messages: 34
james.robinson
Hello golang-dev@googlegroups.com (cc: golang-codereviews@googlegroups.com, nigeltao@golang.org), I'd like you to review this change to https://code.google.com/p/go
5 years, 8 months ago (2014-05-07 21:14:04 UTC) #1
gobot
Replacing golang-dev with golang-codereviews. To the author of this CL: If you are using 'hg ...
5 years, 8 months ago (2014-05-07 21:18:02 UTC) #2
bradfitz
The tree is in stabilization mode until Go 1.3 is out. (We intentionally don't have ...
5 years, 8 months ago (2014-05-07 22:00:37 UTC) #3
james.robinson
On 2014/05/07 22:00:37, bradfitz wrote: > > But please research first related attempts like > ...
5 years, 8 months ago (2014-05-08 00:02:43 UTC) #4
kortschak
There was also some relevant discussion here https://codereview.appspot.com/13512052 Thanks for working on this. On Thu, ...
5 years, 8 months ago (2014-05-08 01:48:02 UTC) #5
james.robinson
Will ping again for formal review post 1.3, but since this proved promising throwing the ...
5 years, 8 months ago (2014-05-08 05:55:30 UTC) #6
james.robinson
PTAL at the latest patchset. It adds a Reset() to the reader types provided by ...
5 years, 5 months ago (2014-08-04 20:54:06 UTC) #7
bradfitz
https://codereview.appspot.com/97140043/diff/120001/src/pkg/archive/zip/register.go File src/pkg/archive/zip/register.go (right): https://codereview.appspot.com/97140043/diff/120001/src/pkg/archive/zip/register.go#newcode23 src/pkg/archive/zip/register.go:23: type Decompressor func(io.Reader) flate.InflateReader not acceptable. this is an ...
5 years, 5 months ago (2014-08-04 23:56:02 UTC) #8
james.robinson
On 2014/08/04 23:56:02, bradfitz wrote: > https://codereview.appspot.com/97140043/diff/120001/src/pkg/archive/zip/register.go > File src/pkg/archive/zip/register.go (right): > > https://codereview.appspot.com/97140043/diff/120001/src/pkg/archive/zip/register.go#newcode23 > ...
5 years, 5 months ago (2014-08-06 18:10:30 UTC) #9
kortschak
On 2014/08/06 18:10:30, james.robinson wrote: > Ah yes, in all the syscall spam I missed ...
5 years, 5 months ago (2014-08-07 00:30:56 UTC) #10
james.robinson
PTAL and sorry for the extreme delay. This patch adds a new interface flate.Resettable with ...
5 years, 3 months ago (2014-09-27 20:29:22 UTC) #11
adg
Hi, this will have to wait until after December 1 when the release freeze is ...
5 years, 3 months ago (2014-09-28 09:01:14 UTC) #12
bradfitz
I thought there were open bugs for this? On Sun, Sep 28, 2014 at 2:00 ...
5 years, 3 months ago (2014-09-28 16:42:25 UTC) #13
james.robinson
On 2014/09/28 16:42:25, bradfitz wrote: > I thought there were open bugs for this? > ...
5 years, 3 months ago (2014-09-28 20:37:42 UTC) #14
kortschak
https://codereview.appspot.com/97140043/diff/160001/src/compress/flate/inflate.go File src/compress/flate/inflate.go (right): https://codereview.appspot.com/97140043/diff/160001/src/compress/flate/inflate.go#newcode59 src/compress/flate/inflate.go:59: // This interface allows resetting a reader to read ...
5 years, 3 months ago (2014-09-28 23:33:55 UTC) #15
bradfitz
https://codereview.appspot.com/97140043/diff/160001/src/compress/flate/inflate.go File src/compress/flate/inflate.go (right): https://codereview.appspot.com/97140043/diff/160001/src/compress/flate/inflate.go#newcode59 src/compress/flate/inflate.go:59: // This interface allows resetting a reader to read ...
5 years, 3 months ago (2014-09-29 18:39:50 UTC) #16
bradfitz
https://codereview.appspot.com/97140043/diff/160001/src/compress/zlib/reader.go File src/compress/zlib/reader.go (right): https://codereview.appspot.com/97140043/diff/160001/src/compress/zlib/reader.go#newcode68 src/compress/zlib/reader.go:68: func NewReaderEx(r io.Reader) (*Reader, error) { we're not adding ...
5 years, 3 months ago (2014-09-29 18:41:16 UTC) #17
james.robinson
Thanks for all the review comments, as you can tell I'm still learning things here. ...
5 years, 3 months ago (2014-09-30 05:33:19 UTC) #18
kortschak
https://codereview.appspot.com/97140043/diff/180001/src/compress/flate/inflate.go File src/compress/flate/inflate.go (right): https://codereview.appspot.com/97140043/diff/180001/src/compress/flate/inflate.go#newcode62 src/compress/flate/inflate.go:62: type Resettable interface { s/Resettable/Resetter/g https://codereview.appspot.com/97140043/diff/180001/src/compress/gzip/gunzip.go File src/compress/gzip/gunzip.go (right): ...
5 years, 3 months ago (2014-09-30 12:09:44 UTC) #19
james.robinson
I've updated everything to 'Resetter', PTAL. https://codereview.appspot.com/97140043/diff/180001/src/compress/zlib/reader.go File src/compress/zlib/reader.go (right): https://codereview.appspot.com/97140043/diff/180001/src/compress/zlib/reader.go#newcode54 src/compress/zlib/reader.go:54: // Resetter resets ...
5 years, 3 months ago (2014-10-03 04:37:52 UTC) #20
kortschak
https://codereview.appspot.com/97140043/diff/180001/src/compress/zlib/reader.go File src/compress/zlib/reader.go (right): https://codereview.appspot.com/97140043/diff/180001/src/compress/zlib/reader.go#newcode54 src/compress/zlib/reader.go:54: // Resetter resets a ReadCloser returned by NewReader or ...
5 years, 3 months ago (2014-10-03 04:53:52 UTC) #21
james.robinson
Brad - Dan and I are starting to go back and forth about which of ...
5 years, 3 months ago (2014-10-03 05:06:57 UTC) #22
james.robinson
Ping
5 years, 3 months ago (2014-10-09 04:47:24 UTC) #23
bradfitz
Sorry, I can't look at this for another couple days at least. Perhaps Nigel can ...
5 years, 3 months ago (2014-10-09 20:48:35 UTC) #24
nigeltao
I haven't been following this code review too closely, as there already seemed to be ...
5 years, 3 months ago (2014-10-10 02:38:10 UTC) #25
nigeltao
https://codereview.appspot.com/97140043/diff/220001/src/compress/gzip/gunzip_test.go File src/compress/gzip/gunzip_test.go (right): https://codereview.appspot.com/97140043/diff/220001/src/compress/gzip/gunzip_test.go#newcode381 src/compress/gzip/gunzip_test.go:381: fmt.Fprintln(os.Stderr, err) b.Fatal(err)
5 years, 3 months ago (2014-10-10 02:40:35 UTC) #26
james.robinson
Thanks Nigel. The main issue that I think needs resolution is about whether to add ...
5 years, 3 months ago (2014-10-10 22:48:14 UTC) #27
nigeltao
I'm OK with having two separate but equivalent interfaces in two separate packages. I'd like ...
5 years, 3 months ago (2014-10-13 07:12:57 UTC) #28
james.robinson
https://codereview.appspot.com/97140043/diff/260001/src/compress/flate/inflate.go File src/compress/flate/inflate.go (right): https://codereview.appspot.com/97140043/diff/260001/src/compress/flate/inflate.go#newcode65 src/compress/flate/inflate.go:65: Reset(r io.Reader, dict []byte) On 2014/10/13 07:12:57, nigeltao wrote: ...
5 years, 3 months ago (2014-10-15 07:19:33 UTC) #29
nigeltao
LGTM. Have you (or your organization) submitted a contributor license agreement yet? https://golang.org/doc/contribute.html#copyright
5 years, 3 months ago (2014-10-17 06:29:53 UTC) #30
james.robinson
I'm a google employee, so the copyright is already google's. I started on this work ...
5 years, 3 months ago (2014-10-17 06:44:51 UTC) #31
jamesr1
On 2014/10/17 06:44:51, james.robinson wrote: > I'm a google employee, so the copyright is already ...
5 years, 3 months ago (2014-10-17 06:55:42 UTC) #32
nigeltao
*** Submitted as https://code.google.com/p/go/source/detail?r=e1e2e3c3cdb0 *** compress/flate: add Reset() to allow reusing large buffers to compress ...
5 years, 3 months ago (2014-10-20 01:58:22 UTC) #33
rsc
5 years, 3 months ago (2014-10-20 20:49:18 UTC) #34
For the record, the CL description is wrong.
s/compress/decompress/
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b