Issue 9678046: code review 9678046: runtime: remove unused filed from Hchan (Closed)
Descriptionruntime: remove unused field from Hchan
Remove alignment logic as well, it's not respected by chanbuf() anyway.
Patch Set 1 #Patch Set 2 : diff -r d6e06d0f3c29 https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 3 : diff -r d6e06d0f3c29 https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 1
Patch Set 4 : diff -r d6e06d0f3c29 https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 1
Patch Set 5 : diff -r 60e04bb0d8b9 https://dvyukov%40google.com@code.google.com/p/go/ #
Total comments: 2
Patch Set 6 : diff -r b1217a6ca24c https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 7 : diff -r b1217a6ca24c https://dvyukov%40google.com@code.google.com/p/go/ #Patch Set 8 : diff -r b1217a6ca24c https://dvyukov%40google.com@code.google.com/p/go/ #MessagesTotal messages: 24
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://dvyukov%40google.com@code.google.com/p/go/
Sign in to reply to this message.
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#newco...
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.
Sign in to reply to this message.
> 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#newco...
> 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.
Sign in to reply to this message.
This CL fails on arm machines, 'fatal error: makechan: bad chan size'
Sign in to reply to this message.
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)?
Sign in to reply to this message.
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). On Fri, May 24, 2013 at 11:13 PM, <dvyukov@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)? > > https://codereview.appspot.com/9678046/
Sign in to reply to this message.
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. On Fri, May 24, 2013 at 11:15 PM, Dave Cheney <dave@cheney.net> wrote: > 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). > > On Fri, May 24, 2013 at 11:13 PM, <dvyukov@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)? >> >> https://codereview.appspot.com/9678046/
Sign in to reply to this message.
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 On Fri, May 24, 2013 at 5:16 PM, Dave Cheney <dave@cheney.net> wrote: > 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. > > On Fri, May 24, 2013 at 11:15 PM, Dave Cheney <dave@cheney.net> wrote: > > 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). > > > > On Fri, May 24, 2013 at 11:13 PM, <dvyukov@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)? > >> > >> https://codereview.appspot.com/9678046/ >
Sign in to reply to this message.
Removed the check, PTAL
Sign in to reply to this message.
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%
Sign in to reply to this message.
Also, could this lead to unaligned loads on arm ?
Sign in to reply to this message.
On Fri, May 24, 2013 at 9:13 PM, <dvyukov@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)
Sign in to reply to this message.
On Fri, May 24, 2013 at 7:17 PM, minux <minux.ma@gmail.com> wrote: > > On Fri, May 24, 2013 at 9:13 PM, <dvyukov@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. 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) >
Sign in to reply to this message.
On 2013/05/24 13:56:59, dfc wrote: > Also, could this lead to unaligned loads on arm ? Tested on arm5 without issue.
Sign in to reply to this message.
Please fix the typo in the description: s/filed/field/
Sign in to reply to this message.
On 2013/05/25 02:53:17, r wrote: > Please fix the typo in the description: s/filed/field/ Done
Sign in to reply to this message.
https://codereview.appspot.com/9678046/diff/14001/src/pkg/runtime/chan.c File src/pkg/runtime/chan.c (left): https://codereview.appspot.com/9678046/diff/14001/src/pkg/runtime/chan.c#oldc... src/pkg/runtime/chan.c:108: // calculate rounded size of Hchan OK, I can see why this existing code doesn't work. But as far as I can see, something like this is conceptually required. Values are moved in and out using the type's copy algorithm. That algorithm assumes that the type is correctly aligned--e.g., look at runtime·slicecopy in alg.c. So the channel code really ought to be ensuring that the type values are correctly aligned. The type size is such that this should work correctly--provided the channel buffer is aligned correctly. So it seems to me that the code here needs to compute sizeof(*c) rounded up to elem->align. And that value needs to be stored in Hchan. And chanbuf needs to change (byte*)(c+1) to (byte*)c + c->chansize. We know that chansize will be less than 256, so we can replace the existing elemalign field with chansize. I expect that the current code does work fine, but it only works fine by coincidence. I think we should strive to ensure that it works fine always.
Sign in to reply to this message.
On 2013/05/31 18:48:58, iant wrote: > https://codereview.appspot.com/9678046/diff/14001/src/pkg/runtime/chan.c > File src/pkg/runtime/chan.c (left): > > https://codereview.appspot.com/9678046/diff/14001/src/pkg/runtime/chan.c#oldc... > src/pkg/runtime/chan.c:108: // calculate rounded size of Hchan > OK, I can see why this existing code doesn't work. But as far as I can see, > something like this is conceptually required. Values are moved in and out using > the type's copy algorithm. That algorithm assumes that the type is correctly > aligned--e.g., look at runtime·slicecopy in alg.c. So the channel code really > ought to be ensuring that the type values are correctly aligned. The type size > is such that this should work correctly--provided the channel buffer is aligned > correctly. > > So it seems to me that the code here needs to compute sizeof(*c) rounded up to > elem->align. And that value needs to be stored in Hchan. And chanbuf needs to > change (byte*)(c+1) to (byte*)c + c->chansize. We know that chansize will be > less than 256, so we can replace the existing elemalign field with chansize. > > I expect that the current code does work fine, but it only works fine by > coincidence. I think we should strive to ensure that it works fine always. Let's wait for the decision on https://codereview.appspot.com/9730043/
Sign in to reply to this message.
On 2013/05/31 18:48:58, iant wrote: > https://codereview.appspot.com/9678046/diff/14001/src/pkg/runtime/chan.c > File src/pkg/runtime/chan.c (left): > > https://codereview.appspot.com/9678046/diff/14001/src/pkg/runtime/chan.c#oldc... > src/pkg/runtime/chan.c:108: // calculate rounded size of Hchan > OK, I can see why this existing code doesn't work. But as far as I can see, > something like this is conceptually required. Values are moved in and out using > the type's copy algorithm. That algorithm assumes that the type is correctly > aligned--e.g., look at runtime·slicecopy in alg.c. So the channel code really > ought to be ensuring that the type values are correctly aligned. The type size > is such that this should work correctly--provided the channel buffer is aligned > correctly. > > So it seems to me that the code here needs to compute sizeof(*c) rounded up to > elem->align. And that value needs to be stored in Hchan. And chanbuf needs to > change (byte*)(c+1) to (byte*)c + c->chansize. We know that chansize will be > less than 256, so we can replace the existing elemalign field with chansize. > > I expect that the current code does work fine, but it only works fine by > coincidence. I think we should strive to ensure that it works fine always. PTAL I have not implemented the full-fledged alignment, but it's better and simpler than now, and will break loudly if anything goes wrong.
Sign in to reply to this message.
https://codereview.appspot.com/9678046/diff/29001/src/pkg/runtime/chan.c File src/pkg/runtime/chan.c (right): https://codereview.appspot.com/9678046/diff/29001/src/pkg/runtime/chan.c#newc... src/pkg/runtime/chan.c:41: uint16 pad; // ensures proper alignment of the buffer I'm sorry to nit pick, but there is no field called buffer in this struct, could I ask you to reword this please. https://codereview.appspot.com/9678046/diff/29001/src/pkg/runtime/chan.c#newc... src/pkg/runtime/chan.c:103: if((sizeof(*c)%MAXALIGN) != 0 || elem->align > MAXALIGN) it is sad that this is no longer something the compiler can optimise away, I liked your previous version. I understand if it isn't possible to implement this anymore.
Sign in to reply to this message.
On 2013/06/06 12:55:52, dfc wrote: > https://codereview.appspot.com/9678046/diff/29001/src/pkg/runtime/chan.c > File src/pkg/runtime/chan.c (right): > > https://codereview.appspot.com/9678046/diff/29001/src/pkg/runtime/chan.c#newc... > src/pkg/runtime/chan.c:41: uint16 pad; // ensures proper alignment of the > buffer > I'm sorry to nit pick, but there is no field called buffer in this struct, could > I ask you to reword this please. Done. PTAL > https://codereview.appspot.com/9678046/diff/29001/src/pkg/runtime/chan.c#newc... > src/pkg/runtime/chan.c:103: if((sizeof(*c)%MAXALIGN) != 0 || elem->align > > MAXALIGN) > it is sad that this is no longer something the compiler can optimise away, I > liked your previous version. I understand if it isn't possible to implement this > anymore. It will be awesome if it becomes a hotspot, then I will happily implement the check in the compiler :)
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=623df01aa32b *** runtime: remove unused field from Hchan Remove alignment logic as well, it's not respected by chanbuf() anyway. R=golang-dev, dave, minux.ma, r, iant, rsc CC=golang-dev https://codereview.appspot.com/9678046
Sign in to reply to this message.
|
