code review 108240044: archive/tar: reuse temporary buffer in readHeader

archive/tar: reuse temporary buffer in readHeader

A temporary 512 bytes buffer is allocated for every call to
readHeader. This buffer isn't returned to the caller and it could
be reused to lower the number of memory allocations.

This CL improves it by using a pool and zeroing out the buffer before
putting it back into the pool.

benchmark                  old ns/op     new ns/op     delta
BenchmarkListFiles100k     545249903     538832687     -1.18%

benchmark                  old allocs    new allocs    delta
BenchmarkListFiles100k     2105167       2005692       -4.73%

benchmark                  old bytes     new bytes     delta
BenchmarkListFiles100k     105903472     54831527      -48.22%

This improvement is very important if your code has to deal with a lot
of tarballs which contain a lot of files.

[unclejack, 2014-06-27 20:24:14]

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

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

[bradfitz, 2014-06-27 20:43:34]

This is a bad use of sync.Pool.  Just put a []byte that you re-use off the
*Reader struct instead.



On Fri, Jun 27, 2014 at 1:24 PM, <unclejacksons@gmail.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com (cc: bradfitz@golang.org,
> golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> archive/tar: reuse temporary buffer in readHeader
>
> A temporary 512 bytes buffer is allocated for every call to
> readHeader. This buffer isn't returned to the caller and it could
> be reused to lower the number of memory allocations.
>
> This CL improves it by using a pool and zeroing out the buffer before
> putting it back into the pool.
>
> benchmark                  old ns/op     new ns/op     delta
> BenchmarkListFiles100k     545249903     538832687     -1.18%
>
> benchmark                  old allocs    new allocs    delta
> BenchmarkListFiles100k     2105167       2005692       -4.73%
>
> benchmark                  old bytes     new bytes     delta
> BenchmarkListFiles100k     105903472     54831527      -48.22%
>
> This improvement is very important if your code has to deal with a lot
> of tarballs which contain a lot of files.
>
> Please review this at https://codereview.appspot.com/108240044/
>
> Affected files (+15, -1 lines):
>   M src/pkg/archive/tar/reader.go
>
>
> Index: src/pkg/archive/tar/reader.go
> ===================================================================
> --- a/src/pkg/archive/tar/reader.go
> +++ b/src/pkg/archive/tar/reader.go
> @@ -15,6 +15,7 @@
>         "os"
>         "strconv"
>         "strings"
> +       "sync"
>         "time"
>  )
>
> @@ -425,8 +426,21 @@
>         return given == unsigned || given == signed
>  }
>
> +var headerPool = sync.Pool{
> +       New: func() interface{} {
> +               b := make([]byte, blockSize)
> +               return &b
> +       },
> +}
> +
>  func (tr *Reader) readHeader() *Header {
> -       header := make([]byte, blockSize)
> +       v := headerPool.Get().(*[]byte)
> +       header := *v
> +       defer func() {
> +               copy(header[0:blockSize], zeroBlock[0:blockSize])
> +               headerPool.Put(v)
> +       }()
> +
>         if _, tr.err = io.ReadFull(tr.r, header); tr.err != nil {
>                 return nil
>         }
>
>
>

[unclejack, 2014-06-27 21:43:38]

Hello,

I've updated it based on your feedback.

PTAL


benchmark                  old ns/op     new ns/op     delta       
BenchmarkListFiles100k     545249903     486434630     -10.79%     

benchmark                  old allocs     new allocs     delta      
BenchmarkListFiles100k     2105167        2005693        -4.73%     

benchmark                  old bytes     new bytes     delta       
BenchmarkListFiles100k     105903472     54832020      -48.22%


On 2014/06/27 20:43:34, bradfitz wrote:
> This is a bad use of sync.Pool.  Just put a []byte that you re-use off the
> *Reader struct instead.
> 
> 
> 
> On Fri, Jun 27, 2014 at 1:24 PM, <mailto:unclejacksons@gmail.com> wrote:
> 
> > Reviewers: golang-codereviews,
> >
> > Message:
> > Hello mailto:golang-codereviews@googlegroups.com (cc:
mailto:bradfitz@golang.org,
> > mailto:golang-codereviews@googlegroups.com),
> >
> > I'd like you to review this change to
> > https://code.google.com/p/go/
> >
> >
> > Description:
> > archive/tar: reuse temporary buffer in readHeader
> >
> > A temporary 512 bytes buffer is allocated for every call to
> > readHeader. This buffer isn't returned to the caller and it could
> > be reused to lower the number of memory allocations.
> >
> > This CL improves it by using a pool and zeroing out the buffer before
> > putting it back into the pool.
> >
> > benchmark                  old ns/op     new ns/op     delta
> > BenchmarkListFiles100k     545249903     538832687     -1.18%
> >
> > benchmark                  old allocs    new allocs    delta
> > BenchmarkListFiles100k     2105167       2005692       -4.73%
> >
> > benchmark                  old bytes     new bytes     delta
> > BenchmarkListFiles100k     105903472     54831527      -48.22%
> >
> > This improvement is very important if your code has to deal with a lot
> > of tarballs which contain a lot of files.
> >
> > Please review this at https://codereview.appspot.com/108240044/
> >
> > Affected files (+15, -1 lines):
> >   M src/pkg/archive/tar/reader.go
> >
> >
> > Index: src/pkg/archive/tar/reader.go
> > ===================================================================
> > --- a/src/pkg/archive/tar/reader.go
> > +++ b/src/pkg/archive/tar/reader.go
> > @@ -15,6 +15,7 @@
> >         "os"
> >         "strconv"
> >         "strings"
> > +       "sync"
> >         "time"
> >  )
> >
> > @@ -425,8 +426,21 @@
> >         return given == unsigned || given == signed
> >  }
> >
> > +var headerPool = sync.Pool{
> > +       New: func() interface{} {
> > +               b := make([]byte, blockSize)
> > +               return &b
> > +       },
> > +}
> > +
> >  func (tr *Reader) readHeader() *Header {
> > -       header := make([]byte, blockSize)
> > +       v := headerPool.Get().(*[]byte)
> > +       header := *v
> > +       defer func() {
> > +               copy(header[0:blockSize], zeroBlock[0:blockSize])
> > +               headerPool.Put(v)
> > +       }()
> > +
> >         if _, tr.err = io.ReadFull(tr.r, header); tr.err != nil {
> >                 return nil
> >         }
> >
> >
> >

[dave_cheney.net, 2014-06-29 00:04:07]

You may be able to improve this further using the 'slice of byte array' trick.

https://codereview.appspot.com/108240044/diff/40001/src/pkg/archive/tar/reade...
File src/pkg/archive/tar/reader.go (right):

https://codereview.appspot.com/108240044/diff/40001/src/pkg/archive/tar/reade...
src/pkg/archive/tar/reader.go:36: hdrBuff []byte         // buffer to use in
readHeader
hdrBuff[blockSize]byte

https://codereview.appspot.com/108240044/diff/40001/src/pkg/archive/tar/reade...
src/pkg/archive/tar/reader.go:438: 
header := tr.hdrBuff[:]
copy(header[0:blockSize], zeroBlock[0:blockSize])

[unclejack, 2014-06-29 15:56:17]

I've updated the patch.


On 2014/06/29 00:04:07, dfc wrote:
> You may be able to improve this further using the 'slice of byte array' trick.
> 
>
https://codereview.appspot.com/108240044/diff/40001/src/pkg/archive/tar/reade...
> File src/pkg/archive/tar/reader.go (right):
> 
>
https://codereview.appspot.com/108240044/diff/40001/src/pkg/archive/tar/reade...
> src/pkg/archive/tar/reader.go:36: hdrBuff []byte         // buffer to use in
> readHeader
> hdrBuff[blockSize]byte
> 
>
https://codereview.appspot.com/108240044/diff/40001/src/pkg/archive/tar/reade...
> src/pkg/archive/tar/reader.go:438: 
> header := tr.hdrBuff[:]
> copy(header[0:blockSize], zeroBlock[0:blockSize])

[bradfitz, 2014-07-01 17:54:47]

R=dsymonds

[dsymonds, 2014-07-02 00:44:28]

LGTM

one little change please

https://codereview.appspot.com/108240044/diff/60001/src/pkg/archive/tar/reade...
File src/pkg/archive/tar/reader.go (right):

https://codereview.appspot.com/108240044/diff/60001/src/pkg/archive/tar/reade...
src/pkg/archive/tar/reader.go:431: copy(tr.hdrBuff[0:blockSize],
zeroBlock[0:blockSize])
copy(tr.hdrBuff[:], zeroBlock)

[dave_cheney.net, 2014-07-02 00:49:51]

https://codereview.appspot.com/108240044/diff/60001/src/pkg/archive/tar/reade...
File src/pkg/archive/tar/reader.go (right):

https://codereview.appspot.com/108240044/diff/60001/src/pkg/archive/tar/reade...
src/pkg/archive/tar/reader.go:431: copy(tr.hdrBuff[0:blockSize],
zeroBlock[0:blockSize])
On 2014/07/02 00:44:28, dsymonds wrote:
> copy(tr.hdrBuff[:], zeroBlock)

why not 

copy(header, zeroBlock) ?

[dsymonds, 2014-07-02 00:50:19]

yep, even better.

[unclejack, 2014-07-02 07:48:25]

I've updated the patch.

dsymonds, PTAL

On 2014/07/02 00:50:19, dsymonds wrote:
> yep, even better.

[unclejack, 2014-07-02 08:53:26]

https://codereview.appspot.com/108240044/diff/40001/src/pkg/archive/tar/reade...
File src/pkg/archive/tar/reader.go (right):

https://codereview.appspot.com/108240044/diff/40001/src/pkg/archive/tar/reade...
src/pkg/archive/tar/reader.go:36: hdrBuff []byte         // buffer to use in
readHeader
On 2014/06/29 00:04:07, dfc wrote:
> hdrBuff[blockSize]byte 

Done.

https://codereview.appspot.com/108240044/diff/40001/src/pkg/archive/tar/reade...
src/pkg/archive/tar/reader.go:438: 
On 2014/06/29 00:04:07, dfc wrote:
> header := tr.hdrBuff[:]
> copy(header[0:blockSize], zeroBlock[0:blockSize])

Done.

https://codereview.appspot.com/108240044/diff/60001/src/pkg/archive/tar/reade...
File src/pkg/archive/tar/reader.go (right):

https://codereview.appspot.com/108240044/diff/60001/src/pkg/archive/tar/reade...
src/pkg/archive/tar/reader.go:431: copy(tr.hdrBuff[0:blockSize],
zeroBlock[0:blockSize])
On 2014/07/02 00:44:28, dsymonds wrote:
> copy(tr.hdrBuff[:], zeroBlock)

Done.

https://codereview.appspot.com/108240044/diff/60001/src/pkg/archive/tar/reade...
src/pkg/archive/tar/reader.go:431: copy(tr.hdrBuff[0:blockSize],
zeroBlock[0:blockSize])
On 2014/07/02 00:49:51, dfc wrote:
> On 2014/07/02 00:44:28, dsymonds wrote:
> > copy(tr.hdrBuff[:], zeroBlock)
> 
> why not 
> 
> copy(header, zeroBlock) ?
> 

This was breaking all.bash:
archive/tar
# archive/tar
pkg/archive/tar/reader.go:431: first argument to copy should be slice; have
[512]byte

[dave_cheney.net, 2014-07-02 10:35:39]

Unclejacksons, i'm happy to commit this change now.

Before that happens I need you to complete the CLA document described
in http://golang.org/doc/contribute.html. If you've already done this
then a googler will review it and update the AUTHORS and CONTRIBUTORS
files and we can submit this change.

Cheers

Dave

On Wed, Jul 2, 2014 at 5:48 PM,  <unclejacksons@gmail.com> wrote:
> I've updated the patch.
>
> dsymonds, PTAL
>
> On 2014/07/02 00:50:19, dsymonds wrote:
>>
>> yep, even better.
>
>
>
>
> https://codereview.appspot.com/108240044/

[bradfitz, 2014-07-02 13:49:11]

CLA is in.



On Wed, Jul 2, 2014 at 3:35 AM, Dave Cheney <dave@cheney.net> wrote:

> Unclejacksons, i'm happy to commit this change now.
>
> Before that happens I need you to complete the CLA document described
> in http://golang.org/doc/contribute.html. If you've already done this
> then a googler will review it and update the AUTHORS and CONTRIBUTORS
> files and we can submit this change.
>
> Cheers
>
> Dave
>
> On Wed, Jul 2, 2014 at 5:48 PM,  <unclejacksons@gmail.com> wrote:
> > I've updated the patch.
> >
> > dsymonds, PTAL
> >
> > On 2014/07/02 00:50:19, dsymonds wrote:
> >>
> >> yep, even better.
> >
> >
> >
> >
> > https://codereview.appspot.com/108240044/
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[dsymonds, 2014-07-02 23:38:20]

LGTM

[dsymonds, 2014-07-02 23:41:32]

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

archive/tar: reuse temporary buffer in readHeader

A temporary 512 bytes buffer is allocated for every call to
readHeader. This buffer isn't returned to the caller and it could
be reused to lower the number of memory allocations.

This CL improves it by using a pool and zeroing out the buffer before
putting it back into the pool.

benchmark                  old ns/op     new ns/op     delta
BenchmarkListFiles100k     545249903     538832687     -1.18%

benchmark                  old allocs    new allocs    delta
BenchmarkListFiles100k     2105167       2005692       -4.73%

benchmark                  old bytes     new bytes     delta
BenchmarkListFiles100k     105903472     54831527      -48.22%

This improvement is very important if your code has to deal with a lot
of tarballs which contain a lot of files.

LGTM=dsymonds
R=golang-codereviews, dave, dsymonds, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/108240044

Committer: David Symonds <dsymonds@golang.org>