code review 9678046: runtime: remove unused filed from Hchan (issue 9678046)

[dvy...@google.com]

Reviewers: golang-dev1,

Message:
Hello golan...@googlegroups.com,

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


Description:
runtime: remove unused filed from Hchan
Remove alignment logic as well, it's not respected by chanbuf() anyway.

Please review this at https://codereview.appspot.com/9678046/

Affected files:
M src/pkg/runtime/chan.c


Index: src/pkg/runtime/chan.c
===================================================================
--- a/src/pkg/runtime/chan.c
+++ b/src/pkg/runtime/chan.c
@@ -39,7 +39,6 @@
uintgo dataqsiz; // size of the circular q
uint16 elemsize;
bool closed;
- uint8 elemalign;
Alg* elemalg; // interface for element type
uintgo sendx; // send index
uintgo recvx; // receive index
@@ -93,7 +92,6 @@
runtime·makechan_c(ChanType *t, int64 hint)
{
Hchan *c;
- uintptr n;
Type *elem;

elem = t->elem;
@@ -105,22 +103,19 @@
if(hint < 0 || (intgo)hint != hint || (elem->size > 0 && hint > MaxMem /
elem->size))
runtime·panicstring("makechan: size out of range");

- // calculate rounded size of Hchan
- n = sizeof(*c);
- while(n & MAXALIGN)
- n++;
+ if(sizeof(*c)&MAXALIGN)
+ runtime·throw("makechan: bad chan size");

// allocate memory in one call
- c = (Hchan*)runtime·mal(n + hint*elem->size);
+ c = (Hchan*)runtime·mal(sizeof(*c) + hint*elem->size);
c->elemsize = elem->size;
c->elemalg = elem->alg;
- c->elemalign = elem->align;
c->dataqsiz = hint;
runtime·settype(c, (uintptr)t | TypeInfo_Chan);

if(debug)
- runtime·printf("makechan: chan=%p; elemsize=%D; elemalg=%p;
elemalign=%d; dataqsiz=%D\n",
- c, (int64)elem->size, elem->alg, elem->align, (int64)c->dataqsiz);
+ runtime·printf("makechan: chan=%p; elemsize=%D; elemalg=%p;
dataqsiz=%D\n",
+ c, (int64)elem->size, elem->alg, (int64)c->dataqsiz);

return c;
}

[da...@cheney.net]

On more i5 linux/amd64 machine this appears to have made things a little
slower

lucky(~/go/src/pkg/runtime) % ~/go/misc/benchcmp {old,new}.txt
benchmark old ns/op new ns/op delta
BenchmarkChanUncontended 71 70 -0.14%
BenchmarkChanContended 71 71 -0.56%
BenchmarkChanSync 146 152 +4.11%
BenchmarkChanProdCons0 147 156 +6.12%
BenchmarkChanProdCons10 90 96 +5.95%
BenchmarkChanProdCons100 69 70 +2.16%
BenchmarkChanProdConsWork0 774 777 +0.39%
BenchmarkChanProdConsWork10 713 719 +0.84%
BenchmarkChanProdConsWork100 688 691 +0.44%
BenchmarkChanCreation 181 186 +2.76%
BenchmarkChanSem 62 64 +3.51%


https://codereview.appspot.com/9678046/diff/5001/src/pkg/runtime/chan.c
File src/pkg/runtime/chan.c (right):

https://codereview.appspot.com/9678046/diff/5001/src/pkg/runtime/chan.c#newcode107
src/pkg/runtime/chan.c:107: runtime·throw("makechan: bad chan size");
Will the compiled down to a constant ? ie, sizeof(*c) is known at
compile time, as is MAXALIGN, so this could compile down to if (0) and
be removed ? I guess I should check the output of the compiler.

https://codereview.appspot.com/9678046/

[da...@cheney.net]

> lucky(~/go/src/pkg/runtime) % ~/go/misc/benchcmp {old,new}.txt
> benchmark old ns/op new ns/op delta
> BenchmarkChanUncontended 71 70 -0.14%
> BenchmarkChanContended 71 71 -0.56%
> BenchmarkChanSync 146 152 +4.11%
> BenchmarkChanProdCons0 147 156 +6.12%
> BenchmarkChanProdCons10 90 96 +5.95%
> BenchmarkChanProdCons100 69 70 +2.16%
> BenchmarkChanProdConsWork0 774 777 +0.39%
> BenchmarkChanProdConsWork10 713 719 +0.84%
> BenchmarkChanProdConsWork100 688 691 +0.44%
> BenchmarkChanCreation 181 186 +2.76%
> BenchmarkChanSem 62 64 +3.51%

This could just be noise on this machine. Reverting the removal of the
uint8 field (which I thought was affecting the padding) made no
difference.

https://codereview.appspot.com/9678046/diff/5001/src/pkg/runtime/chan.c
> File src/pkg/runtime/chan.c (right):


https://codereview.appspot.com/9678046/diff/5001/src/pkg/runtime/chan.c#newcode107
> src/pkg/runtime/chan.c:107: runtime·throw("makechan: bad chan size");
> Will the compiled down to a constant ? ie, sizeof(*c) is known at
compile time,
> as is MAXALIGN, so this could compile down to if (0) and be removed ?
I guess I
> should check the output of the compiler.

nm says this check is completely removed at compile time, which is a
nice bonus.


https://codereview.appspot.com/9678046/

[da...@cheney.net]

This CL fails on arm machines, 'fatal error: makechan: bad chan size'

https://codereview.appspot.com/9678046/

[dvy...@google.com]

On 2013/05/24 13:07:16, dfc wrote:
> This CL fails on arm machines, 'fatal error: makechan: bad chan size'

Does ARM has any instructions that require 8-byte alignment (double,
64-bit int operations, vector ops, etc)?

https://codereview.appspot.com/9678046/

[Dave Cheney]

I'm going to say no, but Minux may correct me. If there are 8 byte
wide operations, we probably handle them badly anyway (see 64bit
atomic problems).

[Dave Cheney]

I added back the uint8 elemalign field, and another uint8 _pad2 at the
bottom to fix the alignment issues, but this CL still shows a
performance decrease of 5%, which is very confusing.

[Dmitry Vyukov]

breaks on 386 as well

so we were lying ourselves, the alignment was never enforced

people here say that neon may require 8-byte alignment for vector operations, but we do not generate them

I am going to remove that check for now

[dvy...@google.com]

Removed the check, PTAL

https://codereview.appspot.com/9678046/

[da...@cheney.net]

On 2013/05/24 13:27:43, dvyukov wrote:
> Removed the check, PTAL

Results from linux/arm chromebook

localhost(~/go/src/pkg/runtime) % ~/go/misc/benchcmp {old,new}.txt |
less

benchmark old ns/op new ns/op delta

BenchmarkChanUncontended 557 533 -4.31%
BenchmarkChanContended 546 533 -2.38%
BenchmarkChanSync 1052 1190 +13.12%
BenchmarkChanProdCons0 1076 1082 +0.56%
BenchmarkChanProdCons10 688 683 -0.73%
BenchmarkChanProdCons100 562 547 -2.67%
BenchmarkChanProdConsWork0 2454 2354 -4.07%
BenchmarkChanProdConsWork10 1995 1977 -0.90%
BenchmarkChanProdConsWork100 1844 1818 -1.41%
BenchmarkChanCreation 1167 1104 -5.40%
BenchmarkChanSem 568 528 -7.04%

https://codereview.appspot.com/9678046/

[da...@cheney.net]

Also, could this lead to unaligned loads on arm ?

https://codereview.appspot.com/9678046/

[minux]

On Fri, May 24, 2013 at 9:13 PM, <dvy...@google.com> wrote:

On 2013/05/24 13:07:16, dfc wrote:

This CL fails on arm machines, 'fatal error: makechan: bad chan size'

Does ARM has any instructions that require 8-byte alignment (double,
64-bit int operations, vector ops, etc)?

native 64-bit atomic instructions require 8-byte alignment.

SIMD (NEON) could require up to 32-byte alignment, but our compiler

won't ever generate those instructions (the linker doesn't support them,

so i guess if people use it, they will have to issue they use proper alignment

themselves)

[Dmitry Vyukov]

On Fri, May 24, 2013 at 7:17 PM, minux <minu...@gmail.com> wrote:

On Fri, May 24, 2013 at 9:13 PM, <dvy...@google.com> wrote:

On 2013/05/24 13:07:16, dfc wrote:

This CL fails on arm machines, 'fatal error: makechan: bad chan size'

Does ARM has any instructions that require 8-byte alignment (double,
64-bit int operations, vector ops, etc)?

native 64-bit atomic instructions require 8-byte alignment.

We do only memcpy on chan buffer, so this should not break.

[da...@cheney.net]

On 2013/05/24 13:56:59, dfc wrote:
> Also, could this lead to unaligned loads on arm ?

Tested on arm5 without issue.

https://codereview.appspot.com/9678046/

[r...@golang.org]

Please fix the typo in the description: s/filed/field/


https://codereview.appspot.com/9678046/

[dvy...@google.com]

On 2013/05/25 02:53:17, r wrote:
> Please fix the typo in the description: s/filed/field/

Done

https://codereview.appspot.com/9678046/