code review 4441078: go/printer: added simple performance benchmark

go/printer: added simple performance benchmark

[gri, 2011-04-28 22:52:15]

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

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

[dave_cheney.net, 2011-04-28 23:01:21]

Hi Robert,

Could you use the newly created ioutil.Discard Writer in place of the
bytes.Buffer?

Cheers

Dave

[bradfitzgoog, 2011-04-28 23:01:40]

The nodes.go file should probably be copied into a "testdata" directory so
numbers are comparable over time.

On Thu, Apr 28, 2011 at 3:52 PM, <gri@golang.org> wrote:

> Reviewers: r,
>
> Message:
> Hello r (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> go/printer: added simple performance benchmark
>
> Please review this at http://codereview.appspot.com/4441078/
>
> Affected files:
>  A src/pkg/go/printer/performance_test.go
>
>
> Index: src/pkg/go/printer/performance_test.go
> ===================================================================
> new file mode 100644
> --- /dev/null
> +++ b/src/pkg/go/printer/performance_test.go
> @@ -0,0 +1,51 @@
> +// Copyright 2009 The Go Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style
> +// license that can be found in the LICENSE file.
> +
> +// This file implements a simple printer performance benchmark:
> +// gotest -bench=BenchmarkPrint
> +
> +package printer
> +
> +import (
> +       "bytes"
> +       "go/ast"
> +       "go/parser"
> +       "go/token"
> +       "log"
> +       "os"
> +       "testing"
> +)
> +
> +
> +var (
> +       testfset = token.NewFileSet()
> +       testfile *ast.File
> +       testbuf  bytes.Buffer
> +)
> +
> +
> +func init() {
> +       const filename = "nodes.go"
> +       var err os.Error
> +       testfile, err = parser.ParseFile(testfset, filename, nil, 0)
> +       if err != nil {
> +               log.Fatalf("could not parse %s without errors: %s",
> filename, err)
> +       }
> +}
> +
> +
> +func benchmarkPrint() {
> +       testbuf.Reset()
> +       err := Fprint(&testbuf, fset, testfile)
> +       if err != nil {
> +               log.Fatalf("print error: %s", err)
> +       }
> +}
> +
> +
> +func BenchmarkPrint(b *testing.B) {
> +       for i := 0; i < b.N; i++ {
> +               benchmarkPrint()
> +       }
> +}
>
>
>

[gri, 2011-04-28 23:16:01]

done and done; also using a larger file now.
PTAL.
- gri



On Thu, Apr 28, 2011 at 4:01 PM, Brad Fitzpatrick <bradfitz@google.com> wrote:
> The nodes.go file should probably be copied into a "testdata" directory so
> numbers are comparable over time.
>
> On Thu, Apr 28, 2011 at 3:52 PM, <gri@golang.org> wrote:
>>
>> Reviewers: r,
>>
>> Message:
>> Hello r (cc: golang-dev@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>>
>>
>> Description:
>> go/printer: added simple performance benchmark
>>
>> Please review this at http://codereview.appspot.com/4441078/
>>
>> Affected files:
>>  A src/pkg/go/printer/performance_test.go
>>
>>
>> Index: src/pkg/go/printer/performance_test.go
>> ===================================================================
>> new file mode 100644
>> --- /dev/null
>> +++ b/src/pkg/go/printer/performance_test.go
>> @@ -0,0 +1,51 @@
>> +// Copyright 2009 The Go Authors. All rights reserved.
>> +// Use of this source code is governed by a BSD-style
>> +// license that can be found in the LICENSE file.
>> +
>> +// This file implements a simple printer performance benchmark:
>> +// gotest -bench=BenchmarkPrint
>> +
>> +package printer
>> +
>> +import (
>> +       "bytes"
>> +       "go/ast"
>> +       "go/parser"
>> +       "go/token"
>> +       "log"
>> +       "os"
>> +       "testing"
>> +)
>> +
>> +
>> +var (
>> +       testfset = token.NewFileSet()
>> +       testfile *ast.File
>> +       testbuf  bytes.Buffer
>> +)
>> +
>> +
>> +func init() {
>> +       const filename = "nodes.go"
>> +       var err os.Error
>> +       testfile, err = parser.ParseFile(testfset, filename, nil, 0)
>> +       if err != nil {
>> +               log.Fatalf("could not parse %s without errors: %s",
>> filename, err)
>> +       }
>> +}
>> +
>> +
>> +func benchmarkPrint() {
>> +       testbuf.Reset()
>> +       err := Fprint(&testbuf, fset, testfile)
>> +       if err != nil {
>> +               log.Fatalf("print error: %s", err)
>> +       }
>> +}
>> +
>> +
>> +func BenchmarkPrint(b *testing.B) {
>> +       for i := 0; i < b.N; i++ {
>> +               benchmarkPrint()
>> +       }
>> +}
>>
>>
>
>

[bradfitz, 2011-04-28 23:27:03]

LGTM

[r, 2011-04-28 23:40:52]

LGTM but can drop error check

http://codereview.appspot.com/4441078/diff/10001/src/pkg/go/printer/performan...
File src/pkg/go/printer/performance_test.go (right):

http://codereview.appspot.com/4441078/diff/10001/src/pkg/go/printer/performan...
src/pkg/go/printer/performance_test.go:33: if err := Fprint(ioutil.Discard,
fset, testfile); err != nil {
this cannot possibly return an error, so it's not worth checking.
Fprint only errors on write, and Discard never errors.

[gri, 2011-04-29 00:02:56]

Apologies for the spam, but PTAL.

Several corrections:
- parse comments
- print with same configuration as gofmt
- make sure file is idempotent (and so print actually works) before testing

- gri

On Thu, Apr 28, 2011 at 4:40 PM,  <r@golang.org> wrote:
> LGTM but can drop error check
>
>
>
>
http://codereview.appspot.com/4441078/diff/10001/src/pkg/go/printer/performan...
> File src/pkg/go/printer/performance_test.go (right):
>
>
http://codereview.appspot.com/4441078/diff/10001/src/pkg/go/printer/performan...
> src/pkg/go/printer/performance_test.go:33: if err :=
> Fprint(ioutil.Discard, fset, testfile); err != nil {
> this cannot possibly return an error, so it's not worth checking.
> Fprint only errors on write, and Discard never errors.
>
> http://codereview.appspot.com/4441078/
>

[r, 2011-04-29 00:04:52]

LGTM

http://codereview.appspot.com/4441078/diff/3005/src/pkg/go/printer/performanc...
File src/pkg/go/printer/performance_test.go (right):

http://codereview.appspot.com/4441078/diff/3005/src/pkg/go/printer/performanc...
src/pkg/go/printer/performance_test.go:47: if bytes.Compare(buf.Bytes(), src) !=
0 {
use bytes.Equal

[gri, 2011-04-29 00:06:37]

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

go/printer: added simple performance benchmark

R=r, dfc, bradfitzwork, bradfitz
CC=golang-dev
http://codereview.appspot.com/4441078