code review 6278050: go.image/tiff: add a benchmark for the encoder

go.image/tiff: add a benchmark for the encoder

The idea is to see whether it is worthwhile to implement special cases.

On my Mac Book Pro:

BenchmarkEncode	    1000	   1689656 ns/op	  36.69 MB/s

[bsiegert, 2012-06-04 17:06:32]

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

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

[minux1, 2012-06-04 18:09:51]

http://codereview.appspot.com/6278050/diff/5001/tiff/writer_test.go
File tiff/writer_test.go (right):

http://codereview.appspot.com/6278050/diff/5001/tiff/writer_test.go#newcode56
tiff/writer_test.go:56: b.StartTimer()
you can use b.ResetTimer() instead of StopTimer+StartTimer.

http://codereview.appspot.com/6278050/diff/5001/tiff/writer_test.go#newcode58
tiff/writer_test.go:58: out.Truncate(0)
why not out.Reset()?

[nigeltao, 2012-06-05 01:12:55]

http://codereview.appspot.com/6278050/diff/5001/tiff/writer_test.go
File tiff/writer_test.go (right):

http://codereview.appspot.com/6278050/diff/5001/tiff/writer_test.go#newcode59
tiff/writer_test.go:59: err = Encode(out, img)
Use ioutil.Discard instead of out.

http://codereview.appspot.com/6278050/diff/5001/tiff/writer_test.go#newcode64
tiff/writer_test.go:64: b.SetBytes(int64(out.Len()))
I suppose that you could argue either way, but the image/png encoding benchmarks
(see $GOROOT/src/pkg/image/png/writer_test.go) calls SetBytes with the size of
the decompressed image, not the compressed image. That means that achieving
better compression without being actually slower doesn't result in a 'worse'
MB/s number.

[bsiegert, 2012-06-05 17:43:48]

Hello nigeltao@golang.org, minux.ma@gmail.com (cc: golang-dev@googlegroups.com),

Please take another look.

[bsiegert, 2012-06-05 17:44:57]

http://codereview.appspot.com/6278050/diff/5001/tiff/writer_test.go
File tiff/writer_test.go (right):

http://codereview.appspot.com/6278050/diff/5001/tiff/writer_test.go#newcode56
tiff/writer_test.go:56: b.StartTimer()
On 2012/06/04 18:09:51, minux wrote:
> you can use b.ResetTimer() instead of StopTimer+StartTimer.

Done.

http://codereview.appspot.com/6278050/diff/5001/tiff/writer_test.go#newcode58
tiff/writer_test.go:58: out.Truncate(0)
On 2012/06/04 18:09:51, minux wrote:
> why not out.Reset()?

Yes, thanks. However, if I use ioutil.Discard, this is not necessary.

http://codereview.appspot.com/6278050/diff/5001/tiff/writer_test.go#newcode59
tiff/writer_test.go:59: err = Encode(out, img)
On 2012/06/05 01:12:56, nigeltao wrote:
> Use ioutil.Discard instead of out.

I originally used the Buffer to have the length for the SetBytes call. However,
as per your suggestion, I just use the number of raw bytes.

http://codereview.appspot.com/6278050/diff/5001/tiff/writer_test.go#newcode64
tiff/writer_test.go:64: b.SetBytes(int64(out.Len()))
On 2012/06/05 01:12:56, nigeltao wrote:
> I suppose that you could argue either way, but the image/png encoding
benchmarks
> (see $GOROOT/src/pkg/image/png/writer_test.go) calls SetBytes with the size of
> the decompressed image, not the compressed image. That means that achieving
> better compression without being actually slower doesn't result in a 'worse'
> MB/s number.

Thanks for the suggestion. Done.

[nigeltao, 2012-06-06 00:04:03]

http://codereview.appspot.com/6278050/diff/10001/tiff/writer_test.go
File tiff/writer_test.go (right):

http://codereview.appspot.com/6278050/diff/10001/tiff/writer_test.go#newcode31
tiff/writer_test.go:31: img, err := openImage(testdataDir + filename)
Drop the "testdataDir".

http://codereview.appspot.com/6278050/diff/10001/tiff/writer_test.go#newcode51
tiff/writer_test.go:51: img, err := openImage(testdataDir + "video-001.tiff")
Drop the "testdataDir".

[bsiegert, 2012-06-06 21:02:52]

All done, PTAL. Funny that the tests still passed, "../testdata/../testdata" is
the same directory.

http://codereview.appspot.com/6278050/diff/10001/tiff/writer_test.go
File tiff/writer_test.go (right):

http://codereview.appspot.com/6278050/diff/10001/tiff/writer_test.go#newcode31
tiff/writer_test.go:31: img, err := openImage(testdataDir + filename)
On 2012/06/06 00:04:03, nigeltao wrote:
> Drop the "testdataDir".

Done.

http://codereview.appspot.com/6278050/diff/10001/tiff/writer_test.go#newcode51
tiff/writer_test.go:51: img, err := openImage(testdataDir + "video-001.tiff")
On 2012/06/06 00:04:03, nigeltao wrote:
> Drop the "testdataDir".

Done.

[nigeltao, 2012-06-06 23:36:38]

LGTM.

[nigeltao, 2012-06-06 23:41:47]

*** Submitted as
http://code.google.com/p/go/source/detail?r=37300243245c&repo=image ***

go.image/tiff: add a benchmark for the encoder

The idea is to see whether it is worthwhile to implement special cases.

On my Mac Book Pro:

BenchmarkEncode	    1000	   1689656 ns/op	  36.69 MB/s

R=nigeltao, minux.ma
CC=golang-dev
http://codereview.appspot.com/6278050

Committer: Nigel Tao <nigeltao@golang.org>