code review 7716048: reflect: add garbage collection info in ChanOf, MapOf, ...

reflect: add garbage collection info in ChanOf, MapOf, PtrTo, SliceOf

ArrayOf will remain unexported (and therefore unused) for Go 1.1.

Fixes issue 4375.

[rsc, 2013-03-15 17:51:59]

Hello atom (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2013-03-15 17:52:35]

Please take a careful look at this. It is probably all wrong. I would also
appreciate suggestions on what might work well for tests. Obviously what's
there is insufficient.

Thanks.

[atom, 2013-03-15 18:46:36]

On 2013/03/15 17:52:35, rsc wrote:
> Please take a careful look at this. It is probably all wrong. I would also
> appreciate suggestions on what might work well for tests. Obviously what's
> there is insufficient.
> 
> Thanks.

I believe GC will need to scan eface->type:
https://codereview.appspot.com/7744047/

[r, 2013-03-15 18:54:26]

https://codereview.appspot.com/7716048/diff/2001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):

https://codereview.appspot.com/7716048/diff/2001/src/pkg/reflect/all_test.go#...
src/pkg/reflect/all_test.go:1940: //XXX
???

https://codereview.appspot.com/7716048/diff/2001/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):

https://codereview.appspot.com/7716048/diff/2001/src/pkg/reflect/type.go#newc...
src/pkg/reflect/type.go:1002: // garbage collection program for pointer to
memory without pointers.
"program"?

[atom, 2013-03-15 19:09:14]

https://codereview.appspot.com/7716048/diff/2001/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):

https://codereview.appspot.com/7716048/diff/2001/src/pkg/reflect/type.go#newc...
src/pkg/reflect/type.go:1002: // garbage collection program for pointer to
memory without pointers.
On 2013/03/15 18:54:26, r wrote:
> "program"?

It is a program, kind of. The function scanblock() works like an interpreter of
an abstract instruction set.

If it is possible to implement scanblock() and precise GC without the
interpreter pattern - which seems impossible to me - I would like to know how.

[rsc, 2013-03-15 19:23:25]

Re: XXX, the tests are not done yet. Part of the reason to mail it out was
to get some advice on testing from Atom.

Russ

[atom, 2013-03-15 19:26:50]

On 2013/03/15 19:09:14, atom wrote:
> https://codereview.appspot.com/7716048/diff/2001/src/pkg/reflect/type.go
> File src/pkg/reflect/type.go (right):
> 
>
https://codereview.appspot.com/7716048/diff/2001/src/pkg/reflect/type.go#newc...
> src/pkg/reflect/type.go:1002: // garbage collection program for pointer to
> memory without pointers.
> On 2013/03/15 18:54:26, r wrote:
> > "program"?
> 
> It is a program, kind of. The function scanblock() works like an interpreter
of
> an abstract instruction set.
> 
> If it is possible to implement scanblock() and precise GC without the
> interpreter pattern - which seems impossible to me - I would like to know how.

An option is carving bits (pointer/not-pointer) into objects immediately after
they are allocated. I believe that having more accurate knowledge provided by
the "programs" may turn out to be more useful in the future than bit carving.

[rsc, 2013-03-15 19:36:20]

The details of the garbage collector aren't up for redesign today. I'll
change program to bytecode program.

[atom, 2013-03-16 07:36:36]

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

https://codereview.appspot.com/7716048/diff/2001/src/cmd/gc/reflect.c#newcode...
src/cmd/gc/reflect.c:1087: // NOTE: If the TCHAN representation changes to
include
https://codereview.appspot.com/7473044/

[atom, 2013-03-17 17:27:45]

On 2013/03/15 17:52:35, rsc wrote:
> I would also appreciate suggestions on what might work well for tests.

CL 7879043 contains the following cases:

    case MTypes_Single:
    case MTypes_Words:
    case MTypes_Bytes:

The addroot() functions will be called only if Go code allocates a value V of
some type T, and T,V are both created via package reflect at runtime.

The source code for case MTypes_Single is new. Source code for the other cases
is just an optimization.

[rsc, 2013-03-18 14:48:32]

We don't necessarily have to treat the type info found in various data
structures as roots. Package reflect must arrange to return the same type
info from identical calls to the constructors, so it necessarily has a
pointer to the type info saved away itself. I think it would probably be
simpler and faster to rely on finding the references via that cache.

Russ

[atom, 2013-03-18 16:15:23]

https://codereview.appspot.com/7716048/diff/2001/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):

https://codereview.appspot.com/7716048/diff/2001/src/pkg/reflect/type.go#newc...
src/pkg/reflect/type.go:1350: var lookupCache struct {
Is it possible for reflection to create type []T at runtime, while []T is also a
compiler generated type? If it was possible then there would be two distinct
instances of []T and Go code (type assertions, type switches) could fail.

[rsc, 2013-03-18 20:39:15]

On Mon, Mar 18, 2013 at 12:15 PM, <0xE2.0x9A.0x9B@gmail.com> wrote:

>
> https://codereview.appspot.**com/7716048/diff/2001/src/pkg/**
>
reflect/type.go<https://codereview.appspot.com/7716048/diff/2001/src/pkg/reflect/type.go>
> File src/pkg/reflect/type.go (right):
>
> https://codereview.appspot.**com/7716048/diff/2001/src/pkg/**
>
reflect/type.go#newcode1350<https://codereview.appspot.com/7716048/diff/2001/src/pkg/reflect/type.go#newcode1350>
> src/pkg/reflect/type.go:1350: var lookupCache struct {
> Is it possible for reflection to create type []T at runtime, while []T
> is also a compiler generated type? If it was possible then there would
> be two distinct instances of []T and Go code (type assertions, type
> switches) could fail.
>

No, it's not possible, for exactly the reason you point out. This takes
some care, but it's true in both the gc and the gccgo toolchains.

Russ

[rsc, 2013-03-24 18:11:06]

PTAL

I have written tests and confirmed that they work by breaking each of the
gc info and running the corresponding test.

[r, 2013-03-25 16:51:01]

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

https://codereview.appspot.com/7716048/diff/21001/src/cmd/gc/reflect.c#newcod...
src/cmd/gc/reflect.c:1160: // at least once ArrayOf is implemented.
isn't it implemented?

https://codereview.appspot.com/7716048/diff/21001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):

https://codereview.appspot.com/7716048/diff/21001/src/pkg/reflect/all_test.go...
src/pkg/reflect/all_test.go:2203: func TestPtrToGC(t *testing.T) {
if you need the commented-out line to test this, ...?

https://codereview.appspot.com/7716048/diff/21001/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):

https://codereview.appspot.com/7716048/diff/21001/src/pkg/reflect/type.go#new...
src/pkg/reflect/type.go:1096: // INCORRECT. Uncomment to test TestPtrToGC.
can you please explain this better?
maybe do it once outside the functions and have all 4 appearances refer to that

[iant, 2013-03-25 17:35:06]

LGTM

[rsc, 2013-03-25 19:59:21]

https://codereview.appspot.com/7716048/diff/21001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):

https://codereview.appspot.com/7716048/diff/21001/src/pkg/reflect/all_test.go...
src/pkg/reflect/all_test.go:2203: func TestPtrToGC(t *testing.T) {
On 2013/03/25 16:51:01, r wrote:
> if you need the commented-out line to test this, ...?

By "test the test" I mean confirm that it fails when the implementation is
broken.
I will clean up the comment.

[atom, 2013-03-25 21:23:10]

On 2013/03/24 18:11:06, rsc wrote:
> PTAL
> 
> I have written tests and confirmed that they work by breaking each of the
> gc info and running the corresponding test.

I can confirm that enabling CollectStats in mgc0.c leads to expected counts for
all "Test.*GC.*" functions.

[rsc, 2013-03-25 22:13:16]

PTAL

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

https://codereview.appspot.com/7716048/diff/21001/src/cmd/gc/reflect.c#newcod...
src/cmd/gc/reflect.c:1160: // at least once ArrayOf is implemented.
On 2013/03/25 16:51:01, r wrote:
> isn't it implemented?

It's mostly implemented, but it's unexported (because it's not done).
It won't be in Go 1.1.

[r, 2013-03-25 22:15:32]

LGTM

[r, 2013-03-26 18:50:31]

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

reflect: add garbage collection info in ChanOf, MapOf, PtrTo, SliceOf

ArrayOf will remain unexported (and therefore unused) for Go 1.1.

Fixes issue 4375.

R=0xe2.0x9a.0x9b, r, iant
CC=golang-dev
https://codereview.appspot.com/7716048

Committer: Rob Pike <r@golang.org>

[remyoudompheng, 2013-07-21 19:52:00]

R=close