code review 4910041: runtime: speed up cgo calls

runtime: speed up cgo calls

Allocate Defer on stack during cgo calls, as suggested
by dvyukov. Also includes some comment corrections.

benchmark                   old,ns/op   new,ns/op
BenchmarkCgoCall                  669         330
(Intel Xeon CPU 1.80GHz * 4, Linux 386)

[brainman, 2011-08-16 02:49:32]

Hello golang-dev@googlegroups.com,

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

[dvyukov, 2011-08-16 10:54:04]

I am a bit skeptical about making G any bigger. Can't we move sparedefer to M?

Another option is to allocate Defer directly on stack (add onstack flag to Defer
and do not free it the case of panic). As a bonus it will speed up cgo callbacks
as well. Russ, what do you think on this?

In the long term I see it as per-M cache of Defers with limited number of args
(say no more than 2 uint64).

[rsc, 2011-08-16 18:56:19]

Dmitriy's suggestion sounds good to me.
Can you try adding a flag to struct Defer,
like bool nofree?  Then you don't need
sparedefer, and other fixed defers can
reuse the mechanism.

Russ

[brainman, 2011-08-17 02:17:56]

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

Please take another look.

[dvyukov, 2011-08-17 07:32:38]

Looks splendid to me

http://codereview.appspot.com/4910041/diff/2008/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

http://codereview.appspot.com/4910041/diff/2008/src/pkg/runtime/proc.c#newcod...
src/pkg/runtime/proc.c:1126: runtime·memmove(d->args, d->argp, d->siz);
I think it's better to add explicit d->nofree = false here. It won't be visible
against the malloc background.

[brainman, 2011-08-17 12:45:33]

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

Please take another look.

[brainman, 2011-08-17 12:48:07]

Thank you for review.

http://codereview.appspot.com/4910041/diff/2008/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

http://codereview.appspot.com/4910041/diff/2008/src/pkg/runtime/proc.c#newcod...
src/pkg/runtime/proc.c:1126: runtime·memmove(d->args, d->argp, d->siz);
On 2011/08/17 07:32:38, dvyukov wrote:
> I think it's better to add explicit d->nofree = false here. It won't be
visible
> against the malloc background.

Done. Sometimes I forget, this is not Go, and I can't assume default values.

Good that you told me, because one of the tests failed. nofree field can not be
last field of Defer, so I moved it up high.

I also had problems when I put nofree in between link and args, I suspect, it
has to do with aligning. I hope someone could test this on amd64 before I
submit.

[dvyukov, 2011-08-17 13:25:44]

On 2011/08/17 12:48:07, brainman wrote:
> Thank you for review.
> 
> http://codereview.appspot.com/4910041/diff/2008/src/pkg/runtime/proc.c
> File src/pkg/runtime/proc.c (right):
> 
>
http://codereview.appspot.com/4910041/diff/2008/src/pkg/runtime/proc.c#newcod...
> src/pkg/runtime/proc.c:1126: runtime·memmove(d->args, d->argp, d->siz);
> On 2011/08/17 07:32:38, dvyukov wrote:
> > I think it's better to add explicit d->nofree = false here. It won't be
> visible
> > against the malloc background.
> 
> Done. Sometimes I forget, this is not Go, and I can't assume default values.
> 
> Good that you told me, because one of the tests failed. nofree field can not
be
> last field of Defer, so I moved it up high.
> 
> I also had problems when I put nofree in between link and args, I suspect, it
> has to do with aligning. I hope someone could test this on amd64 before I
> submit.

Oh boy, I was looking at Panic instead of Defer somehow. Panic's last member is
bool recovered, so additional bool fits nicely after it.

You can't put nofree after args, because args is a fake member. Real Defer can
be larger than the struct:
	d = runtime·malloc(sizeof(*d) + siz - sizeof(d->args));
And so nofree was overwritten when real args are copied into defer:
	runtime·memmove(d->args, d->argp, d->siz);

You can't put nofree between link and args because then args are indeed aligned
improperly. I think you can put it there if you make it uintptr.

I've tested the latest version on linux/amd64 (executed gotest in runtime and
misc/cgo/test). No panics and no failed tests.

[dvyukov, 2011-08-17 13:32:26]

I've benchmarked it on Xeon E5620 2.40GHz, and it's 50 vs 140 ns.

[rsc, 2011-08-17 15:22:17]

nice

http://codereview.appspot.com/4910041/diff/16001/src/pkg/runtime/cgocall.c
File src/pkg/runtime/cgocall.c (right):

http://codereview.appspot.com/4910041/diff/16001/src/pkg/runtime/cgocall.c#ne...
src/pkg/runtime/cgocall.c:137: if(d.nofree) {
You are assuming that d is zeroed to begin with, but it's not.
Insert d.nofree = false before line 109 (outside the if).

http://codereview.appspot.com/4910041/diff/16001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

http://codereview.appspot.com/4910041/diff/16001/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1127: d->nofree = false;
This you don't need to do, because
runtime.malloc zeros the memory it returns.

http://codereview.appspot.com/4910041/diff/16001/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

http://codereview.appspot.com/4910041/diff/16001/src/pkg/runtime/runtime.h#ne...
src/pkg/runtime/runtime.h:363: bool	nofree;
Please move this up between siz and argp.
On a 32-bit machine it will take up 4 bytes
no matter where we put it,
but on a 64-bit machine there are 4 padding
bytes between siz and argp and it can steal
one of those.

[dvyukov, 2011-08-17 15:40:35]

On Wed, Aug 17, 2011 at 7:22 PM,  <rsc@golang.org> wrote:
>
http://codereview.appspot.com/4910041/diff/16001/src/pkg/runtime/proc.c#newco...
> src/pkg/runtime/proc.c:1127: d->nofree = false;
> This you don't need to do, because
> runtime.malloc zeros the memory it returns.

I think it at least deserves a comment  that we expect it to be false.

[rsc, 2011-08-17 15:49:54]

On Wed, Aug 17, 2011 at 11:40, Dmitry Vyukov <dvyukov@google.com> wrote:

> On Wed, Aug 17, 2011 at 7:22 PM,  <rsc@golang.org> wrote:
> >
>
http://codereview.appspot.com/4910041/diff/16001/src/pkg/runtime/proc.c#newco...
> > src/pkg/runtime/proc.c:1127: d->nofree = false;
> > This you don't need to do, because
> > runtime.malloc zeros the memory it returns.
>
> I think it at least deserves a comment  that we expect it to be false.
>

The whole point of calling it nofree is so that
the zero value is the correct default.
We don't bother filling in zero fields.

[brainman, 2011-08-18 02:39:41]

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

Please take another look.

[brainman, 2011-08-18 02:40:39]

http://codereview.appspot.com/4910041/diff/16001/src/pkg/runtime/cgocall.c
File src/pkg/runtime/cgocall.c (right):

http://codereview.appspot.com/4910041/diff/16001/src/pkg/runtime/cgocall.c#ne...
src/pkg/runtime/cgocall.c:137: if(d.nofree) {
On 2011/08/17 15:22:17, rsc wrote:
> You are assuming that d is zeroed to begin with, but it's not.
> Insert d.nofree = false before line 109 (outside the if).

Done.

http://codereview.appspot.com/4910041/diff/16001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

http://codereview.appspot.com/4910041/diff/16001/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1127: d->nofree = false;
On 2011/08/17 15:22:17, rsc wrote:
> This you don't need to do, because
> runtime.malloc zeros the memory it returns.

Done.

http://codereview.appspot.com/4910041/diff/16001/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

http://codereview.appspot.com/4910041/diff/16001/src/pkg/runtime/runtime.h#ne...
src/pkg/runtime/runtime.h:363: bool	nofree;
On 2011/08/17 15:22:17, rsc wrote:
> but on a 64-bit machine there are 4 padding
> bytes between siz and argp and it can steal
> one of those.

Woopi! "Money for nothing and your chicks for free ..."

Sorry ... Done.

[rsc, 2011-08-18 16:16:52]

LGTM

[rsc, 2011-08-18 16:17:23]

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

runtime: speed up cgo calls

Allocate Defer on stack during cgo calls, as suggested
by dvyukov. Also includes some comment corrections.

benchmark                   old,ns/op   new,ns/op
BenchmarkCgoCall                  669         330
(Intel Xeon CPU 1.80GHz * 4, Linux 386)

R=dvyukov, rsc
CC=golang-dev
http://codereview.appspot.com/4910041

Committer: Russ Cox <rsc@golang.org>