code review 129090043: cmd/gc, runtime: refactor interface inlining decision i...

cmd/gc, runtime: refactor interface inlining decision into compiler

We need to change the interface value representation for
concurrent garbage collection, so that there is no ambiguity
about whether the data word holds a pointer or scalar.

This CL does NOT make any representation changes.

Instead, it removes representation assumptions from
various pieces of code throughout the tree.
The isdirectiface function in cmd/gc/subr.c is now
the only place that decides that policy.
The policy propagates out from there in the reflect
metadata, as a new flag in the internal kind value.

A follow-up CL will change the representation by
changing the isdirectiface function. If that CL causes
problems, it will be easy to roll back.

Update issue 8405.

[rsc, 2014-08-13 15:08:45]

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

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

[iant, 2014-08-13 15:54:16]

LGTM

https://codereview.appspot.com/129090043/diff/20001/src/cmd/pack/pack_test.go
File src/cmd/pack/pack_test.go (right):

https://codereview.appspot.com/129090043/diff/20001/src/cmd/pack/pack_test.go...
src/cmd/pack/pack_test.go:232: //defer os.RemoveAll(dir)
This change is unrelated to this CL.

https://codereview.appspot.com/129090043/diff/20001/src/pkg/database/sql/conv...
File src/pkg/database/sql/convert_test.go (right):

https://codereview.appspot.com/129090043/diff/20001/src/pkg/database/sql/conv...
src/pkg/database/sql/convert_test.go:286: var tests = []struct {
This seems like a worthwhile change but I don't understand the connection to
this CL.

https://codereview.appspot.com/129090043/diff/20001/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

https://codereview.appspot.com/129090043/diff/20001/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:614: // value does not fit in word.
s/does not fit in word/can not be stored in interface/

[iant, 2014-08-13 17:47:58]

If you turn on IfacePointerOnly then I think the scalar field of reflect.Value
is no longer used.

[rsc, 2014-08-13 18:34:07]

https://codereview.appspot.com/129090043/diff/20001/src/cmd/pack/pack_test.go
File src/cmd/pack/pack_test.go (right):

https://codereview.appspot.com/129090043/diff/20001/src/cmd/pack/pack_test.go...
src/cmd/pack/pack_test.go:232: //defer os.RemoveAll(dir)
On 2014/08/13 15:54:15, iant wrote:
> This change is unrelated to this CL.

Yes, this snuck in. Removed.

https://codereview.appspot.com/129090043/diff/20001/src/pkg/database/sql/conv...
File src/pkg/database/sql/convert_test.go (right):

https://codereview.appspot.com/129090043/diff/20001/src/pkg/database/sql/conv...
src/pkg/database/sql/convert_test.go:286: var tests = []struct {
On 2014/08/13 15:54:15, iant wrote:
> This seems like a worthwhile change but I don't understand the connection to
> this CL.

This is necessary (along with the change below) because the conversion of values
like uint8(1) to interface{} now allocate. The test used to have an assignment
like that when preparing the arguments to 'test'. That seems unrelated to what
is really being tested (that RawBytes doesn't allocate), so I hoisted it up.

[rsc, 2014-08-19 01:12:43]

On 2014/08/13 17:47:58, iant wrote:
> If you turn on IfacePointerOnly then I think the scalar field of reflect.Value
> is no longer used.

Yes, there are many simplifications at that point. I'd like to wait on those
until after 1.4 though, just in case we need to roll this back.

[rsc, 2014-08-19 01:12:50]

https://codereview.appspot.com/129090043/diff/20001/src/pkg/reflect/value.go
File src/pkg/reflect/value.go (right):

https://codereview.appspot.com/129090043/diff/20001/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:614: // value does not fit in word.
On 2014/08/13 15:54:16, iant wrote:
> s/does not fit in word/can not be stored in interface/

Done.

[rsc, 2014-08-19 01:13:16]

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

cmd/gc, runtime: refactor interface inlining decision into compiler

We need to change the interface value representation for
concurrent garbage collection, so that there is no ambiguity
about whether the data word holds a pointer or scalar.

This CL does NOT make any representation changes.

Instead, it removes representation assumptions from
various pieces of code throughout the tree.
The isdirectiface function in cmd/gc/subr.c is now
the only place that decides that policy.
The policy propagates out from there in the reflect
metadata, as a new flag in the internal kind value.

A follow-up CL will change the representation by
changing the isdirectiface function. If that CL causes
problems, it will be easy to roll back.

Update issue 8405.

LGTM=iant
R=golang-codereviews, iant
CC=golang-codereviews, r
https://codereview.appspot.com/129090043

[gobot, 2014-08-19 02:27:52]

This CL appears to have broken the windows-386 builder.
See http://build.golang.org/log/bce4cb056acac994864e8b6c40b31574945e2168