code review 43040043: reflect: rewrite Value to separate out pointer vs. nonp...

reflect: rewrite Value to separate out pointer vs. nonpointer info.
Needed for precise gc and copying stacks.

reflect.Value now takes 4 words instead of 3.

Still to do:
 - un-iword-ify channel ops.
 - un-iword-ify method receivers.

[khr, 2013-12-17 19:14:18]

Hello golang-dev@googlegroups.com,

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

[iant, 2013-12-18 02:11:36]

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

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:66: // is set, or for for any pointer-like values.
s/for for/for/

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:67: // When the data is smaller than a word, it begins
at
For a pointer, I don't see how the data could ever be smaller than a word.  I
think you can omit this sentence here while leaving it for the scalar field.

[rsc, 2013-12-18 03:56:48]

i assume you plan to do the "still to do" in other CLs.

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

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:67: // When the data is smaller than a word, it begins
at
On 2013/12/18 02:11:37, iant wrote:
> For a pointer, I don't see how the data could ever be smaller than a word.  I
> think you can omit this sentence here while leaving it for the scalar field.

The data pointed at can certainly be smaller than a word, but the wording here
is still a bit out of place.
I'd rewrite the whole comment to:

// Pointer-valued data or, if flagIndir is set, pointer to data.

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:116: // Returns the underlying pointer in the value.
doc comments should be complete sentences.

// getPtr returns the underlying pointer represented by v.
(except s/getPtr/pointer/; see next comment)

http://golang.org/doc/effective_go.html#commentary

this applies to more doc comments below, not explicitly flagged.

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:118: func (v Value) getPtr() unsafe.Pointer {
s/getPtr/pointer/
getXxx is non-idiomatic. always better to find a way to drop the get.
http://golang.org/doc/effective_go.html#Getters

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:119: if !(v.typ.size == ptrSize && v.typ.pointers()) {
It is usually easier to read the condition if you push the ! down.

if v.typ.size != ptrSize || !v.typ.pointers() {

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:130: t := v.typ
it would be nice to simplify this function.

var i interface{}
e := (*interface{})(unsafe.Pointer(&i))
switch {
case t.size > ptrSize:
    // indirect in Value, indirect in interface
    ptr := v.ptr
    ... maybe copy
    e.word = ptr
case v.flag&flagIndir != 0:
    // indirect in Value, but no indirect in interface
    if t.pointers() {
        e.word = *(*unsafe.Pointer)(v.ptr)
    } else {
        e.word = iword(loadScalar(v.ptr, t.size))
    }
default:
    // no indirect in Value, no indirect in interface
    if t.pointers() {
        e.word = iword(v.ptr)
    } else {
        e.word = iword(v.scalar)
    }
}
e.typ = t // e.word now visible to garbage collector
return i

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:883: x.flag |= (v.flag & flagRO)
drop unnecessary ( )

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:922: var ptr unsafe.Pointer = nil
s/= nil//
s/= 0// on next line

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:1018: var val unsafe.Pointer = nil
same

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:1059: *(*byte)(unsafe.Pointer(&b)) =
*(*byte)(unsafe.Pointer(uintptr(s.Data) + uintptr(i)))
nice endian bug fix

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:1141: // TODO: deprecate this
move the TODO inside the func. as written it shows up in godoc, but it is not
polished enough for that.

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:1232: k = unsafe.Pointer(&key.ptr)
This generates garbage that didn't used to happen.
It's not something we can fix in this CL, I just want to point it out.
It's unfortunate.

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:1244: // copy result so a Value won't keep the map
alive forever
// Copy result to isolate from future changes to map data.

(that is, the copy is for correctness, not for performance)

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:1283: // copy result so a Value won't keep the map
alive forever
same

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:1424: // TODO: deprecate
move inside func

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:1451: // TODO: What happens on a nil slice?  An empty
but not nil slice?
A nil slice returns 0.
A non-nil slice returns non-zero.

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:1903: // TODO: deprecate
move into func

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:1925: // The above isn't safe, we use the one below
internally.
complete sentences please

// stringHeader is a safe version of StringHeader used within this package.

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:1938: Data uintptr // TODO: make unsafe.Pointer?
move TODO somewhere not visible in godoc (outside the type, and also not in the
doc comment)

https://codereview.appspot.com/43040043/diff/120002/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:1943: // The above isn't safe, we use the one below
internally.
same as stringHeader

[khr1, 2013-12-19 02:07:10]

On Tue, Dec 17, 2013 at 6:11 PM, <iant@golang.org> wrote:

>
> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go
> File src/pkg/reflect/value.go (right):
>
> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode66
> src/pkg/reflect/value.go:66: // is set, or for for any pointer-like
> values.
> s/for for/for/
>
> Yep.


> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode67
> src/pkg/reflect/value.go:67: // When the data is smaller than a word, it
> begins at
> For a pointer, I don't see how the data could ever be smaller than a
> word.  I think you can omit this sentence here while leaving it for the
> scalar field.
>
> https://codereview.appspot.com/43040043/
>

[khr1, 2013-12-19 02:07:17]

On Tue, Dec 17, 2013 at 7:56 PM, <rsc@golang.org> wrote:

> i assume you plan to do the "still to do" in other CLs.
>
> Yes.


>
>
>
> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go
> File src/pkg/reflect/value.go (right):
>
> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode67
> src/pkg/reflect/value.go:67: // When the data is smaller than a word, it
> begins at
> On 2013/12/18 02:11:37, iant wrote:
>
>> For a pointer, I don't see how the data could ever be smaller than a
>>
> word.  I
>
>> think you can omit this sentence here while leaving it for the scalar
>>
> field.
>
> The data pointed at can certainly be smaller than a word, but the
> wording here is still a bit out of place.
> I'd rewrite the whole comment to:
>
> // Pointer-valued data or, if flagIndir is set, pointer to data.
>
> Done.


> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode116
> src/pkg/reflect/value.go:116: // Returns the underlying pointer in the
> value.
> doc comments should be complete sentences.
>
> // getPtr returns the underlying pointer represented by v.
> (except s/getPtr/pointer/; see next comment)
>
> http://golang.org/doc/effective_go.html#commentary
>
> this applies to more doc comments below, not explicitly flagged.
>
> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode118
> src/pkg/reflect/value.go:118: func (v Value) getPtr() unsafe.Pointer {
> s/getPtr/pointer/
> getXxx is non-idiomatic. always better to find a way to drop the get.
> http://golang.org/doc/effective_go.html#Getters


Fixed.


>
>
> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode119
> src/pkg/reflect/value.go:119: if !(v.typ.size == ptrSize &&
> v.typ.pointers()) {
> It is usually easier to read the condition if you push the ! down.
>
> if v.typ.size != ptrSize || !v.typ.pointers() {
>

Fixed.


>
> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode130
> src/pkg/reflect/value.go:130: t := v.typ
> it would be nice to simplify this function.
>
> var i interface{}
> e := (*interface{})(unsafe.Pointer(&i))
> switch {
> case t.size > ptrSize:
>     // indirect in Value, indirect in interface
>     ptr := v.ptr
>     ... maybe copy
>     e.word = ptr
> case v.flag&flagIndir != 0:
>     // indirect in Value, but no indirect in interface
>     if t.pointers() {
>         e.word = *(*unsafe.Pointer)(v.ptr)
>     } else {
>         e.word = iword(loadScalar(v.ptr, t.size))
>     }
> default:
>     // no indirect in Value, no indirect in interface
>     if t.pointers() {
>         e.word = iword(v.ptr)
>     } else {
>         e.word = iword(v.scalar)
>     }
> }
> e.typ = t // e.word now visible to garbage collector
> return i
>
>
I used the *(*interface{})(unsafe.Pointer(&emptyInterface{...})) construct
because it makes it painfully clear that there's never a time when we have
a "raw" iword floating around.  In particular, across a call. Your code is
also ok, but it's less clear.  But I like your code, so I'm going to change
it.


> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode883
> src/pkg/reflect/value.go:883: x.flag |= (v.flag & flagRO)
> drop unnecessary ( )
>
> Done.


> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode922
> src/pkg/reflect/value.go:922: var ptr unsafe.Pointer = nil
> s/= nil//
> s/= 0// on next line
>
> Done.


> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode1018
> src/pkg/reflect/value.go:1018: var val unsafe.Pointer = nil
> same
>
> Done.


> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode1059
> src/pkg/reflect/value.go:1059: *(*byte)(unsafe.Pointer(&b)) =
> *(*byte)(unsafe.Pointer(uintptr(s.Data) + uintptr(i)))
> nice endian bug fix
>
There are probably others...


>
> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode1141
> src/pkg/reflect/value.go:1141: // TODO: deprecate this
> move the TODO inside the func. as written it shows up in godoc, but it
> is not polished enough for that.
>
> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode1232
> src/pkg/reflect/value.go:1232: k = unsafe.Pointer(&key.ptr)
> This generates garbage that didn't used to happen.
> It's not something we can fix in this CL, I just want to point it out.
> It's unfortunate.
>

Would annotating mapaccess with //go:noescape fix that?


>
> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode1244
> src/pkg/reflect/value.go:1244: // copy result so a Value won't keep the
> map alive forever
> // Copy result to isolate from future changes to map data.
>
> (that is, the copy is for correctness, not for performance)
>
Right.


>
> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode1283
> src/pkg/reflect/value.go:1283: // copy result so a Value won't keep the
> map alive forever
> same
>
> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode1424
> src/pkg/reflect/value.go:1424: // TODO: deprecate
> move inside func
>
> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode1451
> src/pkg/reflect/value.go:1451: // TODO: What happens on a nil slice?  An
> empty but not nil slice?
> A nil slice returns 0.
> A non-nil slice returns non-zero.
>
I added some text to the doc comment.

>
> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode1903
> src/pkg/reflect/value.go:1903: // TODO: deprecate
> move into func
>
> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode1925
> src/pkg/reflect/value.go:1925: // The above isn't safe, we use the one
> below internally.
> complete sentences please
>
> // stringHeader is a safe version of StringHeader used within this
> package.
>
Fixed.

>
> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode1938
> src/pkg/reflect/value.go:1938: Data uintptr // TODO: make
> unsafe.Pointer?
> move TODO somewhere not visible in godoc (outside the type, and also not
> in the doc comment)
>
I just got rid of it, the declaration of sliceHeader below that makes it
clear enough.

>
> https://codereview.appspot.com/43040043/diff/120002/src/
> pkg/reflect/value.go#newcode1943
> src/pkg/reflect/value.go:1943: // The above isn't safe, we use the one
> below internally.
> same as stringHeader
>
Fixed.

>
> https://codereview.appspot.com/43040043/
>

[rsc, 2013-12-19 17:13:06]

LGTM

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

https://codereview.appspot.com/43040043/diff/150001/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:112: // pointer() returns the underlying pointer
represented by v.
s/()//

https://codereview.appspot.com/43040043/diff/150001/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:124: // Casts v to the empty interface.
// packEface converts v

(Noun, but also Go uses the word convert instead of cast in the language
definitions.)

https://codereview.appspot.com/43040043/diff/150001/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:166: // Casts the empty interface i to a Value.
// unpackEface converts the ...

[khr, 2013-12-19 23:15:30]

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

reflect: rewrite Value to separate out pointer vs. nonpointer info.
Needed for precise gc and copying stacks.

reflect.Value now takes 4 words instead of 3.

Still to do:
 - un-iword-ify channel ops.
 - un-iword-ify method receivers.

R=golang-dev, iant, rsc, khr
CC=golang-dev
https://codereview.appspot.com/43040043

[dvyukov, 2013-12-20 08:59:21]

FYI, this caused ~50% degradation of json benchmark:

http://goperfd.appspot.com/perfdetail?commit=ecccf07e7f9d341ca577433b794ab806...

[khr1, 2013-12-20 23:08:05]

So what incantation do I need to do to reproduce this on the command line?


2013/12/20 <dvyukov@google.com>

> FYI, this caused ~50% degradation of json benchmark:
>
> http://goperfd.appspot.com/perfdetail?commit=
> ecccf07e7f9d341ca577433b794ab8061fbdc092&commit0=
> e5776ade4bc02d937c9100c0ce79395e76cff614&kind=benchmark&benchmark=json
>
>
>
> https://codereview.appspot.com/43040043/
>

[dfc, 2013-12-20 23:12:45]

Here is what I used

 2004  hg update -r e5776ade4bc02d937c9100c0ce79395e76cff614
 2005  ./make.bash
 2006  go test -bench=. encoding/json > old.txt
 2007  hg update -r ecccf07e7f9d341ca577433b794ab8061fbdc092
 2008  ./make.bash
 2009  go test -bench=. encoding/json > new.txt
 2010  ~/go/misc/benchcmp {old,new}.txt
 2011  history
lucky(~/go/src) % ~/go/misc/benchcmp {old,new}.txt
benchmark                      old ns/op    new ns/op    delta
BenchmarkCodeEncoder            25513267     36812653  +44.29%
BenchmarkCodeMarshal            27115349     38723301  +42.81%
BenchmarkCodeDecoder            91611782    128648758  +40.43%
BenchmarkCodeUnmarshal          90997758    151416339  +66.40%
BenchmarkCodeUnmarshalReuse     85971439    142930175  +66.25%
BenchmarkUnmarshalString             777         1092  +40.54%
BenchmarkUnmarshalFloat64            644          909  +41.15%
BenchmarkUnmarshalInt64              588          872  +48.30%
BenchmarkSkipValue               2003906      1972727   -1.56%
BenchmarkEncoderEncode              1010         1383  +36.93%

benchmark                       old MB/s     new MB/s  speedup
BenchmarkCodeEncoder               76.06        52.71    0.69x
BenchmarkCodeMarshal               71.56        50.11    0.70x
BenchmarkCodeDecoder               21.18        15.08    0.71x
BenchmarkCodeUnmarshal             21.32        12.82    0.60x
BenchmarkSkipValue                122.53       124.47    1.02x

benchmark                     old allocs   new allocs    delta
BenchmarkEncoderEncode                 2            2    0.00%

benchmark                      old bytes    new bytes    delta
BenchmarkEncoderEncode               221          221    0.00%

On Sat, Dec 21, 2013 at 10:08 AM, Keith Randall <khr@google.com> wrote:
> So what incantation do I need to do to reproduce this on the command line?
>
>
> 2013/12/20 <dvyukov@google.com>
>
>> FYI, this caused ~50% degradation of json benchmark:
>>
>>
>>
http://goperfd.appspot.com/perfdetail?commit=ecccf07e7f9d341ca577433b794ab806...
>>
>>
>>
>> https://codereview.appspot.com/43040043/
>
>
> --
>
> ---
> 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.

[ugorji, 2013-12-20 23:20:32]

The performance hit was not just on json. I upgraded, and saw severe 
degradation on all encodings (json, gob, and the ones I maintain ie 
msgpack, binc). All suffered about 50% performance degradation on my 
benchmarks, and increase in allocations. 

On Friday, December 20, 2013 6:12:44 PM UTC-5, Dave Cheney wrote:
>
> Here is what I used 
>
>  2004  hg update -r e5776ade4bc02d937c9100c0ce79395e76cff614 
>  2005  ./make.bash 
>  2006  go test -bench=. encoding/json > old.txt 
>  2007  hg update -r ecccf07e7f9d341ca577433b794ab8061fbdc092 
>  2008  ./make.bash 
>  2009  go test -bench=. encoding/json > new.txt 
>  2010  ~/go/misc/benchcmp {old,new}.txt 
>  2011  history 
> lucky(~/go/src) % ~/go/misc/benchcmp {old,new}.txt 
> benchmark                      old ns/op    new ns/op    delta 
> BenchmarkCodeEncoder            25513267     36812653  +44.29% 
> BenchmarkCodeMarshal            27115349     38723301  +42.81% 
> BenchmarkCodeDecoder            91611782    128648758  +40.43% 
> BenchmarkCodeUnmarshal          90997758    151416339  +66.40% 
> BenchmarkCodeUnmarshalReuse     85971439    142930175  +66.25% 
> BenchmarkUnmarshalString             777         1092  +40.54% 
> BenchmarkUnmarshalFloat64            644          909  +41.15% 
> BenchmarkUnmarshalInt64              588          872  +48.30% 
> BenchmarkSkipValue               2003906      1972727   -1.56% 
> BenchmarkEncoderEncode              1010         1383  +36.93% 
>
> benchmark                       old MB/s     new MB/s  speedup 
> BenchmarkCodeEncoder               76.06        52.71    0.69x 
> BenchmarkCodeMarshal               71.56        50.11    0.70x 
> BenchmarkCodeDecoder               21.18        15.08    0.71x 
> BenchmarkCodeUnmarshal             21.32        12.82    0.60x 
> BenchmarkSkipValue                122.53       124.47    1.02x 
>
> benchmark                     old allocs   new allocs    delta 
> BenchmarkEncoderEncode                 2            2    0.00% 
>
> benchmark                      old bytes    new bytes    delta 
> BenchmarkEncoderEncode               221          221    0.00% 
>
> On Sat, Dec 21, 2013 at 10:08 AM, Keith Randall <k...@google.com<javascript:>>

> wrote: 
> > So what incantation do I need to do to reproduce this on the command 
> line? 
> > 
> > 
> > 2013/12/20 <dvy...@google.com <javascript:>> 
> > 
> >> FYI, this caused ~50% degradation of json benchmark: 
> >> 
> >> 
> >> 
>
http://goperfd.appspot.com/perfdetail?commit=ecccf07e7f9d341ca577433b794ab806...

> >> 
> >> 
> >> 
> >> https://codereview.appspot.com/43040043/ 
> > 
> > 
> > -- 
> > 
> > --- 
> > 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+...@googlegroups.com <javascript:>. 
> > For more options, visit https://groups.google.com/groups/opt_out. 
>

[rsc, 2013-12-20 23:34:18]

It's possible that //go:noescape annotations will help. I forgot to reply
to that question.

Russ

[khr1, 2013-12-21 18:00:39]

See 44990043, the performance regression was due to struct copies being
inlined for 3-word structures but not 4-word structures.

More generally, maybe we should revisit how struct copies are generated.



On Fri, Dec 20, 2013 at 3:34 PM, Russ Cox <rsc@golang.org> wrote:

> It's possible that //go:noescape annotations will help. I forgot to reply
> to that question.
>
> Russ
>
>

[dvyukov, 2014-01-16 06:55:05]

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

https://codereview.appspot.com/43040043/diff/190001/src/pkg/reflect/value.go#...
src/pkg/reflect/value.go:73: scalar uintptr
I am curious, can we put interface{} here, which is exactly {type, word}. And
then use unsafe to split it into words and manipulate them directly. Or
something along these lines?
The point is that compiler/GC must handle interface{}'s in a type-safe manner
one way or another, regardless of the fact that the word can be pointer or not.
So we can take advantage of this support to also have "maybe pointer" word if we
pretend that it is interface{}.

[khr1, 2014-01-16 07:49:04]

Yes, I'd like to get get Value back to 3 words by doing this.  It isn't
trivial, you have to handle flagAddr correctly and makefunc gets
complicated.  I have a start on it, CL 52670045.



On Wed, Jan 15, 2014 at 10:55 PM, <dvyukov@google.com> wrote:

>
> https://codereview.appspot.com/43040043/diff/190001/src/
> pkg/reflect/value.go
> File src/pkg/reflect/value.go (right):
>
> https://codereview.appspot.com/43040043/diff/190001/src/
> pkg/reflect/value.go#newcode73
> src/pkg/reflect/value.go:73: scalar uintptr
> I am curious, can we put interface{} here, which is exactly {type,
> word}. And then use unsafe to split it into words and manipulate them
> directly. Or something along these lines?
> The point is that compiler/GC must handle interface{}'s in a type-safe
> manner one way or another, regardless of the fact that the word can be
> pointer or not. So we can take advantage of this support to also have
> "maybe pointer" word if we pretend that it is interface{}.
>
> https://codereview.appspot.com/43040043/
>

[dvyukov, 2014-01-16 08:11:13]

Great!

On Thu, Jan 16, 2014 at 11:49 AM, Keith Randall <khr@google.com> wrote:
> Yes, I'd like to get get Value back to 3 words by doing this.  It isn't
> trivial, you have to handle flagAddr correctly and makefunc gets
> complicated.  I have a start on it, CL 52670045.
>
>
>
> On Wed, Jan 15, 2014 at 10:55 PM, <dvyukov@google.com> wrote:
>>
>>
>>
>> https://codereview.appspot.com/43040043/diff/190001/src/pkg/reflect/value.go
>> File src/pkg/reflect/value.go (right):
>>
>>
>>
https://codereview.appspot.com/43040043/diff/190001/src/pkg/reflect/value.go#...
>> src/pkg/reflect/value.go:73: scalar uintptr
>> I am curious, can we put interface{} here, which is exactly {type,
>> word}. And then use unsafe to split it into words and manipulate them
>> directly. Or something along these lines?
>> The point is that compiler/GC must handle interface{}'s in a type-safe
>> manner one way or another, regardless of the fact that the word can be
>> pointer or not. So we can take advantage of this support to also have
>> "maybe pointer" word if we pretend that it is interface{}.
>>
>> https://codereview.appspot.com/43040043/
>
>

[ugorji, 2014-02-27 10:31:39]

Hi,

Will this change https://codereview.appspot.com/52670045/ make it in to go 
1.3? It seems like it would have a clear performance impact for code using 
reflection (fmt, json, gob, other codecs, etc).

Thanks.

On Thursday, January 16, 2014 2:49:04 AM UTC-5, Keith Randall wrote:
>
> Yes, I'd like to get get Value back to 3 words by doing this.  It isn't 
> trivial, you have to handle flagAddr correctly and makefunc gets 
> complicated.  I have a start on it, CL 52670045.
>
>
>
> On Wed, Jan 15, 2014 at 10:55 PM, <dvy...@google.com <javascript:>> wrote:
>
>>
>> https://codereview.appspot.com/43040043/diff/190001/src/
>> pkg/reflect/value.go
>> File src/pkg/reflect/value.go (right):
>>
>> https://codereview.appspot.com/43040043/diff/190001/src/
>> pkg/reflect/value.go#newcode73
>> src/pkg/reflect/value.go:73: scalar uintptr
>> I am curious, can we put interface{} here, which is exactly {type,
>> word}. And then use unsafe to split it into words and manipulate them
>> directly. Or something along these lines?
>> The point is that compiler/GC must handle interface{}'s in a type-safe
>> manner one way or another, regardless of the fact that the word can be
>> pointer or not. So we can take advantage of this support to also have
>> "maybe pointer" word if we pretend that it is interface{}.
>>
>> https://codereview.appspot.com/43040043/
>>
>
>

[khr1, 2014-02-27 16:10:02]

No, that change is not working yet.


On Thu, Feb 27, 2014 at 2:31 AM, Ugorji <ugorji@gmail.com> wrote:

> Hi,
>
> Will this change https://codereview.appspot.com/52670045/ make it in to
> go 1.3? It seems like it would have a clear performance impact for code
> using reflection (fmt, json, gob, other codecs, etc).
>
> Thanks.
>
> On Thursday, January 16, 2014 2:49:04 AM UTC-5, Keith Randall wrote:
>
>> Yes, I'd like to get get Value back to 3 words by doing this.  It isn't
>> trivial, you have to handle flagAddr correctly and makefunc gets
>> complicated.  I have a start on it, CL 52670045.
>>
>>
>>
>> On Wed, Jan 15, 2014 at 10:55 PM, <dvy...@google.com> wrote:
>>
>>>
>>> https://codereview.appspot.com/43040043/diff/190001/src/pkg/
>>> reflect/value.go
>>> File src/pkg/reflect/value.go (right):
>>>
>>> https://codereview.appspot.com/43040043/diff/190001/src/pkg/
>>> reflect/value.go#newcode73
>>> src/pkg/reflect/value.go:73: scalar uintptr
>>> I am curious, can we put interface{} here, which is exactly {type,
>>> word}. And then use unsafe to split it into words and manipulate them
>>> directly. Or something along these lines?
>>> The point is that compiler/GC must handle interface{}'s in a type-safe
>>> manner one way or another, regardless of the fact that the word can be
>>> pointer or not. So we can take advantage of this support to also have
>>> "maybe pointer" word if we pretend that it is interface{}.
>>>
>>> https://codereview.appspot.com/43040043/
>>>
>>
>>

[dvyukov, 2014-02-27 17:22:35]

Filed an issue for 1.4
https://code.google.com/p/go/issues/detail?id=7425


On Thu, Feb 27, 2014 at 8:10 PM, Keith Randall <khr@google.com> wrote:
> No, that change is not working yet.
>
>
> On Thu, Feb 27, 2014 at 2:31 AM, Ugorji <ugorji@gmail.com> wrote:
>>
>> Hi,
>>
>> Will this change https://codereview.appspot.com/52670045/ make it in to go
>> 1.3? It seems like it would have a clear performance impact for code using
>> reflection (fmt, json, gob, other codecs, etc).
>>
>> Thanks.
>>
>> On Thursday, January 16, 2014 2:49:04 AM UTC-5, Keith Randall wrote:
>>>
>>> Yes, I'd like to get get Value back to 3 words by doing this.  It isn't
>>> trivial, you have to handle flagAddr correctly and makefunc gets
>>> complicated.  I have a start on it, CL 52670045.
>>>
>>>
>>>
>>> On Wed, Jan 15, 2014 at 10:55 PM, <dvy...@google.com> wrote:
>>>>
>>>>
>>>>
>>>>
https://codereview.appspot.com/43040043/diff/190001/src/pkg/reflect/value.go
>>>> File src/pkg/reflect/value.go (right):
>>>>
>>>>
>>>>
https://codereview.appspot.com/43040043/diff/190001/src/pkg/reflect/value.go#...
>>>> src/pkg/reflect/value.go:73: scalar uintptr
>>>> I am curious, can we put interface{} here, which is exactly {type,
>>>> word}. And then use unsafe to split it into words and manipulate them
>>>> directly. Or something along these lines?
>>>> The point is that compiler/GC must handle interface{}'s in a type-safe
>>>> manner one way or another, regardless of the fact that the word can be
>>>> pointer or not. So we can take advantage of this support to also have
>>>> "maybe pointer" word if we pretend that it is interface{}.
>>>>
>>>> https://codereview.appspot.com/43040043/
>>>
>>>
>