code review 12953048: compress/flate: implement Reset method on Writer.

compress/flate: implement Reset method on Writer.

Fixes issue 6138.

[remyoudompheng, 2013-08-24 07:48:36]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[remyoudompheng, 2013-08-24 07:50:06]

TestWriterReset may be a bit slow because of the use of DeepEqual, but it can be
improved with CL 13000044.
On my old Pentium M and the DeepEqual patch, TestWriterReset takes 0.2s in short
mode.

[bradfitz, 2013-08-24 14:38:01]

https://codereview.appspot.com/12953048/diff/6001/src/pkg/compress/flate/defl...
File src/pkg/compress/flate/deflate.go (right):

https://codereview.appspot.com/12953048/diff/6001/src/pkg/compress/flate/defl...
src/pkg/compress/flate/deflate.go:554: func (writer *Writer) Reset(w io.Writer)
error {
receiver should be named "w" for consistency with other methods on *Writer.

Also, you're not preserving the dictionary from NewWriterDict.  I was thinking
that we'd store (a copy of?) NewWriterDict's dict []byte, and re-do the
dictWriter thing on a Reset of a *Writer which was originally created with
NewWriterDict.

[remyoudompheng, 2013-08-24 15:55:22]

https://codereview.appspot.com/12953048/diff/6001/src/pkg/compress/flate/defl...
File src/pkg/compress/flate/deflate.go (right):

https://codereview.appspot.com/12953048/diff/6001/src/pkg/compress/flate/defl...
src/pkg/compress/flate/deflate.go:554: func (writer *Writer) Reset(w io.Writer)
error {
On 2013/08/24 14:38:02, bradfitz wrote:
> receiver should be named "w" for consistency with other methods on *Writer.

How would you call the io.Writer argument?

> Also, you're not preserving the dictionary from NewWriterDict.  I was thinking
> that we'd store (a copy of?) NewWriterDict's dict []byte, and re-do the
> dictWriter thing on a Reset of a *Writer which was originally created with
> NewWriterDict.

Or maybe the dictionary should be an argument of the Reset function?
What seems more natural to you ?

[bradfitz, 2013-08-24 15:58:51]

On Sat, Aug 24, 2013 at 10:55 AM, <remyoudompheng@gmail.com> wrote:

>
> https://codereview.appspot.**com/12953048/diff/6001/src/**
>
pkg/compress/flate/deflate.go<https://codereview.appspot.com/12953048/diff/6001/src/pkg/compress/flate/deflate.go>
> File src/pkg/compress/flate/**deflate.go (right):
>
> https://codereview.appspot.**com/12953048/diff/6001/src/**
>
pkg/compress/flate/deflate.go#**newcode554<https://codereview.appspot.com/12953048/diff/6001/src/pkg/compress/flate/deflate.go#newcode554>
> src/pkg/compress/flate/**deflate.go:554: func (writer *Writer) Reset(w
> io.Writer) error {
> On 2013/08/24 14:38:02, bradfitz wrote:
>
>> receiver should be named "w" for consistency with other methods on
>>
> *Writer.
>
> How would you call the io.Writer argument?


dst?


>  Also, you're not preserving the dictionary from NewWriterDict.  I was
>>
> thinking
>
>> that we'd store (a copy of?) NewWriterDict's dict []byte, and re-do
>>
> the
>
>> dictWriter thing on a Reset of a *Writer which was originally created
>>
> with
>
>> NewWriterDict.
>>
>
> Or maybe the dictionary should be an argument of the Reset function?
> What seems more natural to you ?
>

If we make it Reset(dict []byte) where dict is nil-able for "none", it
might make sense to put the compression level in Reset too.  I'd be fine
with that at least.  It's a bigger API, though.  The dict should be small,
though, so making a copy shouldn't be too bad. (much smaller than 512 KB).

[remyoudompheng, 2013-08-24 16:03:47]

On 2013/08/24 15:58:51, bradfitz wrote:
> If we make it Reset(dict []byte) where dict is nil-able for "none", it
> might make sense to put the compression level in Reset too.  I'd be fine
> with that at least.  It's a bigger API, though.  The dict should be small,
> though, so making a copy shouldn't be too bad. (much smaller than 512 KB).

I expect the compression level to influence the detail of allocated data
structures (maybe buffers are larger, for example level=NoCompression is
special), and I expect Reset to really reuse a Writer, not reshape it, so it's
OK to me to not have a way to modify the compression level.

Did anybody else think about this? We are going to be stuck with this API for a
while.

[bradfitz, 2013-08-24 16:07:55]

Let's just go with as simple and least surprising of an API for now.  We
could add ResetLevelDict later if we really had to.
 On Aug 24, 2013 11:03 AM, <remyoudompheng@gmail.com> wrote:

> On 2013/08/24 15:58:51, bradfitz wrote:
>
>> If we make it Reset(dict []byte) where dict is nil-able for "none", it
>> might make sense to put the compression level in Reset too.  I'd be
>>
> fine
>
>> with that at least.  It's a bigger API, though.  The dict should be
>>
> small,
>
>> though, so making a copy shouldn't be too bad. (much smaller than 512
>>
> KB).
>
> I expect the compression level to influence the detail of allocated data
> structures (maybe buffers are larger, for example level=NoCompression is
> special), and I expect Reset to really reuse a Writer, not reshape it,
> so it's OK to me to not have a way to modify the compression level.
>
> Did anybody else think about this? We are going to be stuck with this
> API for a while.
>
>
https://codereview.appspot.**com/12953048/<https://codereview.appspot.com/129...
>

[remyoudompheng, 2013-08-29 10:10:23]

https://codereview.appspot.com/12953048/diff/6001/src/pkg/compress/flate/defl...
File src/pkg/compress/flate/deflate.go (right):

https://codereview.appspot.com/12953048/diff/6001/src/pkg/compress/flate/defl...
src/pkg/compress/flate/deflate.go:554: func (writer *Writer) Reset(w io.Writer)
error {
On 2013/08/24 14:38:02, bradfitz wrote:
> receiver should be named "w" for consistency with other methods on *Writer.
> 
> Also, you're not preserving the dictionary from NewWriterDict.  I was thinking
> that we'd store (a copy of?) NewWriterDict's dict []byte, and re-do the
> dictWriter thing on a Reset of a *Writer which was originally created with
> NewWriterDict.

Done.

[remyoudompheng, 2013-08-29 10:10:41]

Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[bradfitz, 2013-08-29 18:45:30]

LGTM

https://codereview.appspot.com/12953048/diff/16001/src/pkg/compress/flate/def...
File src/pkg/compress/flate/deflate.go (right):

https://codereview.appspot.com/12953048/diff/16001/src/pkg/compress/flate/def...
src/pkg/compress/flate/deflate.go:435: for s := d.hashHead; len(s) > 0; s =
s[32:] {
prefer to see this 32 be a len(zeros) or a constant, instead of manually kept in
sync with above

https://codereview.appspot.com/12953048/diff/16001/src/pkg/compress/flate/def...
src/pkg/compress/flate/deflate.go:444: for s := d.window; len(s) > 0; s =
s[256:] {
likewise

https://codereview.appspot.com/12953048/diff/16001/src/pkg/compress/flate/def...
src/pkg/compress/flate/deflate.go:556: func (w *Writer) Reset(dst io.Writer)
error {
this doesn't need to return an error.  The only error that the NewWriter{,Dict}
returns is if the level was bad. But if we already have a *Writer and we're
preserving the same level, this must always succeed.

I understand the desire to give ourselves a future chance to return an error,
but I can't imagine why we'd ever want to.  Advertising that this can return an
error will cause people to needlessly check for it.

[remyoudompheng, 2013-08-29 19:01:14]

https://codereview.appspot.com/12953048/diff/16001/src/pkg/compress/flate/def...
File src/pkg/compress/flate/deflate.go (right):

https://codereview.appspot.com/12953048/diff/16001/src/pkg/compress/flate/def...
src/pkg/compress/flate/deflate.go:435: for s := d.hashHead; len(s) > 0; s =
s[32:] {
On 2013/08/29 18:45:30, bradfitz wrote:
> prefer to see this 32 be a len(zeros) or a constant, instead of manually kept
in
> sync with above

Done.

Written differently, I'm not even sure the code was correct.

https://codereview.appspot.com/12953048/diff/16001/src/pkg/compress/flate/def...
src/pkg/compress/flate/deflate.go:444: for s := d.window; len(s) > 0; s =
s[256:] {
On 2013/08/29 18:45:30, bradfitz wrote:
> likewise

Done.

https://codereview.appspot.com/12953048/diff/16001/src/pkg/compress/flate/def...
src/pkg/compress/flate/deflate.go:556: func (w *Writer) Reset(dst io.Writer)
error {
On 2013/08/29 18:45:30, bradfitz wrote:
> this doesn't need to return an error.  The only error that the
NewWriter{,Dict}
> returns is if the level was bad. But if we already have a *Writer and we're
> preserving the same level, this must always succeed.
> 
> I understand the desire to give ourselves a future chance to return an error,
> but I can't imagine why we'd ever want to.  Advertising that this can return
an
> error will cause people to needlessly check for it.

Done.

Sorry it was left from a state where the signature was different.

[bradfitz, 2013-08-29 19:03:48]

LGTM

[remyoudompheng, 2013-08-29 19:09:35]

*** Submitted as https://code.google.com/p/go/source/detail?r=fe4bfff38132 ***

compress/flate: implement Reset method on Writer.

Fixes issue 6138.

R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/12953048