Code review - Issue 13512052: code review 13512052: compress/gzip: add Reset method to Readerhttps://codereview.appspot.com/2014-04-17T02:43:48+00:00rietveld
Message from unknown
2013-09-11T12:01:30+00:00kortschakurn:md5:633bd6358bdb7c1342a657ee2835c5ba
Message from unknown
2013-10-14T23:00:57+00:00kortschakurn:md5:ce06ab5e40c6a15eead4249921b42ddb
Message from unknown
2013-10-14T23:01:25+00:00kortschakurn:md5:e46e3945f931605ab91bde3c145e0dc0
Message from unknown
2013-10-14T23:02:08+00:00kortschakurn:md5:3690253e38744c1db89dc1ecbb8a4e6f
Message from unknown
2013-12-09T21:59:46+00:00kortschakurn:md5:96c47c65c7ab5e2f8fc6b73ee491a8ec
Message from dan.kortschak@adelaide.edu.au
2013-12-09T21:59:55+00:00kortschakurn:md5:59a7a2585b6d0f3a95444d17587752e6
Hello golang-dev@googlegroups.com,
I'd like you to review this change to
https://code.google.com/p/go/
Message from bradfitz@golang.org
2013-12-10T11:55:19+00:00bradfitzurn:md5:cc37c74a9cf20926ed6f286dd2b8cce6
Looks like this still allocates a flate.NewReader each time still? That's
pretty big.
On Tue, Dec 10, 2013 at 1:59 AM, <dan.kortschak@adelaide.edu.au> wrote:
> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> compress/gzip: add Reset method to Reader
>
> Fixes issue 6364.
>
> Please review this at https://codereview.appspot.com/13512052/
>
> Affected files (+31, -0 lines):
> M src/pkg/compress/gzip/gunzip.go
> M src/pkg/compress/gzip/gunzip_test.go
>
>
> Index: src/pkg/compress/gzip/gunzip.go
> ===================================================================
> --- a/src/pkg/compress/gzip/gunzip.go
> +++ b/src/pkg/compress/gzip/gunzip.go
> @@ -89,6 +89,17 @@
> return z, nil
> }
>
> +// Reset discards the Reader z's state and makes it equivalent to the
> +// result of its original state from NewReader, but reading from r
> instead.
> +// This permits reusing a Reader rather than allocating a new one.
> +func (z *Reader) Reset(r io.Reader) error {
> + z.r = makeReader(r)
> + z.digest.Reset()
> + z.size = 0
> + z.err = nil
> + return z.readHeader(true)
> +}
> +
> // GZIP (RFC 1952) is little-endian, unlike ZLIB (RFC 1950).
> func get4(p []byte) uint32 {
> return uint32(p[0]) | uint32(p[1])<<8 | uint32(p[2])<<16 |
> uint32(p[3])<<24
> Index: src/pkg/compress/gzip/gunzip_test.go
> ===================================================================
> --- a/src/pkg/compress/gzip/gunzip_test.go
> +++ b/src/pkg/compress/gzip/gunzip_test.go
> @@ -303,6 +303,26 @@
> if s != tt.raw {
> t.Errorf("%s: got %d-byte %q want %d-byte %q",
> tt.name, n, s, len(tt.raw), tt.raw)
> }
> +
> + // Test Reader Reset.
> + in = bytes.NewBuffer(tt.gzip)
> + err = gzip.Reset(in)
> + if err != nil {
> + t.Errorf("%s: Reset: %s", tt.name, err)
> + continue
> + }
> + if tt.name != gzip.Name {
> + t.Errorf("%s: got name %s", tt.name, gzip.Name)
> + }
> + b.Reset()
> + n, err = io.Copy(b, gzip)
> + if err != tt.err {
> + t.Errorf("%s: io.Copy: %v want %v", tt.name, err,
> tt.err)
> + }
> + s = b.String()
> + if s != tt.raw {
> + t.Errorf("%s: got %d-byte %q want %d-byte %q",
> tt.name, n, s, len(tt.raw), tt.raw)
> + }
> }
> }
>
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
Message from dan.kortschak@adelaide.edu.au
2013-12-10T20:16:27+00:00kortschakurn:md5:4ad439201361d06175a06e68130b1aed
On 2013/12/10 11:55:19, bradfitz wrote:
> Looks like this still allocates a flate.NewReader each time still? That's
> pretty big.
Yes. flate doesn't offer any API to do otherwise. If the notion of a Reset were propagate back to flate.decompressor then that could be used, but that can be done in another CL. Calling Reset on the flate.decompressor would require an assertion unless flat.NewReader's signature can be changed.
Message from dan.kortschak@adelaide.edu.au
2013-12-13T05:40:18+00:00kortschakurn:md5:0099a5e252a4641b9c8f8455bac4751a
On 2013/12/10 20:16:27, kortschak wrote:
> If the notion of a Reset were
> propagate back to flate.decompressor then that could be used, but that can be
> done in another CL. Calling Reset on the flate.decompressor would require an
> assertion unless flat.NewReader's signature can be changed.
Alternatively to adding Reset to flate.decompressor, your https://codereview.appspot.com/13416045 will reduce the load from flate.NewReader.
Message from bradfitz@golang.org
2013-12-16T20:29:48+00:00bradfitzurn:md5:fa03e5137746c4612961b163fd3ee2cd
Regarding https://codereview.appspot.com/13416045, I'd like to see
sync.Pool go in before we add more buffered-chan pools all over the place.
On Thu, Dec 12, 2013 at 9:40 PM, <dan.kortschak@adelaide.edu.au> wrote:
> On 2013/12/10 20:16:27, kortschak wrote:
>
>> If the notion of a Reset were
>> propagate back to flate.decompressor then that could be used, but that
>>
> can be
>
>> done in another CL. Calling Reset on the flate.decompressor would
>>
> require an
>
>> assertion unless flat.NewReader's signature can be changed.
>>
>
> Alternatively to adding Reset to flate.decompressor, your
> https://codereview.appspot.com/13416045 will reduce the load from
> flate.NewReader.
>
> https://codereview.appspot.com/13512052/
>
Message from rsc@golang.org
2013-12-18T02:42:40+00:00rscurn:md5:62d181da331c2078978a024719ff36e4
i commented on 13416045. reset is better. please do not start adding
recycling pools everywhere.
Message from bradfitz@golang.org
2013-12-18T02:49:24+00:00bradfitzurn:md5:23aca80097f35321af2d7f2de9192233
I think you're missing some context.
On Wed, Dec 18, 2013 at 6:42 AM, Russ Cox <rsc@golang.org> wrote:
> i commented on 13416045. reset is better. please do not start adding
> recycling pools everywhere.
>
>
Message from rsc@golang.org
2013-12-18T02:59:53+00:00rscurn:md5:3f43a70d926553c78ac68e74b0c03c17
On Tue, Dec 17, 2013 at 9:49 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:
> I think you're missing some context.
>
What context am I missing? You wrote "Regarding
https://codereview.appspot.com/13416045, I'd like to see sync.Pool go in
before we add more buffered-chan pools all over the place." My response is
that even if sync.Pool goes in, CL 13416045 is still not okay.
Russ
Message from dan.kortschak@adelaide.edu.au
2013-12-18T04:33:43+00:00kortschakurn:md5:d3c99f86124938932b7cb03b7780530e
On 2013/12/18 02:42:40, rsc wrote:
> i commented on 13416045. reset is better.
I agree it's simpler and easier to reason about. I wondering though how to add a Reset method to the flate.decompressor. Does gzip.Reader then assert .(Reseter) on its decompressor field?
Message from bradfitz@golang.org
2013-12-19T02:36:24+00:00bradfitzurn:md5:92e0cdc3c4702c20b6f0406b74821dd4
On Tue, Dec 17, 2013 at 8:33 PM, <dan.kortschak@adelaide.edu.au> wrote:
> On 2013/12/18 02:42:40, rsc wrote:
>
>> i commented on 13416045. reset is better.
>>
>
> I agree it's simpler and easier to reason about. I wondering though how
> to add a Reset method to the flate.decompressor. Does gzip.Reader then
> assert .(Reseter) on its decompressor field?
>
Yeah, either that, or document the concrete type of NewReader's
io.ReadCloser result, saying that it's guaranteed to be e.g.
*flate.Decompressor (we'd have to then export that. unfortunately the
better name, *Reader, is already taken by a useless interface...)
You want to take over that CL?
Message from gobot@golang.org
2013-12-20T16:26:03+00:00goboturn:md5:ab9ed0d44e9f343ff432e7d48fb7b93f
Replacing golang-dev with golang-codereviews.
Message from dan.kortschak@adelaide.edu.au
2013-12-20T20:56:18+00:00kortschakurn:md5:a28827cc7b4d6a286b53fb504af9a4c6
On 2013/12/19 02:36:24, bradfitz wrote:
> Yeah, either that, or document the concrete type of NewReader's
> io.ReadCloser result, saying that it's guaranteed to be e.g.
> *flate.Decompressor (we'd have to then export that. unfortunately the
> better name, *Reader, is already taken by a useless interface...)
Neither of these are particularly appealing.
> You want to take over that CL?
I can, but I'd like to get rsc's view on which approach to take.
Message from gobot@golang.org
2014-01-14T21:32:46+00:00goboturn:md5:cf14fd01b75abd435ca2256745b0d57c
R=rsc@golang.org (assigned by bradfitz@golang.org)
Message from dan.kortschak@adelaide.edu.au
2014-02-13T05:47:29+00:00kortschakurn:md5:d8561059bcfde0988aeeeb6adc536cd5
*ping* to get off waiting for author list.
Message from rsc@golang.org
2014-02-13T06:05:26+00:00rscurn:md5:c1f62f141d1cacf8c161701bca8390c3
it didn't work. i don't know why. the server is seeing your messages. i
know this CL is in my court.
Message from rsc@golang.org
2014-04-17T02:43:26+00:00rscurn:md5:abb02cb4f37706a474658bcea28a4dcc
LGTM
There's a typo in the test, which I will fix and submit.
Message from rsc@golang.org
2014-04-17T02:43:48+00:00rscurn:md5:475cec924688a98ffd7c02382e413a11
*** Submitted as https://code.google.com/p/go/source/detail?r=3f2fed7c0a19 ***
compress/gzip: add Reset method to Reader
Fixes issue 6364.
LGTM=rsc
R=golang-codereviews, bradfitz, rsc, gobot
CC=golang-codereviews
https://codereview.appspot.com/13512052
Committer: Russ Cox <rsc@golang.org>