code review 7026044: runtime: faster segmented stacks

runtime: faster segmented stacks
benchmark                      old ns/op    new ns/op    delta
BenchmarkStackGrowth                 665          548  -17.59%
BenchmarkStackGrowth-2               333          274  -17.72%
BenchmarkStackGrowth-4               224          168  -25.00%
BenchmarkStackGrowth-8               124           91  -26.21%
BenchmarkStackGrowth-16               82           70  -14.55%
BenchmarkStackGrowth-32               73           59  -19.49%

[dvyukov, 2013-01-03 14:31:58]

Please review the following change

[dvyukov, 2013-01-03 15:27:50]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://dvyukov%40google.com@code.google.com/p/go/

[nigeltao, 2013-01-04 01:03:33]

https://codereview.appspot.com/7026044/diff/7001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/7026044/diff/7001/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1125: label.sp = top->gobuf.sp;
Can this be
label = top->gobuf;

[dvyukov, 2013-01-04 05:30:31]

https://codereview.appspot.com/7026044/diff/7001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/7026044/diff/7001/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1125: label.sp = top->gobuf.sp;
On 2013/01/04 01:03:33, nigeltao wrote:
> Can this be
> label = top->gobuf;

You're right. Thanks!
Done.
Compiler generates inefficient code for copy of whole Stktop struct (as in
original code). But  Gobuf is smaller, and struct copy code for it is equivalent
to my manual member-by-member copy.

[minux1, 2013-01-05 14:09:00]

https://codereview.appspot.com/7026044/diff/3008/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/7026044/diff/3008/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1117: dstend = dst + argsize/PtrSize;
how about s/PtrSize/sizeof(*dst)/ ?

https://codereview.appspot.com/7026044/diff/3008/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1168: if(reflectcall) {
so for non reflect.calls, framesize will always be bigger
than minalloc?

https://codereview.appspot.com/7026044/diff/3008/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1223: dstend = dst + argsize/PtrSize;
ditto.

[rsc, 2013-01-07 05:28:47]

https://codereview.appspot.com/7026044/diff/3008/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/7026044/diff/3008/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1117: dstend = dst + argsize/PtrSize;
On 2013/01/05 14:09:00, minux wrote:
> how about s/PtrSize/sizeof(*dst)/ ?

Yes please.

However, this word-based copy is only correct if we know argsize is always an
exact multiple of PtrSize. That's reasonable to demand, but I am not sure if the
compilers guarantee it just yet. If you change the linkers to reject object
files with non-conforming argument sizes, then I'll feel a lot more comfortable
about this.

https://codereview.appspot.com/7026044/diff/3008/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1119: for(; dst != dstend; dst++, src++)
while(dst < dstend)
    *dst++ = *src++;

https://codereview.appspot.com/7026044/diff/3008/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1168: if(reflectcall) {
On 2013/01/05 14:09:00, minux wrote:
> so for non reflect.calls, framesize will always be bigger
> than minalloc?

minalloc is a clumsy hack added so that the garbage collector can use
a reflect call to get running on a fresh stack frame. It doesn't get used
otherwise.

https://codereview.appspot.com/7026044/diff/3008/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1223: dstend = dst + argsize/PtrSize;
ditto.

[dvyukov, 2013-01-08 12:59:02]

PTAL

https://codereview.appspot.com/7026044/diff/3008/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/7026044/diff/3008/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1117: dstend = dst + argsize/PtrSize;
On 2013/01/05 14:09:00, minux wrote:
> how about s/PtrSize/sizeof(*dst)/ ?

Done.

https://codereview.appspot.com/7026044/diff/3008/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1117: dstend = dst + argsize/PtrSize;
On 2013/01/07 05:28:47, rsc wrote:
> On 2013/01/05 14:09:00, minux wrote:
> > how about s/PtrSize/sizeof(*dst)/ ?
> 
> Yes please.
> 
> However, this word-based copy is only correct if we know argsize is always an
> exact multiple of PtrSize. That's reasonable to demand, but I am not sure if
the
> compilers guarantee it just yet. If you change the linkers to reject object
> files with non-conforming argument sizes, then I'll feel a lot more
comfortable
> about this.

But newstack() contains:
if(argsize % sizeof(uintptr) != 0) throw();
Doesn't argsize is the same in newstack and oldstack?

https://codereview.appspot.com/7026044/diff/3008/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1119: for(; dst != dstend; dst++, src++)
On 2013/01/07 05:28:47, rsc wrote:
> while(dst < dstend)
>     *dst++ = *src++;

Done.

https://codereview.appspot.com/7026044/diff/3008/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1168: if(reflectcall) {
On 2013/01/07 05:28:47, rsc wrote:
> On 2013/01/05 14:09:00, minux wrote:
> > so for non reflect.calls, framesize will always be bigger
> > than minalloc?
> 
> minalloc is a clumsy hack added so that the garbage collector can use
> a reflect call to get running on a fresh stack frame. It doesn't get used
> otherwise.

I've added a comment here.

https://codereview.appspot.com/7026044/diff/3008/src/pkg/runtime/proc.c#newco...
src/pkg/runtime/proc.c:1223: dstend = dst + argsize/PtrSize;
On 2013/01/05 14:09:00, minux wrote:
> ditto.

Done.

[rsc, 2013-01-09 19:13:24]

LGTM

[dvyukov, 2013-01-10 07:45:35]

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

runtime: faster segmented stacks
benchmark                      old ns/op    new ns/op    delta
BenchmarkStackGrowth                 665          548  -17.59%
BenchmarkStackGrowth-2               333          274  -17.72%
BenchmarkStackGrowth-4               224          168  -25.00%
BenchmarkStackGrowth-8               124           91  -26.21%
BenchmarkStackGrowth-16               82           70  -14.55%
BenchmarkStackGrowth-32               73           59  -19.49%

R=nigeltao, minux.ma, rsc
CC=golang-dev
https://codereview.appspot.com/7026044