Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(742)

Issue 12894043: code review 12894043: archive/zip: remove an allocation, speed up a test (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 3 months ago by bradfitz
Modified:
4 years, 3 months ago
Reviewers:
adg1, rsc
CC:
golang-dev, rsc
Visibility:
Public.

Description

archive/zip: remove an allocation, speed up a test Update Issue 6138 TestOver65kFiles spends all its time garbage collecting. Removing the 1.4 MB of allocations per each of the 65k files brings this from 34 seconds to 0.23 seconds.

Patch Set 1 #

Patch Set 2 : diff -r a128bb95c3bd https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r a128bb95c3bd https://go.googlecode.com/hg/ #

Patch Set 4 : diff -r 2dd7d9e8f560 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+5 lines, -6 lines) Patch
M src/pkg/archive/zip/reader.go View 1 1 chunk +1 line, -2 lines 0 comments Download
M src/pkg/archive/zip/zip_test.go View 1 1 chunk +4 lines, -4 lines 0 comments Download

Messages

Total messages: 4
bradfitz
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
4 years, 3 months ago (2013-08-13 21:29:45 UTC) #1
rsc
LGTM
4 years, 3 months ago (2013-08-13 21:47:11 UTC) #2
bradfitz
*** Submitted as https://code.google.com/p/go/source/detail?r=2bc5fc177d58 *** archive/zip: remove an allocation, speed up a test Update Issue ...
4 years, 3 months ago (2013-08-13 21:48:11 UTC) #3
adg1
4 years, 3 months ago (2013-08-14 02:32:34 UTC) #4
LGTM

I knew this was inefficient but I didn't realise how bad. Thanks.
On 14 Aug 2013 07:29, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> archive/zip: remove an allocation, speed up a test
>
> Update Issue 6138
>
> TestOver65kFiles spends all its time garbage collecting.
> Removing the 1.4 MB of allocations per each of the 65k
> files brings this from 34 seconds to 0.23 seconds.
>
> Please review this at
https://codereview.appspot.**com/12894043/<https://codereview.appspot.com/128...
>
> Affected files:
>   M src/pkg/archive/zip/reader.go
>   M src/pkg/archive/zip/zip_test.**go
>
>
> Index: src/pkg/archive/zip/reader.go
> ==============================**==============================**=======
> --- a/src/pkg/archive/zip/reader.**go
> +++ b/src/pkg/archive/zip/reader.**go
> @@ -179,9 +179,8 @@
>  // findBodyOffset does the minimum work to verify the file has a header
>  // and returns the file body offset.
>  func (f *File) findBodyOffset() (int64, error) {
> -       r := io.NewSectionReader(f.zipr, f.headerOffset,
> f.zipsize-f.headerOffset)
>         var buf [fileHeaderLen]byte
> -       if _, err := io.ReadFull(r, buf[:]); err != nil {
> +       if _, err := f.zipr.ReadAt(buf[:], f.headerOffset); err != nil {
>                 return 0, err
>         }
>         b := readBuf(buf[:])
> Index: src/pkg/archive/zip/zip_test.**go
> ==============================**==============================**=======
> --- a/src/pkg/archive/zip/zip_**test.go
> +++ b/src/pkg/archive/zip/zip_**test.go
> @@ -17,14 +17,14 @@
>  )
>
>  func TestOver65kFiles(t *testing.T) {
> -       if testing.Short() {
> -               t.Skip("slow test; skipping")
> -       }
>         buf := new(bytes.Buffer)
>         w := NewWriter(buf)
>         const nFiles = (1 << 16) + 42
>         for i := 0; i < nFiles; i++ {
> -               _, err := w.Create(fmt.Sprintf("%d.dat", i))
> +               _, err := w.CreateHeader(&FileHeader{
> +                       Name:   fmt.Sprintf("%d.dat", i),
> +                       Method: Store, // avoid Issue 6136 and Issue 6138
> +               })
>                 if err != nil {
>                         t.Fatalf("creating file %d: %v", i, err)
>                 }
>
>
> --
>
> ---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<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld c59f3c4