code review 4911043: rpc: add benchmark for async rpc calls

rpc: add benchmark for async rpc calls
Also makes sync benchmark concurrent.

[dvyukov, 2011-08-18 12:02:14]

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

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

[rsc, 2011-08-18 16:38:36]

R=r

[r, 2011-08-19 04:27:39]

http://codereview.appspot.com/4911043/diff/3002/src/pkg/rpc/server_test.go
File src/pkg/rpc/server_test.go (right):

http://codereview.appspot.com/4911043/diff/3002/src/pkg/rpc/server_test.go#ne...
src/pkg/rpc/server_test.go:481: procs := 4 * runtime.GOMAXPROCS(-1)
this breaks history. why do this? if i want parallelism in the benchmark i can
set GOMAXPROCS. the purpose of this benchmark is to test end-to-end performance,
not parallelism.

http://codereview.appspot.com/4911043/diff/3002/src/pkg/rpc/server_test.go#ne...
src/pkg/rpc/server_test.go:489: reply := new(Reply)
why not leave this outside the loops?

[dvyukov, 2011-08-22 13:57:36]

PTAL

http://codereview.appspot.com/4911043/diff/3002/src/pkg/rpc/server_test.go
File src/pkg/rpc/server_test.go (right):

http://codereview.appspot.com/4911043/diff/3002/src/pkg/rpc/server_test.go#ne...
src/pkg/rpc/server_test.go:481: procs := 4 * runtime.GOMAXPROCS(-1)
On 2011/08/19 04:27:39, r wrote:
> this breaks history. why do this? if i want parallelism in the benchmark i can
> set GOMAXPROCS. the purpose of this benchmark is to test end-to-end
performance,
> not parallelism.

Well, the idea was that, due to IO, multiplexing of several goroutines even with
GOMAXPROCS=1 may provide some additional performance (indeed I see performance
improvement from 102394 to 80470 ns/op). And the pattern is frequently used in
real server software - a front-end server creates a goroutine per request, and
that goroutine does sync RPC to a back-end server.
However this is actually not what the change was intended to change. I rolled it
back.

http://codereview.appspot.com/4911043/diff/3002/src/pkg/rpc/server_test.go#ne...
src/pkg/rpc/server_test.go:489: reply := new(Reply)
On 2011/08/19 04:27:39, r wrote:
> why not leave this outside the loops?

Hummm... won't it be shared between several goroutines then? As far as I see it
will mess things up.

[r, 2011-08-23 06:21:29]

I think you misinterpreted what I was saying.

I don't want to lose the non-parallel benchmark. Moreover I think it's important
that a benchmark not depend on GOMAXPROCS. We should preserve the original and
write a new benchmark that checks parallel performance for a fixed number of
goroutines, perhaps overridable by a flag.

What you've done is parallelize the benchmarks in a non-determinstic way.

[rsc, 2011-08-23 12:44:03]

> I don't want to lose the non-parallel benchmark. Moreover I think it's
> important that a benchmark not depend on GOMAXPROCS. We should preserve
> the original and write a new benchmark that checks parallel performance
> for a fixed number of goroutines, perhaps overridable by a flag.

That's not how the benchmarks work.  There is a -cpu flag that lets
you pick how many cpus to use (-cpu=1,2,3,4), and the benchmarks find out
how many to use by querying the GOMAXPROCS setting.

> What you've done is parallelize the benchmarks in a non-determinstic
> way.

It's deterministic: when GOMAXPROCS>1 the benchmarks have -N
appended to their names automatically to distinguish them.

cd $GOROOT/src/pkg/runtime
gotest -bench Append
GOMAXPROCS=2 gotest -bench Append

Russ

[dvyukov, 2011-08-23 13:09:15]

On 2011/08/23 12:44:03, rsc wrote:
> > I don't want to lose the non-parallel benchmark. Moreover I think it's
> > important that a benchmark not depend on GOMAXPROCS. We should preserve
> > the original and write a new benchmark that checks parallel performance
> > for a fixed number of goroutines, perhaps overridable by a flag.
> 
> That's not how the benchmarks work.  There is a -cpu flag that lets
> you pick how many cpus to use (-cpu=1,2,3,4), and the benchmarks find out
> how many to use by querying the GOMAXPROCS setting.
> 
> > What you've done is parallelize the benchmarks in a non-determinstic
> > way.
> 
> It's deterministic: when GOMAXPROCS>1 the benchmarks have -N
> appended to their names automatically to distinguish them.
> 
> cd $GOROOT/src/pkg/runtime
> gotest -bench Append
> GOMAXPROCS=2 gotest -bench Append


Yes, if GOMAXPROCS is not set or -cpu=1 is specified, then this benchmark is
equivalent to the old one. However, now it's very convenient to benchmark it
with several values of GOMAXPROCS:

$ gotest -bench=EndToEnd$ -cpu=1,2,4
rpc.BenchmarkEndToEnd	   10000	    161950 ns/op
rpc.BenchmarkEndToEnd-2	   20000	     80039 ns/op
rpc.BenchmarkEndToEnd-4	   50000	     38252 ns/op

[rsc, 2011-08-26 19:58:30]

http://codereview.appspot.com/4911043/diff/12001/src/pkg/rpc/server_test.go
File src/pkg/rpc/server_test.go (right):

http://codereview.appspot.com/4911043/diff/12001/src/pkg/rpc/server_test.go#n...
src/pkg/rpc/server_test.go:517: func benchmarkEndToEndAsync(dial func()
(*Client, os.Error), b *testing.B) {
I don't think this is an interesting benchmark.
You're doing unbounded parallelism here.

[dvyukov, 2011-08-31 14:57:17]

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

Please take another look.

[rsc, 2011-08-31 17:07:15]

LGTM

[rsc, 2011-08-31 17:07:31]

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

rpc: add benchmark for async rpc calls
Also makes sync benchmark concurrent.

R=r, rsc
CC=golang-dev
http://codereview.appspot.com/4911043

Committer: Russ Cox <rsc@golang.org>