code review 133850043: runtime: round channel size in allocation instead of us...

runtime: round channel size in allocation instead of using system-specific pad field

Followup to CL 128700043.

[rsc, 2014-08-25 16:22:02]

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

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

[bradfitz, 2014-08-25 16:22:48]

LGTM

I pointed this out to Minux too.



On Mon, Aug 25, 2014 at 9:22 AM, <rsc@golang.org> wrote:

> Reviewers: dvyukov,
>
> Message:
> Hello dvyukov (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> runtime: use _64BIT (not GOARCH_amd64) to test for 64-bit system
>
> Followup to CL 128700043.
>
> Please review this at https://codereview.appspot.com/133850043/
>
> Affected files (+1, -1 lines):
>   M src/pkg/runtime/chan.h
>
>
> Index: src/pkg/runtime/chan.h
> ===================================================================
> --- a/src/pkg/runtime/chan.h
> +++ b/src/pkg/runtime/chan.h
> @@ -21,7 +21,7 @@
>         byte*   buf;
>         uint16  elemsize;
>         uint32  closed;
> -#ifndef GOARCH_amd64
> +#ifndef _64BIT /* TODO(rsc): Why is this here? */
>         uint32  pad;                    // ensures proper alignment of the
> buffer that follows Hchan in memory
>  #endif
>         Type*   elemtype;               // element type
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[dvyukov, 2014-08-25 16:24:45]

is amd64p32 _64BIT?

On Mon, Aug 25, 2014 at 8:22 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> LGTM
>
> I pointed this out to Minux too.
>
>
>
> On Mon, Aug 25, 2014 at 9:22 AM, <rsc@golang.org> wrote:
>>
>> Reviewers: dvyukov,
>>
>> Message:
>> Hello dvyukov (cc: golang-codereviews@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go/
>>
>>
>> Description:
>> runtime: use _64BIT (not GOARCH_amd64) to test for 64-bit system
>>
>> Followup to CL 128700043.
>>
>> Please review this at https://codereview.appspot.com/133850043/
>>
>> Affected files (+1, -1 lines):
>>   M src/pkg/runtime/chan.h
>>
>>
>> Index: src/pkg/runtime/chan.h
>> ===================================================================
>> --- a/src/pkg/runtime/chan.h
>> +++ b/src/pkg/runtime/chan.h
>> @@ -21,7 +21,7 @@
>>         byte*   buf;
>>         uint16  elemsize;
>>         uint32  closed;
>> -#ifndef GOARCH_amd64
>> +#ifndef _64BIT /* TODO(rsc): Why is this here? */
>>         uint32  pad;                    // ensures proper alignment of the
>> buffer that follows Hchan in memory
>>  #endif
>>         Type*   elemtype;               // element type
>>
>>
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "golang-codereviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-codereviews+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/d/optout.
>
>

[rsc, 2014-08-25 16:26:52]

On Mon, Aug 25, 2014 at 12:24 PM, Dmitry Vyukov <dvyukov@google.com> wrote:

> is amd64p32 _64BIT?
>

You can answer this by grepping for _64BIT in the source tree. It's in
src/cmd/6c/txt.c:

if(ewidth[TIND] == 8)
dodefine("_64BIT");

So no.

In general, amd64p32 is a 32-bit system with more registers and faster
64-bit math than usual.

Russ

[dvyukov, 2014-08-25 16:37:04]

LGTM

On Mon, Aug 25, 2014 at 8:26 PM, Russ Cox <rsc@golang.org> wrote:
> On Mon, Aug 25, 2014 at 12:24 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>>
>> is amd64p32 _64BIT?
>
>
> You can answer this by grepping for _64BIT in the source tree. It's in
> src/cmd/6c/txt.c:
>
> if(ewidth[TIND] == 8)
> dodefine("_64BIT");
>
> So no.
>
> In general, amd64p32 is a 32-bit system with more registers and faster
> 64-bit math than usual.
>
> Russ
>

[rsc, 2014-08-25 16:42:09]

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

Please take another look.

[rsc, 2014-08-25 16:42:22]

I changed it to do the right thing at the allocation site, and I removed
the pad field.

[dvyukov, 2014-08-25 16:43:23]

LGTM

https://codereview.appspot.com/133850043/diff/80001/src/pkg/runtime/chan.go
File src/pkg/runtime/chan.go (right):

https://codereview.appspot.com/133850043/diff/80001/src/pkg/runtime/chan.go#n...
src/pkg/runtime/chan.go:18: // TODO(dvyukov): make hchan.buf an unsafe.Pointer,
not a *uint8
that was Keith

[rsc, 2014-08-25 16:43:51]

fixed todo

[rsc, 2014-08-25 16:47:18]

will submit once 386 build passes for me.

[rsc, 2014-08-25 17:32:02]

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

runtime: round channel size in allocation instead of using system-specific pad
field

Followup to CL 128700043.

LGTM=bradfitz, dvyukov
R=dvyukov, bradfitz
CC=golang-codereviews
https://codereview.appspot.com/133850043

[gobot, 2014-08-25 18:13:38]

This CL appears to have broken the darwin-386 builder.
See http://build.golang.org/log/52a149468d98a32b60d5e1072d3db8125847acef