code review 54300053: archive/zip: re-use flate.Writers when writing compress...

archive/zip: re-use flate.Writers when writing compressed files

Prevents a ton of garbage. (Noticed this when writing large
Camlistore zip archives to Amazon Glacier)

Note that the Closer part of the io.WriteCloser is never given
to users. It's an internal detail of the package.

benchmark                         old ns/op     new ns/op     delta
BenchmarkCompressedZipGarbage     42884123      40732373      -5.02%

benchmark                         old allocs     new allocs     delta
BenchmarkCompressedZipGarbage     204            149            -26.96%

benchmark                         old bytes     new bytes     delta
BenchmarkCompressedZipGarbage     4397576       66744         -98.48%

[bradfitz, 2014-02-08 01:07:08]

Hello adg@golang.org (cc: golang-codereviews@googlegroups.com),

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

[adg, 2014-02-09 21:58:38]

https://codereview.appspot.com/54300053/diff/40001/src/pkg/archive/zip/regist...
File src/pkg/archive/zip/register.go (right):

https://codereview.appspot.com/54300053/diff/40001/src/pkg/archive/zip/regist...
src/pkg/archive/zip/register.go:28: fw, ok :=
flateWriterPool.Get().(*flate.Writer)
I didn't realize how nicely comma-ok could be used with Pool's Get method w/ a
type assertion. Very cool.

https://codereview.appspot.com/54300053/diff/40001/src/pkg/archive/zip/regist...
src/pkg/archive/zip/register.go:37: type pooledFlateWriter struct {
this isn't a pooledFlateWriter, it's a syncFlateWriter
but why? the original code doesn't have any synchronization

[bradfitz, 2014-02-09 22:09:31]

https://codereview.appspot.com/54300053/diff/40001/src/pkg/archive/zip/regist...
File src/pkg/archive/zip/register.go (right):

https://codereview.appspot.com/54300053/diff/40001/src/pkg/archive/zip/regist...
src/pkg/archive/zip/register.go:37: type pooledFlateWriter struct {
On 2014/02/09 21:58:39, adg wrote:
> this isn't a pooledFlateWriter, it's a syncFlateWriter
> but why? the original code doesn't have any synchronization

Paranoid about buggy code writing to zip files concurrently with starting
multiple new parts, which would cause us to race on Close and double-return the
writer to the pool.

I guess it doesn't matter, since the user is already disobeying the documented
rules, but I don't think we should corrupt shared state between two callers. 
Only the broken caller should see errors, not a correct caller.

[adg, 2014-02-09 22:11:51]

LGTM

On 2014/02/09 22:09:31, bradfitz wrote:
>
https://codereview.appspot.com/54300053/diff/40001/src/pkg/archive/zip/regist...
> File src/pkg/archive/zip/register.go (right):
> 
>
https://codereview.appspot.com/54300053/diff/40001/src/pkg/archive/zip/regist...
> src/pkg/archive/zip/register.go:37: type pooledFlateWriter struct {
> On 2014/02/09 21:58:39, adg wrote:
> > this isn't a pooledFlateWriter, it's a syncFlateWriter
> > but why? the original code doesn't have any synchronization
> 
> Paranoid about buggy code writing to zip files concurrently with starting
> multiple new parts, which would cause us to race on Close and double-return
the
> writer to the pool.
> 
> I guess it doesn't matter, since the user is already disobeying the documented
> rules, but I don't think we should corrupt shared state between two callers. 
> Only the broken caller should see errors, not a correct caller.

Sounds reasonable.

[rsc, 2014-02-11 19:34:25]

LGTM

I wish we could do better but the flate API + Go 1 compatibility doesn't. The
extra check for reuse is nice. Thanks.

[bradfitz, 2014-02-11 19:41:30]

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

archive/zip: re-use flate.Writers when writing compressed files

Prevents a ton of garbage. (Noticed this when writing large
Camlistore zip archives to Amazon Glacier)

Note that the Closer part of the io.WriteCloser is never given
to users. It's an internal detail of the package.

benchmark                         old ns/op     new ns/op     delta
BenchmarkCompressedZipGarbage     42884123      40732373      -5.02%

benchmark                         old allocs     new allocs     delta
BenchmarkCompressedZipGarbage     204            149            -26.96%

benchmark                         old bytes     new bytes     delta
BenchmarkCompressedZipGarbage     4397576       66744         -98.48%

LGTM=adg, rsc
R=adg, rsc
CC=golang-codereviews
https://codereview.appspot.com/54300053