Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(94)

Issue 6572043: code review 6572043: reflect: add ArrayOf, ChanOf, MapOf, SliceOf (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 6 months ago by rsc
Modified:
11 years, 4 months ago
Reviewers:
CC:
r, remyoudompheng, aam, iant, golang-dev
Visibility:
Public.

Description

reflect: add ArrayOf, ChanOf, MapOf, SliceOf In order to add these, we need to be able to find references to such types that already exist in the binary. To do that, introduce a new linker section holding a list of the types corresponding to arrays, chans, maps, and slices. To offset the storage cost of this list, and to simplify the code, remove the interface{} header from the representation of a runtime type. It was used in early versions of the code but was made obsolete by the kind field: a switch on kind is more efficient than a type switch. In the godoc binary, removing the interface{} header cuts two words from each of about 10,000 types. Adding back the list of pointers to array, chan, map, and slice types reintroduces one word for each of about 500 types. On a 64-bit machine, then, this CL *removes* a net 156 kB of read-only data from the binary. This CL does not include the needed support for precise garbage collection. I have created issue 4375 to track that. This CL also does not set the 'algorithm' - specifically the equality and copy functions - for a new array correctly, so I have unexported ArrayOf for now. That is also part of issue 4375. Fixes issue 2339.

Patch Set 1 #

Patch Set 2 : diff -r 0a3866d6cc6b https://code.google.com/p/go/ #

Patch Set 3 : diff -r 7252ea907502 https://code.google.com/p/go/ #

Total comments: 20

Patch Set 4 : diff -r 7252ea907502 https://code.google.com/p/go/ #

Patch Set 5 : diff -r 7252ea907502 https://code.google.com/p/go/ #

Patch Set 6 : diff -r 7252ea907502 https://code.google.com/p/go/ #

Total comments: 24

Patch Set 7 : diff -r ab868a8f3128 https://go.googlecode.com/hg/ #

Total comments: 7

Patch Set 8 : diff -r 6e9d872ffc66 https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+670 lines, -298 lines) Patch
M src/cmd/gc/go.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M src/cmd/gc/lex.c View 1 2 1 chunk +4 lines, -0 lines 0 comments Download
M src/cmd/gc/reflect.c View 1 2 6 chunks +42 lines, -22 lines 0 comments Download
M src/cmd/ld/data.c View 1 2 5 chunks +17 lines, -3 lines 0 comments Download
M src/cmd/ld/decodesym.c View 1 2 1 chunk +3 lines, -3 lines 0 comments Download
M src/cmd/ld/dwarf.c View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M src/cmd/ld/go.c View 1 2 3 4 5 6 2 chunks +7 lines, -1 line 0 comments Download
M src/cmd/ld/lib.h View 1 2 1 chunk +1 line, -0 lines 0 comments Download
M src/cmd/ld/symtab.c View 1 2 3 4 5 6 2 chunks +6 lines, -0 lines 0 comments Download
M src/pkg/reflect/all_test.go View 1 2 3 4 5 6 1 chunk +82 lines, -0 lines 0 comments Download
M src/pkg/reflect/export_test.go View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M src/pkg/reflect/makefunc.go View 1 2 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/reflect/type.go View 1 2 3 4 5 6 7 32 chunks +407 lines, -152 lines 0 comments Download
M src/pkg/reflect/value.go View 1 2 3 4 46 chunks +66 lines, -66 lines 0 comments Download
M src/pkg/runtime/iface.c View 1 2 3 4 5 6 3 chunks +13 lines, -16 lines 0 comments Download
M src/pkg/runtime/runtime-gdb.py View 1 2 3 4 3 chunks +5 lines, -11 lines 0 comments Download
M src/pkg/runtime/type.h View 1 2 3 chunks +3 lines, -13 lines 0 comments Download
M src/pkg/runtime/type.go View 1 2 3 4 3 chunks +6 lines, -6 lines 0 comments Download
M src/pkg/runtime/typekind.h View 1 2 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 15
rsc
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
11 years, 4 months ago (2012-11-07 21:34:49 UTC) #1
r
haven't looked at the real code yet, but after a skim of the declarations i ...
11 years, 4 months ago (2012-11-07 21:41:51 UTC) #2
rsc
Hello golang-dev@googlegroups.com, r@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 4 months ago (2012-11-07 21:42:33 UTC) #3
rsc
Sorry, the PTAL is just because hg made me run gofmt and I thought the ...
11 years, 4 months ago (2012-11-07 21:43:10 UTC) #4
remyoudompheng
http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go File src/pkg/reflect/all_test.go (right): http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#newcode2717 src/pkg/reflect/all_test.go:2717: } I would like to see something like: vi ...
11 years, 4 months ago (2012-11-07 21:54:15 UTC) #5
rsc
https://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go File src/pkg/reflect/all_test.go (right): https://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#newcode2746 src/pkg/reflect/all_test.go:2746: t.Errorf("using constructed chan: have %q, %q, want %q, %q", ...
11 years, 4 months ago (2012-11-07 21:54:21 UTC) #6
rsc
https://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go File src/pkg/reflect/all_test.go (right): https://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#newcode2717 src/pkg/reflect/all_test.go:2717: } On 2012/11/07 21:54:15, remyoudompheng wrote: > I would ...
11 years, 4 months ago (2012-11-07 22:06:23 UTC) #7
r
http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go File src/pkg/reflect/type.go (right): http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1017 src/pkg/reflect/type.go:1017: // of a *unsafe.Pointer. s/a/an/ ? http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newcode1249 src/pkg/reflect/type.go:1249: // ...
11 years, 4 months ago (2012-11-07 22:38:47 UTC) #8
aam
> where does this number come from? http://primes.utm.edu/curios/page.php/16777619.html
11 years, 4 months ago (2012-11-07 22:59:40 UTC) #9
iant
https://codereview.appspot.com/6572043/diff/14001/src/cmd/ld/symtab.c File src/cmd/ld/symtab.c (right): https://codereview.appspot.com/6572043/diff/14001/src/cmd/ld/symtab.c#newcode389 src/cmd/ld/symtab.c:389: // s->hide = 1; Why is s->hide = 1 ...
11 years, 4 months ago (2012-11-07 23:36:53 UTC) #10
rsc
Hello r@golang.org, remyoudompheng@gmail.com, mirtchovski@gmail.com, iant@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
11 years, 4 months ago (2012-11-12 16:56:33 UTC) #11
r
LGTM as far as i understand it. https://codereview.appspot.com/6572043/diff/21001/src/cmd/gc/reflect.c File src/cmd/gc/reflect.c (right): https://codereview.appspot.com/6572043/diff/21001/src/cmd/gc/reflect.c#newcode635 src/cmd/gc/reflect.c:635: p = ...
11 years, 4 months ago (2012-11-13 00:31:25 UTC) #12
iant
LGTM https://codereview.appspot.com/6572043/diff/21001/src/cmd/gc/reflect.c File src/cmd/gc/reflect.c (right): https://codereview.appspot.com/6572043/diff/21001/src/cmd/gc/reflect.c#newcode525 src/cmd/gc/reflect.c:525: if(ot != 0) Is this useful? https://codereview.appspot.com/6572043/diff/21001/src/pkg/reflect/type.go File ...
11 years, 4 months ago (2012-11-13 01:36:45 UTC) #13
rsc
Sure, preserving toType is fine. I brought back a toType function and documented why it ...
11 years, 4 months ago (2012-11-13 17:59:16 UTC) #14
rsc
11 years, 4 months ago (2012-11-13 18:06:28 UTC) #15
*** Submitted as http://code.google.com/p/go/source/detail?r=2e2a1d8184e6 ***

reflect: add ArrayOf, ChanOf, MapOf, SliceOf

In order to add these, we need to be able to find references
to such types that already exist in the binary. To do that, introduce
a new linker section holding a list of the types corresponding to
arrays, chans, maps, and slices.

To offset the storage cost of this list, and to simplify the code,
remove the interface{} header from the representation of a
runtime type. It was used in early versions of the code but was
made obsolete by the kind field: a switch on kind is more efficient
than a type switch.

In the godoc binary, removing the interface{} header cuts two
words from each of about 10,000 types. Adding back the list of pointers
to array, chan, map, and slice types reintroduces one word for
each of about 500 types. On a 64-bit machine, then, this CL *removes*
a net 156 kB of read-only data from the binary.

This CL does not include the needed support for precise garbage
collection. I have created issue 4375 to track that.

This CL also does not set the 'algorithm' - specifically the equality
and copy functions - for a new array correctly, so I have unexported
ArrayOf for now. That is also part of issue 4375.

Fixes issue 2339.

R=r, remyoudompheng, mirtchovski, iant
CC=golang-dev
http://codereview.appspot.com/6572043

http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):

http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#n...
src/pkg/reflect/all_test.go:2717: }
On 2012/11/07 22:06:23, rsc wrote:
> On 2012/11/07 21:54:15, remyoudompheng wrote:
> > I would like to see something like:
> > 
> > vi := v.Interface()
> > if _, ok := vi.([10]T); !ok {
> >     t.Errorf("constructed type %T != [10]T", vi)
> > }
> 
> The tricky part is that if I add that test I lose what the above is testing,
> namely that the newly constructed type ends up working. If the binary mentions
> [10]T explicitly, reflect won't have to synthesize [10]T. However, I can add a
> test for something different, like [5]T.

Done.

http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#n...
src/pkg/reflect/all_test.go:2731: }
On 2012/11/07 21:54:15, remyoudompheng wrote:
> same test missing

Done.

http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#n...
src/pkg/reflect/all_test.go:2747: }
On 2012/11/07 21:54:15, remyoudompheng wrote:
> same test

Done.

http://codereview.appspot.com/6572043/diff/4001/src/pkg/reflect/all_test.go#n...
src/pkg/reflect/all_test.go:2763: 
On 2012/11/07 21:54:15, remyoudompheng wrote:
> same

Done.

http://codereview.appspot.com/6572043/diff/14001/src/cmd/ld/symtab.c
File src/cmd/ld/symtab.c (right):

http://codereview.appspot.com/6572043/diff/14001/src/cmd/ld/symtab.c#newcode389
src/cmd/ld/symtab.c:389: //	s->hide = 1;
On 2012/11/07 23:36:53, iant wrote:
> Why is s->hide = 1 commented out?

Probably a debugging dreg; fixed.

http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/all_test.go
File src/pkg/reflect/all_test.go (right):

http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/all_test.go#...
src/pkg/reflect/all_test.go:2706: func TestArrayOf(t *testing.T) {
On 2012/11/07 23:36:53, iant wrote:
> I think you also need some tests that these functions return the same type as
> the type in the program.
>     type F float
>     at := ArrayOf(10, TypeOf(F(1)))
>     if at != TypeOf([10]F) { panic("different type") }

Done.

http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go
File src/pkg/reflect/type.go (right):

http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newc...
src/pkg/reflect/type.go:1017: // of a *unsafe.Pointer.
On 2012/11/07 22:38:47, r wrote:
> s/a/an/ ?

Done.

http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newc...
src/pkg/reflect/type.go:1249: // typesByName returns the subslice of typelinks()
with the given string.
On 2012/11/07 22:38:47, r wrote:
> typesByName returns the subslice of typelinks() whose elements have the given
> string representation.

Done.

http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newc...
src/pkg/reflect/type.go:1252: func typesByString(s string) []*rtype {
On 2012/11/07 23:36:53, iant wrote:
> The comment says typesByName but the function is called typesByString.

Done.

http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newc...
src/pkg/reflect/type.go:1276: return typ[i:j]
On 2012/11/07 22:38:47, r wrote:
> looks like you get the empty slice (at the end of the typelinks slice) for no
> such string. should probably document that.

Done.

http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newc...
src/pkg/reflect/type.go:1377: ch.hash = typ.hash*16777619 ^ 'c'
On 2012/11/07 22:38:47, r wrote:
> where does this number come from? at least give it a name - it appears
> everywhere. regardless, i think you want a variadic function that constructs
the
> hash value, so the number appears in only one place. this isn't quite it, i
> don't think, but something like it would be nice:
> 
> func newTypeHash(x ...uintptr) (h uintptr) {
>   for u := range x {
>    h = h*16777619 ^ x
>   }
>   return
> }

Done.

http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newc...
src/pkg/reflect/type.go:1378: ch.hash = typ.hash*16777619 ^ uint32(dir)
On 2012/11/07 23:36:53, iant wrote:
> This code doesn't make sense as written.  Do you want s/typ.hash/ch.hash/ ?

Done.

http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newc...
src/pkg/reflect/type.go:1386: // MapOf returns the map type with the given key
and value types.
On 2012/11/07 22:38:47, r wrote:
> s/value/element/

Done.

http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newc...
src/pkg/reflect/type.go:1388: // MapOf(k, v) represents map[int]string.
On 2012/11/07 22:38:47, r wrote:
> s/v/e/

Done.

http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newc...
src/pkg/reflect/type.go:1391: // not implement Go's == operator), MapOf panics.
On 2012/11/07 22:38:47, r wrote:
> s;$; TODO

Done.

http://codereview.appspot.com/6572043/diff/14001/src/pkg/reflect/type.go#newc...
src/pkg/reflect/type.go:1470: // ArrayOf panics.
On 2012/11/07 22:38:47, r wrote:
> it does?

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b