code review 4644051: test: A set of scalability benchmarks.

test: A set of scalability benchmarks.
See README file for details.

[dvyukov, 2011-06-20 16:43:53]

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

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

[bradfitz, 2011-06-20 16:58:30]

http://codereview.appspot.com/4644051/diff/4001/test/scale/README
File test/scale/README (right):

http://codereview.appspot.com/4644051/diff/4001/test/scale/README#newcode2
test/scale/README:2: sync.Once implementation and estimate it's impact on
performance/scalability.
its, not it's ("it is")

[dvyukov, 2011-06-20 19:34:40]

Hello golang-dev@googlegroups.com, bradfitz@golang.org (cc: dvyukov@google.com,
golang-dev@googlegroups.com),

Please take another look.

[dvyukov, 2011-06-20 19:37:09]

On 2011/06/20 16:58:30, bradfitz wrote:
> http://codereview.appspot.com/4644051/diff/4001/test/scale/README
> File test/scale/README (right):
> 
> http://codereview.appspot.com/4644051/diff/4001/test/scale/README#newcode2
> test/scale/README:2: sync.Once implementation and estimate it's impact on
> performance/scalability.
> its, not it's ("it is")

Fixed.

I've also uploaded new RESULTS, because it's turned out that I made measurements
with some local changes that significantly affect performance.

[rsc, 2011-06-20 19:40:38]

I stopped after the first source file.
Please make these benchmarks look like any other Go benchmarks.

http://codereview.appspot.com/4644051/diff/5017/test/scale/Makefile
File test/scale/Makefile (right):

http://codereview.appspot.com/4644051/diff/5017/test/scale/Makefile#newcode1
test/scale/Makefile:1: include $(GOROOT)/src/Make.inc
Make this look like all the other makefiles, please.

http://codereview.appspot.com/4644051/diff/5017/test/scale/README
File test/scale/README (right):

http://codereview.appspot.com/4644051/diff/5017/test/scale/README#newcode5
test/scale/README:5: once                     5535       136        114       
120        120        132        123        132        110        127        142
       129        120        118        124        125
Please wrap or truncate (with ...) these long lines.

http://codereview.appspot.com/4644051/diff/5017/test/scale/RESULTS
File test/scale/RESULTS (right):

http://codereview.appspot.com/4644051/diff/5017/test/scale/RESULTS#newcode1
test/scale/RESULTS:1: Obtained with
Please add dates, like in test/bench/timing.log.

http://codereview.appspot.com/4644051/diff/5017/test/scale/channel.go
File test/scale/channel.go (right):

http://codereview.appspot.com/4644051/diff/5017/test/scale/channel.go#newcode4
test/scale/channel.go:4: 
each of these files should start with a comment
saying what the point of the benchmark is, what it's
trying to measure, what the parameters are, and so on.

http://codereview.appspot.com/4644051/diff/5017/test/scale/channel.go#newcode119
test/scale/channel.go:119: benchmarks.Push(&Benchmark{"chan-ring-unbuf", func()
{
The usual way to write this would be

func BenchmarkChanRingUnbuf(b *testing.B) {
    benchmarkRing(b, 0)
}

Instead of inventing your own framework, why not work within the existing one?

[dvyukov, 2011-06-21 09:32:53]

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

Please take another look.

[dvyukov, 2011-06-21 09:48:37]

On 2011/06/20 19:40:38, rsc wrote:
> I stopped after the first source file.
> Please make these benchmarks look like any other Go benchmarks.
> 
> http://codereview.appspot.com/4644051/diff/5017/test/scale/Makefile
> File test/scale/Makefile (right):
> 
> http://codereview.appspot.com/4644051/diff/5017/test/scale/Makefile#newcode1
> test/scale/Makefile:1: include $(GOROOT)/src/Make.inc
> Make this look like all the other makefiles, please.

Done.

> http://codereview.appspot.com/4644051/diff/5017/test/scale/README
> File test/scale/README (right):
> 
> http://codereview.appspot.com/4644051/diff/5017/test/scale/README#newcode5
> test/scale/README:5: once                     5535       136        114       
> 120        120        132        123        132        110        127       
142
>        129        120        118        124        125
> Please wrap or truncate (with ...) these long lines.

Done.

> http://codereview.appspot.com/4644051/diff/5017/test/scale/RESULTS
> File test/scale/RESULTS (right):
> 
> http://codereview.appspot.com/4644051/diff/5017/test/scale/RESULTS#newcode1
> test/scale/RESULTS:1: Obtained with
> Please add dates, like in test/bench/timing.log.

Done.
I've also renamed it to timing.log for consistency.

> http://codereview.appspot.com/4644051/diff/5017/test/scale/channel.go
> File test/scale/channel.go (right):
> 
> http://codereview.appspot.com/4644051/diff/5017/test/scale/channel.go#newcode4
> test/scale/channel.go:4: 
> each of these files should start with a comment
> saying what the point of the benchmark is, what it's
> trying to measure, what the parameters are, and so on.

Done.

>
http://codereview.appspot.com/4644051/diff/5017/test/scale/channel.go#newcode119
> test/scale/channel.go:119: benchmarks.Push(&Benchmark{"chan-ring-unbuf",
func()
> {
> The usual way to write this would be
> 
> func BenchmarkChanRingUnbuf(b *testing.B) {
>     benchmarkRing(b, 0)
> }
> 
> Instead of inventing your own framework, why not work within the existing one?

Do you mean gotest, or merely testing.B?
I do not see how I can use testing.B (there is no way to setup name and target
function).
As for gotest, I started with it, but then switched to own framework. There are
several moderate reasons for that rather than single showstopper.
First, it does not support variation of GOMAXPROCS. I would have to create own
wrapper program and launch a lot of gotests with different values of GOMAXPROCS.
gotest output is inappropriate for scalability testing (impossible to parse with
eyes). What is required is exactly:
was:
test-name   100  120  130  80
became:
test-name   100  150  200  250
as for parsing for graph generation, it would be harder as well.
Then, gotest has a hard-coded execution time limit. 1 second is too small to
final benchmarking (concurrent benchmarks are especially subject to variations),
while too long for intermediate runs.
With own framework it's trivial to work around all those issues (as well as any
future).

[rsc, 2011-06-21 16:20:00]

> First, it does not support variation of GOMAXPROCS. I would have to
> create own wrapper program and launch a lot of gotests with different
> values of GOMAXPROCS.

Let's fix that.

> gotest output is inappropriate for scalability testing (impossible to
> parse with eyes). What is required is exactly:
> was:
> test-name   100  120  130  80
> became:
> test-name   100  150  200  250
> as for parsing for graph generation, it would be harder as well.

You can easily write a script to read the gotest output and
generate any summary you like.  I think the current summaries
are actually quite hard to read at scale, and they don't contain
enough information to reproduce.  It would be better to have
the one result per line raw data of the current benchmarks + a script
to generate nice summaries.

> Then, gotest has a hard-coded execution time limit. 1 second is too
> small to final benchmarking (concurrent benchmarks are especially
> subject to variations), while too long for intermediate runs.

Let's fix that.

> With own framework it's trivial to work around all those issues (as well
> as any future).

Yes but there is a standard framework.  Everyone can build their
own framework and duplicate effort, or we can all share a single
framework and do the work once.

Russ

[dvyukov, 2011-06-22 11:52:12]

I do not mind improving gotest. I will post the proposal to golang-nuts.
I will have to do manual load balancing in benchmarks, though.

[rsc, 2011-06-22 14:52:30]

On Wed, Jun 22, 2011 at 07:52,  <dvyukov@google.com> wrote:
> I do not mind improving gotest. I will post the proposal to golang-nuts.
> I will have to do manual load balancing in benchmarks, though.

Okay, thanks.  Let's get gotest's benchmarks set to
accommodate what these tests need and then revisit
this CL once they can use the standard framework.

Russ

[dvyukov, 2011-06-22 14:54:15]

On 2011/06/22 14:52:30, rsc wrote:
> On Wed, Jun 22, 2011 at 07:52,  <mailto:dvyukov@google.com> wrote:
> > I do not mind improving gotest. I will post the proposal to golang-nuts.
> > I will have to do manual load balancing in benchmarks, though.
> 
> Okay, thanks.  Let's get gotest's benchmarks set to
> accommodate what these tests need and then revisit
> this CL once they can use the standard framework.

Please join the discussion here:
https://groups.google.com/forum/#!topic/golang-nuts/caAPVjOFFxQ

[dvyukov, 2011-06-23 12:29:43]

Russ,

I guess the next step after we land gotest changes is to rewrite the benchmarks
to testing.B and to put them into packages they are benchmarking. Right?
We decided to put matrix multiplication benchmark into pkg/runtime as it tests
mostly goroutine creation and scheduling.
Then I need to rewrite graph generation script to work with gotest output. As
far as I see there is no place in Go source tree for such scripts now. Btw, you
mentioned that you have a script that prints summaries like
http://code.google.com/p/go/source/detail?r=4fb77130a7ef. Had you put it
somewhere? I would use it.

[rsc, 2011-06-27 13:16:37]

> I guess the next step after we land gotest changes is to rewrite the
> benchmarks to testing.B and to put them into packages they are
> benchmarking. Right?
> We decided to put matrix multiplication benchmark into pkg/runtime as it
> tests mostly goroutine creation and scheduling.
> Then I need to rewrite graph generation script to work with gotest
> output. As far as I see there is no place in Go source tree for such
> scripts now. Btw, you mentioned that you have a script that prints
> summaries like http://code.google.com/p/go/source/detail?r=4fb77130a7ef.
> Had you put it somewhere? I would use it.

You run old and new benchmarks and then just pipe the
output through

#!/bin/sh
awk '
BEGIN {
	printf("%-45s %12s %12s  %7s\n", "benchmark", "old ns/op", "new
ns/op", "delta")
}

$1 ~ /Benchmark/ && $4 == "ns/op" {
	if(old[$1]) {
		printf("%-45s %12d %12d  %6s%%\n", $1, old[$1], $3,
			sprintf("%+.2f", 100*$3/old[$1]-100));
	}
	old[$1] = $3;
}
' "$@"

[dvyukov, 2011-06-27 13:36:06]

Thanks! Works for me!

[dvyukov, 2011-06-28 10:16:30]

*** Abandoned ***

[dvyukov, 2011-06-28 10:18:00]

On 2011/06/21 16:20:00, rsc wrote:
> > First, it does not support variation of GOMAXPROCS. I would have to
> > create own wrapper program and launch a lot of gotests with different
> > values of GOMAXPROCS.
> 
> Let's fix that.
> 
> > gotest output is inappropriate for scalability testing (impossible to
> > parse with eyes). What is required is exactly:
> > was:
> > test-name   100  120  130  80
> > became:
> > test-name   100  150  200  250
> > as for parsing for graph generation, it would be harder as well.
> 
> You can easily write a script to read the gotest output and
> generate any summary you like.  I think the current summaries
> are actually quite hard to read at scale, and they don't contain
> enough information to reproduce.  It would be better to have
> the one result per line raw data of the current benchmarks + a script
> to generate nice summaries.
> 
> > Then, gotest has a hard-coded execution time limit. 1 second is too
> > small to final benchmarking (concurrent benchmarks are especially
> > subject to variations), while too long for intermediate runs.
> 
> Let's fix that.
> 
> > With own framework it's trivial to work around all those issues (as well
> > as any future).
> 
> Yes but there is a standard framework.  Everyone can build their
> own framework and duplicate effort, or we can all share a single
> framework and do the work once.

I think you were ultimately right. Thanks!

[rsc, 2011-06-28 12:08:05]

Thank you for being so open to suggestions.