code review 9730043: runtime: allocate chan in 2 allocations (Hchan and user...

runtime: allocate chan in 2 allocations (Hchan and user buffer)
Several reasons for this change:
1. Simplify GC code.
2. Allows lazy chan buffer allocations (i.e. chan(int, 1e7)).
3. Allow bitmask-based GC (otherwise it's unclear how to handle chans,
because it's not a struct and not an array).
Benchmarks results are noisy.
chan(int) allocation is faster because it does not need to call setype(),
it just sets FlagNoPointers during mallocgc().

benchmark                             old ns/op    new ns/op    delta
BenchmarkChanUncontended                     55           56   +0.90%
BenchmarkChanUncontended-2                   29           28   -1.37%
BenchmarkChanUncontended-4                   19           19   +1.02%
BenchmarkChanUncontended-8                   14           12  -18.12%
BenchmarkChanContended                       60           56   -6.66%
BenchmarkChanContended-2                    181          137  -24.31%
BenchmarkChanContended-4                    359          338   -5.85%
BenchmarkChanContended-8                    344          377   +9.59%
BenchmarkChanSync                           124          127   +2.42%
BenchmarkChanSync-2                         443          481   +8.58%
BenchmarkChanSync-4                         417          479  +14.87%
BenchmarkChanSync-8                         391          381   -2.56%
BenchmarkChanProdCons0                      128          133   +3.91%
BenchmarkChanProdCons0-2                    278          301   +8.27%
BenchmarkChanProdCons0-4                    709          704   -0.71%
BenchmarkChanProdCons0-8                   1007         1058   +5.06%
BenchmarkChanProdCons10                      82           78   -4.50%
BenchmarkChanProdCons10-2                   327          299   -8.56%
BenchmarkChanProdCons10-4                   697          689   -1.15%
BenchmarkChanProdCons10-8                   755          779   +3.18%
BenchmarkChanProdCons100                     62           62   -0.16%
BenchmarkChanProdCons100-2                  201          169  -15.92%
BenchmarkChanProdCons100-4                  395          414   +4.81%
BenchmarkChanProdCons100-8                  395          447  +13.16%
BenchmarkChanProdConsWork0                  653          649   -0.61%
BenchmarkChanProdConsWork0-2                391          410   +4.86%
BenchmarkChanProdConsWork0-4                686          656   -4.37%
BenchmarkChanProdConsWork0-8               1029         1105   +7.39%
BenchmarkChanProdConsWork10                 603          601   -0.33%
BenchmarkChanProdConsWork10-2               523          441  -15.68%
BenchmarkChanProdConsWork10-4               879          962   +9.44%
BenchmarkChanProdConsWork10-8              1142         1332  +16.64%
BenchmarkChanProdConsWork100                584          582   -0.34%
BenchmarkChanProdConsWork100-2              501          495   -1.20%
BenchmarkChanProdConsWork100-4              926          959   +3.56%
BenchmarkChanProdConsWork100-8             1013         1140  +12.54%
BenchmarkChanCreation                        97           77  -20.53%
BenchmarkChanCreation-2                     114           91  -19.74%
BenchmarkChanCreation-4                      84           74  -12.38%
BenchmarkChanCreation-8                      90           73  -18.89%
BenchmarkChanCreationBuffered               128          143  +11.72%
BenchmarkChanCreationBuffered-2             219          183  -16.44%
BenchmarkChanCreationBuffered-4             159          123  -22.64%
BenchmarkChanCreationBuffered-8             199          158  -20.60%
BenchmarkChanCreationBufferedPtr            139          158  +13.67%
BenchmarkChanCreationBufferedPtr-2          234          225   -3.85%
BenchmarkChanCreationBufferedPtr-4          167          191  +14.37%
BenchmarkChanCreationBufferedPtr-8          191          165  -13.61%
BenchmarkChanSem                             55           53   -4.67%
BenchmarkChanSem-2                           55           52   -4.51%
BenchmarkChanSem-4                           55           52   -4.35%
BenchmarkChanSem-8                           55           52   -4.35%

[dvyukov, 2013-05-24 16:40:29]

Hello golang-dev@googlegroups.com,

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

[iant, 2013-05-31 22:02:49]

This is a simplification but I'm not sure whether it is a good idea or not.  I'd
be happy to hear other opinions.

https://codereview.appspot.com/9730043/diff/15001/src/cmd/gc/reflect.c
File src/cmd/gc/reflect.c (right):

https://codereview.appspot.com/9730043/diff/15001/src/cmd/gc/reflect.c#newcod...
src/cmd/gc/reflect.c:1104: ot = duintptr(s, ot, GC_APTR);
This doesn't seem right.  We don't want to treat the Hchan structure as
containing arbitrary data.  We want to treat it as containing no pointers,
except for the pointer to the channel buffer.  That seems to be what happens
today.

That is, this change will make GC less precise here.

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

https://codereview.appspot.com/9730043/diff/15001/src/pkg/runtime/chan.c#newc...
src/pkg/runtime/chan.c:111: n = sizeof(*c);
This loop is now completely useless (I know you also have another CL in flight
to remove these lines).

https://codereview.appspot.com/9730043/diff/15001/src/pkg/runtime/chan.c#newc...
src/pkg/runtime/chan.c:115: // allocate memory in one call
Remove this comment.

https://codereview.appspot.com/9730043/diff/15001/src/pkg/runtime/chan.c#newc...
src/pkg/runtime/chan.c:121: if(hint) {
s/hint/hint != 0/

https://codereview.appspot.com/9730043/diff/15001/src/pkg/runtime/chan.c#newc...
src/pkg/runtime/chan.c:122: c->elem = runtime·mallocgc(hint*elem->size,
elem->kind&KindNoPointers ? FlagNoPointers : 0, 1, 1);
Might as well just call runtime·cnewarray here.  Then you can remove the hint >
MaxMem / elem->size test above.

[dave_cheney.net, 2013-05-31 23:06:26]

I'm not sure this is a good idea, we've had a lot of permutations for
allocating channels, but I am prepared to be persuaded if this is
important for the GC.

On Sat, Jun 1, 2013 at 8:02 AM,  <iant@golang.org> wrote:
> This is a simplification but I'm not sure whether it is a good idea or
> not.  I'd be happy to hear other opinions.
>
>
>
> https://codereview.appspot.com/9730043/diff/15001/src/cmd/gc/reflect.c
> File src/cmd/gc/reflect.c (right):
>
>
https://codereview.appspot.com/9730043/diff/15001/src/cmd/gc/reflect.c#newcod...
> src/cmd/gc/reflect.c:1104: ot = duintptr(s, ot, GC_APTR);
> This doesn't seem right.  We don't want to treat the Hchan structure as
> containing arbitrary data.  We want to treat it as containing no
> pointers, except for the pointer to the channel buffer.  That seems to
> be what happens today.
>
> That is, this change will make GC less precise here.
>
> https://codereview.appspot.com/9730043/diff/15001/src/pkg/runtime/chan.c
> File src/pkg/runtime/chan.c (right):
>
>
https://codereview.appspot.com/9730043/diff/15001/src/pkg/runtime/chan.c#newc...
> src/pkg/runtime/chan.c:111: n = sizeof(*c);
> This loop is now completely useless (I know you also have another CL in
> flight to remove these lines).
>
>
https://codereview.appspot.com/9730043/diff/15001/src/pkg/runtime/chan.c#newc...
> src/pkg/runtime/chan.c:115: // allocate memory in one call
> Remove this comment.
>
>
https://codereview.appspot.com/9730043/diff/15001/src/pkg/runtime/chan.c#newc...
> src/pkg/runtime/chan.c:121: if(hint) {
> s/hint/hint != 0/
>
>
https://codereview.appspot.com/9730043/diff/15001/src/pkg/runtime/chan.c#newc...
> src/pkg/runtime/chan.c:122: c->elem = runtime·mallocgc(hint*elem->size,
> elem->kind&KindNoPointers ? FlagNoPointers : 0, 1, 1);
> Might as well just call runtime·cnewarray here.  Then you can remove the
> hint > MaxMem / elem->size test above.
>
> https://codereview.appspot.com/9730043/
>
> --
>
> ---You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[dvyukov, 2013-06-01 20:11:01]

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

https://codereview.appspot.com/9730043/diff/15001/src/pkg/runtime/chan.c#newc...
src/pkg/runtime/chan.c:111: n = sizeof(*c);
On 2013/05/31 22:02:49, iant wrote:
> This loop is now completely useless (I know you also have another CL in flight
> to remove these lines).

Done.

https://codereview.appspot.com/9730043/diff/15001/src/pkg/runtime/chan.c#newc...
src/pkg/runtime/chan.c:115: // allocate memory in one call
On 2013/05/31 22:02:49, iant wrote:
> Remove this comment.

Done.

https://codereview.appspot.com/9730043/diff/15001/src/pkg/runtime/chan.c#newc...
src/pkg/runtime/chan.c:121: if(hint) {
On 2013/05/31 22:02:49, iant wrote:
> s/hint/hint != 0/

Done.

https://codereview.appspot.com/9730043/diff/15001/src/pkg/runtime/chan.c#newc...
src/pkg/runtime/chan.c:122: c->elem = runtime·mallocgc(hint*elem->size,
elem->kind&KindNoPointers ? FlagNoPointers : 0, 1, 1);
On 2013/05/31 22:02:49, iant wrote:
> Might as well just call runtime·cnewarray here.  Then you can remove the hint
>
> MaxMem / elem->size test above.

Done.

[dvyukov, 2013-06-01 20:12:18]

On 2013/05/31 23:06:26, dfc wrote:
> I'm not sure this is a good idea, we've had a lot of permutations for
> allocating channels, but I am prepared to be persuaded if this is
> important for the GC.

I will write an email to golang-dev.

[dvyukov, 2013-06-01 20:15:28]

https://codereview.appspot.com/9730043/diff/15001/src/cmd/gc/reflect.c
File src/cmd/gc/reflect.c (right):

https://codereview.appspot.com/9730043/diff/15001/src/cmd/gc/reflect.c#newcod...
src/cmd/gc/reflect.c:1104: ot = duintptr(s, ot, GC_APTR);
On 2013/05/31 22:02:49, iant wrote:
> This doesn't seem right.  We don't want to treat the Hchan structure as
> containing arbitrary data.  We want to treat it as containing no pointers,
> except for the pointer to the channel buffer.  That seems to be what happens
> today.
> 
> That is, this change will make GC less precise here.

Yeah, type info needs to be restored for chans.
I think I need to do the same thing as we do for runtime.M, i.e. generate Go
struct definition and allocate Hchan using that type.

[rsc, 2013-06-03 20:14:15]

NOT LGTM

It's fine to split the channel into two allocated blocks to make garbage
collection easier. However, please make it preserve the precise collection, in
this CL.

[dvyukov, 2013-07-18 14:26:52]

On 2013/06/03 20:14:15, rsc wrote:
> NOT LGTM
> 
> It's fine to split the channel into two allocated blocks to make garbage
> collection easier.


OK, I think it's better to do this when we are ready to really simplify GC based
on this (bitmask-based GC). And this is definitely not for Go1.2.

[dvyukov, 2013-07-18 14:27:24]

*** Abandoned ***