code review 13512052: compress/gzip: add Reset method to Reader

compress/gzip: add Reset method to Reader

Fixes issue 6364.

[kortschak, 2013-12-09 21:59:55]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[bradfitz, 2013-12-10 11:55:19]

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.
>

[kortschak, 2013-12-10 20:16:27]

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.

[kortschak, 2013-12-13 05:40:18]

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.

[bradfitz, 2013-12-16 20:29:48]

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/
>

[rsc, 2013-12-18 02:42:40]

i commented on 13416045. reset is better. please do not start adding
recycling pools everywhere.

[bradfitz, 2013-12-18 02:49:24]

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.
>
>

[rsc, 2013-12-18 02:59:53]

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

[kortschak, 2013-12-18 04:33:43]

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?

[bradfitz, 2013-12-19 02:36:24]

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?

[gobot, 2013-12-20 16:26:03]

Replacing golang-dev with golang-codereviews.

[kortschak, 2013-12-20 20:56:18]

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.

[gobot, 2014-01-14 21:32:46]

R=rsc@golang.org (assigned by bradfitz@golang.org)

[kortschak, 2014-02-13 05:47:29]

*ping* to get off waiting for author list.

[rsc, 2014-02-13 06:05:26]

it didn't work. i don't know why. the server is seeing your messages. i
know this CL is in my court.

[rsc, 2014-04-17 02:43:26]

LGTM

There's a typo in the test, which I will fix and submit.

[rsc, 2014-04-17 02:43:48]

*** 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>