|
|
Descriptionreflect: 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.
Patch Set 1 #Patch Set 2 : diff -r 04f0931c9808 https://khr%40golang.org@code.google.com/p/go/ #Patch Set 3 : diff -r 04f0931c9808 https://khr%40golang.org@code.google.com/p/go/ #Patch Set 4 : diff -r 04f0931c9808 https://khr%40golang.org@code.google.com/p/go/ #Patch Set 5 : diff -r 04f0931c9808 https://khr%40golang.org@code.google.com/p/go/ #Patch Set 6 : diff -r d99f48a196e6 https://khr%40golang.org@code.google.com/p/go/ #Patch Set 7 : diff -r d99f48a196e6 https://khr%40golang.org@code.google.com/p/go/ #Patch Set 8 : diff -r d99f48a196e6 https://khr%40golang.org@code.google.com/p/go/ #Patch Set 9 : diff -r 455c7e94abfc https://khr%40golang.org@code.google.com/p/go/ #
Total comments: 21
Patch Set 10 : diff -r 455c7e94abfc https://khr%40golang.org@code.google.com/p/go/ #
Total comments: 3
Patch Set 11 : diff -r 455c7e94abfc https://khr%40golang.org@code.google.com/p/go/ #Patch Set 12 : diff -r e5776ade4bc0 https://khr%40golang.org@code.google.com/p/go/ #
Total comments: 1
MessagesTotal messages: 19
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://khr%40golang.org@code.google.com/p/go/
Sign in to reply to this message.
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.
Sign in to reply to this message.
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
Sign in to reply to this message.
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/ >
Sign in to reply to this message.
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/ >
Sign in to reply to this message.
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 ...
Sign in to reply to this message.
*** 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
Sign in to reply to this message.
Message was sent while issue was closed.
FYI, this caused ~50% degradation of json benchmark: http://goperfd.appspot.com/perfdetail?commit=ecccf07e7f9d341ca577433b794ab806...
Sign in to reply to this message.
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/ >
Sign in to reply to this message.
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.
Sign in to reply to this message.
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. >
Sign in to reply to this message.
It's possible that //go:noescape annotations will help. I forgot to reply to that question. Russ
Sign in to reply to this message.
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 > >
Sign in to reply to this message.
Message was sent while issue was closed.
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{}.
Sign in to reply to this message.
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/ >
Sign in to reply to this message.
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/ > >
Sign in to reply to this message.
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/ >> > >
Sign in to reply to this message.
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/ >>> >> >>
Sign in to reply to this message.
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/ >>> >>> >
Sign in to reply to this message.
|