code review 72360043: runtime: round stack size to power of 2.

runtime: round stack size to power of 2.
Fixes build on windows/386 and plan9/386.
Fixes issue 7487.

[minux1, 2014-03-07 06:31:33]

Hello golang-codereviews@googlegroups.com,

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

[minux1, 2014-03-07 06:34:57]

PTAL.

[mattn, 2014-03-07 06:42:14]

LGTM

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

https://codereview.appspot.com/72360043/diff/70001/src/pkg/runtime/proc.c#new...
src/pkg/runtime/proc.c:1945: stksize = round2(FixedStack);
Is this need to call each time?

[dvyukov, 2014-03-07 06:46:53]

LGTM

[0intro, 2014-03-07 06:54:25]

Thanks. It also fixes the issue on Plan 9. Please add "plan9/386" in your
description.

[dvyukov, 2014-03-07 07:05:03]

On 2014/03/07 06:46:53, dvyukov wrote:
> LGTM

not lgtm

this will effectively disable fixed stack segment cache, because it caches
segments of FixedStack size
probably you need to adjust FixedStack constant instead

[minux1, 2014-03-07 07:05:41]

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

https://codereview.appspot.com/72360043/diff/70001/src/pkg/runtime/proc.c#new...
src/pkg/runtime/proc.c:1945: stksize = round2(FixedStack);
On 2014/03/07 06:42:15, mattn wrote:
> Is this need to call each time?
in theory, no. but i'm yet to find a way to maintain
that FixedStack is a power-of-2 in stack.h because
it's defined as the sum of StackMin and StackSystem.

Suggestions welcome.

we certainly can add a comment to stack.h and
check it at runtime. What do you think?

[minux1, 2014-03-07 07:09:49]

On 2014/03/07 07:05:03, dvyukov wrote:
> this will effectively disable fixed stack segment cache, because it caches
> segments of FixedStack size
> probably you need to adjust FixedStack constant instead
How about we do this:

make the enum _FixedStack.
and
#define FixedStack round2(_FixedStack)
this has will make the function call each time.

Alternatively,
we can modify StackSystem to be always 4096 (== StackMin).

which do you prefer?

[mattn, 2014-03-07 07:11:57]

I prefer second.

[mattn, 2014-03-07 07:15:36]

On 2014/03/07 07:11:57, mattn wrote:
> I prefer second.

Or, global variable.

int FixedStack = -1;

// And at somewhere

if (FixedStack == -1) FixedStack = round2(FixedStack);

[dvyukov, 2014-03-07 07:18:24]

On Fri, Mar 7, 2014 at 11:09 AM,  <minux.ma@gmail.com> wrote:
> On 2014/03/07 07:05:03, dvyukov wrote:
>>
>> this will effectively disable fixed stack segment cache, because it
>
> caches
>>
>> segments of FixedStack size
>> probably you need to adjust FixedStack constant instead
>
> How about we do this:
>
> make the enum _FixedStack.
> and
> #define FixedStack round2(_FixedStack)
> this has will make the function call each time.
>
> Alternatively,
> we can modify StackSystem to be always 4096 (== StackMin).

this will waste lots of memory on StackGuard


I would manually calculate the constants and the check all necessary
properties of the constants at startup
being able to see the real numbers is useful



> which do you prefer?
>
> https://codereview.appspot.com/72360043/

[minux1, 2014-03-07 16:43:07]

I changed FixedStack to a variable, however it seems to break every
platform.
(go_bootstrap runs fine, but cmd/go just SIGSEGV and silently exits)

Does anyone see problem in the code? I'm pretty confused.

[minux1, 2014-03-07 19:18:23]

PTAL.

[rsc, 2014-03-07 19:19:27]

This CL seems too complex. I propose to just bump 386 to 4k like amd64 is
already. We are already reserving 2k per goroutine. Bumping that to 4k
seems like not a big deal. After all the minimum stack size just came down
from 8k to 4k, so we're still ahead of Go 1.2 as far as minimal goroutine
footprint. I submitted CL 72600043 to fix the windows builder. We can worry
about other cleanup separately.

[rsc, 2014-03-07 19:20:50]

I sent that comment before your PTAL. Your new version is nicer - I just wanted
to get the build fixed. If you want to update your CL to build against tip,
please do.

Thanks.

[minux1, 2014-03-07 19:25:11]

Hello golang-codereviews@googlegroups.com, mattn.jp@gmail.com,
dvyukov@google.com, 0intro@gmail.com, rsc@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[rsc, 2014-03-07 19:30:34]

https://codereview.appspot.com/72360043/diff/130002/src/pkg/runtime/stack.h
File src/pkg/runtime/stack.h (right):

https://codereview.appspot.com/72360043/diff/130002/src/pkg/runtime/stack.h#n...
src/pkg/runtime/stack.h:80: // Do not use this constant directly, use the
variable FixedStack
StackSystemRounded = StackSystem + (-StackSystem & (StackMin-1)),
FixedStack = StackMin + StackSystemRounded,

[minux1, 2014-03-07 19:36:51]

PTAL.

On Fri, Mar 7, 2014 at 2:30 PM, <rsc@golang.org> wrote:

> https://codereview.appspot.com/72360043/diff/130002/src/
> pkg/runtime/stack.h#newcode80
> src/pkg/runtime/stack.h:80: // Do not use this constant directly, use
> the variable FixedStack
> StackSystemRounded = StackSystem + (-StackSystem & (StackMin-1)),
> FixedStack = StackMin + StackSystemRounded,
>
Aha, this is really cool!

[rsc, 2014-03-07 19:44:07]

LGTM

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

https://codereview.appspot.com/72360043/diff/230001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:146: if(FixedStack != runtime·round2(FixedStack))
this is the sort of thing that belongs in runtime.check

[0intro, 2014-03-07 19:51:32]

It works for me.

[minux1, 2014-03-07 20:11:29]

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

runtime: round stack size to power of 2.
Fixes build on windows/386 and plan9/386.
Fixes issue 7487.

LGTM=mattn.jp, dvyukov, rsc
R=golang-codereviews, mattn.jp, dvyukov, 0intro, rsc
CC=golang-codereviews
https://codereview.appspot.com/72360043