Incompatibility of reflection libraries (reflect versus go-reflect), on json-iterator (Go 1.15.8)

[Yan Titarenko]

Hi.

Since there where no comments on https://github.com/json-iterator/go/issues/501 - I decided that an open discussion (not necessarily on behalf of this project - there could be other projects/use cases, which could a cause for contributions, into the reflection library).

I tried to test with Golang 1.15.8. I tried an old version of llvm-goc/gollvm - but can re-check, using the latest version of gollvm.

Also hence that one of the checks was done on x86_64 Windows 10.

It is not clear what kind of patch (and in favor of which engineering targets) would be required, yet - so we are not at the point where we could excavate OS/arch dependent issues.

I see a lot of errors like

./any.go:259:15: undefined: "github.com/goccy/go-reflect".TypeOfPtr
./reflect_array.go:36:15: undefined: "github.com/goccy/go-reflect".UnsafeArrayType

.

I tried to do two things: I tried to revert back from reflect2 to libgo's reflect - and I tried to use goccy's go-reflect, instead of reflect2.

It is interesting that both attempts brought similar errors - but they also brought some unique errors.

But nothing that would bring specific practical wage, just case of that.

Yan

[Yan Titarenko]

CC'ing to committers.

Ian,

please provide some advise.

Thanks in advance

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/ab44891f-eca6-47cb-a4b1-c6292d6015a7n%40googlegroups.com.

[Robert Engels]

Maybe direct your inquiries to the developer if go-reflect?

On Feb 26, 2021, at 8:11 AM, Yan Titarenko <local.tou...@gmail.com> wrote:



To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CANFCsz9PhiGw2Ji2t%2BE1Zo15N%3D6%2BuCZQvyWryoqC0ZbAKNNBMw%40mail.gmail.com.

[Ian Lance Taylor]

On Fri, Feb 26, 2021 at 6:11 AM Yan Titarenko
<local.tou...@gmail.com> wrote:
>
> CC'ing to committers.
>
> Ian,
> please provide some advise.

I'm sorry, I don't have anything useful to add beyond what I've already said.

I don't know why people are using the reflect2 package. The package
does not support gccgo/GoLLVM. The first step is to understand why
people use reflect2, and whether we can improve the standard reflect
package to address whatever problems led to the creation of reflect2.

Ian

[Than McIntosh]

My sense is that developers are using packages like reflect2 for performance reasons, very often to try to improve JSON marshalling/unmarshalling speed. These packages tend to bypass the Go type system using "unsafe" in order to provide reflect-like operations without the overhead (e.g. allocation) that would be incurred if they used the regular reflect package in the standard library.

Because these packages are "reaching under the hood" via unsafe, they tend to be written against a specific implementation (and in fact against a specific version of a specific implementation).  So probably not surprising that they do not play well with gccgo/gollvm.

Than

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CAKOQZ8zbyemhzCd_1cEAkrc5FAwcdDOyo%3DxSBNafcA_mTpdYnQ%40mail.gmail.com.

[Ian Lance Taylor]

On Fri, Feb 26, 2021 at 7:20 AM Than McIntosh <th...@google.com> wrote:
>
>
> My sense is that developers are using packages like reflect2 for performance reasons, very often to try to improve JSON marshalling/unmarshalling speed. These packages tend to bypass the Go type system using "unsafe" in order to provide reflect-like operations without the overhead (e.g. allocation) that would be incurred if they used the regular reflect package in the standard library.
>
> Because these packages are "reaching under the hood" via unsafe, they tend to be written against a specific implementation (and in fact against a specific version of a specific implementation). So probably not surprising that they do not play well with gccgo/gollvm.

OK, but the reflect package generally does not allocate, so what is
the advantage of reflect2? Are there benchmarks showing where
reflect2 is significantly faster?

(I'll note that while this may not be a good idea for the overall
ecosystem, I expect that it is possible to port reflect2 to support
gccgo/GoLLVM.)

Ian

[Yan Titarenko]

[Yan Titarenko]

On Friday, February 26, 2021 at 5:50:44 PM UTC+2 Ian Lance Taylor wrote:

OK, but the reflect package generally does not allocate, so what is
the advantage of reflect2? Are there benchmarks showing where
reflect2 is significantly faster?

The problem is in the fact that benchmarking of json-iterator is itself a package specific one.

This is not like if we could take 10-15 packages, which use reflect package, to benchmark those. And we could each of the end-user Go projects, to make it work with reflect2 - and run the same benchmark. Then we could patch again, to target go-reflect. Then we could do benchmarking again.

 

(I'll note that while this may not be a good idea for the overall
ecosystem, I expect that it is possible to port reflect2 to support
gccgo/GoLLVM.)

In that case it would be critical to resolve these issues:

https://github.com/modern-go/reflect2/issues/16

. Should those methods and types be referenced? perhaps something else should be used?

Or it should - but we would have to implement the missing ones?

btw this is what never being answered, during all the previous discussions.

It might make sense to really work on various reflections APIs - to learn why it is typically grow into a situation when alternatives are written.

Perhaps it makes sense to react on various issues and demands, of libgo, earlier then the whole Go package (for reflection, in this case) would be formed?

At least it would be clear that is not reasonable to spend time on designing/implementing an alternative, compared to patching libgo's package.

Also is a "corner edge" case, with json-iterator - it would give some disadvantages, in case of other end-user Go package. And that is what authors of such "alternative" do not want to deal with - so reflect2 is, unsurprisingly, unmaintained even by the original author.

[Ian Lance Taylor]

On Tue, Mar 2, 2021 at 5:49 AM Yan Titarenko
<local.tou...@gmail.com> wrote:
>
> On Friday, February 26, 2021 at 5:50:44 PM UTC+2 Ian Lance Taylor wrote:
>>
>>
>> OK, but the reflect package generally does not allocate, so what is
>> the advantage of reflect2? Are there benchmarks showing where
>> reflect2 is significantly faster?
>
>
> The problem is in the fact that benchmarking of json-iterator is itself a package specific one.
>
> This is not like if we could take 10-15 packages, which use reflect package, to benchmark those. And we could each of the end-user Go projects, to make it work with reflect2 - and run the same benchmark. Then we could patch again, to target go-reflect. Then we could do benchmarking again.

I agree that in general its better to use larger scale benchmarks.
That said, for a case like this, if reflect2 is faster, it must be
possible to show that in a micro-benchmark, perhaps one that uses
b.ReportAllocs.

But really I'm just trying to get at a basic fact. Right now I don't
understand in what way reflect2 is faster, and the package docs don't
tell me that either. Do you know?

>> (I'll note that while this may not be a good idea for the overall
>> ecosystem, I expect that it is possible to port reflect2 to support
>> gccgo/GoLLVM.)
>
>
> In that case it would be critical to resolve these issues:
> https://github.com/modern-go/reflect2/issues/16
> . Should those methods and types be referenced? perhaps something else should be used?
> Or it should - but we would have to implement the missing ones?
>
> btw this is what never being answered, during all the previous discussions.

I'm sorry, I'm not answering the question because it doesn't really
make sense to me. There is no gccgo/GoLLVM version of a function like
resolveTypeOff. The fix to make reflect2 work with gccgo/GoLLVM is
not to add resolveTypeOff to gccgo/GoLLVM. It's to change the
reflect2 package to work with the gccgo/GoLLVM versions of the reflect
types.

The gccgo/GoLLVM reflect types are just different from the gc reflect
types. This doesn't mean that reflect2 can't support gccgo/GoLLVM,
but it does mean that the package needs to be modified to support
those types, using appropriate build constraints.

> It might make sense to really work on various reflections APIs - to learn why it is typically grow into a situation when alternatives are written.
> Perhaps it makes sense to react on various issues and demands, of libgo, earlier then the whole Go package (for reflection, in this case) would be formed?
> At least it would be clear that is not reasonable to spend time on designing/implementing an alternative, compared to patching libgo's package.
> Also is a "corner edge" case, with json-iterator - it would give some disadvantages, in case of other end-user Go package. And that is what authors of such "alternative" do not want to deal with - so reflect2 is, unsurprisingly, unmaintained even by the original author.

I would certainly be happy to patch libgo if someone can tell me what
should change. This gets us back to the benchmark discussion.

Ian

[Sokolov Yura]

I use reflect2 in our custom more robust deepcopy implementation in two places:

1. When I need to grow slice.

Standard reflect package has no way to enlarge slice without call to reflect.MakeSlice, which allocates unneeded slice header.

2. When I need to iterate map.

Using standard reflect package there is no way to iterate map[string]int without allocation of string header and int for every key-value pair. When

key or values is struct, things become worse.

Also reflect2.UnsafeMap.UnsafeSetIndex is just a wrapper call to mapassign. Yes, it is unsafe in the meaning it doesn't check types, but it is ok in

this place because I've checked types a bit earlier.

This two things give a lot for performance (and allocation-less-ness) of our deepcopy.

Yura

вторник, 2 марта 2021 г. в 21:18:51 UTC+3, Ian Lance Taylor:

[Yan Titarenko]

CC'ing to Mikko/Justin/taowen/Johannes/Aaron. In case if they would be interested to explain about reflect2's usage.

I tried to patch rook - and here is what I got:

https://github.com/advancedwebdeveloper/rook

https://gist.github.com/advancedwebdeveloper/52be86fa92fd9ed484ef26f5cbfc541c

pkg/apis/ceph.rook.io/v1/cluster.go:40:77: undefined: reflect2.DeepEqual

This is not with gollvm:

$ go version
go version go1.16 linux/arm64

I could re-check with gollvm.

I can tell that I haven't found any build errors, when I patched rook, on behalf of go-reflect. So it could allow to do some benchmarking - we could compare with reflect's performance.

--

You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/53f29e5b-385d-4f04-a488-6af1375a91b5n%40googlegroups.com.

[Ian Lance Taylor]

On Wed, Mar 3, 2021 at 7:02 PM Sokolov Yura <funny....@gmail.com> wrote:
>
> I use reflect2 in our custom more robust deepcopy implementation in two places:
>
> 1. When I need to grow slice.
> Standard reflect package has no way to enlarge slice without call to reflect.MakeSlice, which allocates unneeded slice header.

Thanks. I filed https://golang.org/issue/44870 for this.

> 2. When I need to iterate map.
> Using standard reflect package there is no way to iterate map[string]int without allocation of string header and int for every key-value pair. When
> key or values is struct, things become worse.

That is how the language works also. Map keys and values are not
addressable. Keeping an address for them is very deeply unsafe, and
could break irretrievably with future changes to the garbage
collector. Or, to put it another way, supporting this functionality
imposes limitations on the garbage collector that I don't think we
want to impose. I'm not sure what we can do here. Go's map type
intentionally does not give programs complete control over memory.

It's possible that generics will address some of these use cases.

Ian

[Axel Wagner]

Hi,

On Mon, Mar 8, 2021 at 10:07 PM Ian Lance Taylor <ia...@golang.org> wrote:

> 2. When I need to iterate map.
> Using standard reflect package there is no way to iterate map[string]int without allocation of string header and int for every key-value pair. When
> key or values is struct, things become worse.

That is how the language works also.  Map keys and values are not
addressable.

I'm not sure I fully understand the issue, but

for k, v := range m

only needs to allocate storage for k/v *once*. AIUI, `MapIter.Key` and `MapIter.Value` need to allocate on every iteration (or at least that's how I read the comment by Yura). However, if MapIter's methods would have signatures

Key(v Value)

Value(v Value)

we could create *one* value of the key/value type. The methods would then call `v.Set` to copy the non-addressable internal value into the addressable iteration variables. This would be closer to the for loop - we'd still (maybe) need to heap-allocate the iteration variables, but that would only have to happen once per loop, not once per iteration.

Disclaimer: I haven't verified if `MapIter.{Key,Value}` actually do allocate on every call, that's just how I read the comment. It's possible escape analysis is already clever enough for this (though I don't know how it would be).

 

  Keeping an address for them is very deeply unsafe, and
could break irretrievably with future changes to the garbage
collector.  Or, to put it another way, supporting this functionality
imposes limitations on the garbage collector that I don't think we
want to impose.  I'm not sure what we can do here.  Go's map type
intentionally does not give programs complete control over memory. 

It's possible that generics will address some of these use cases.

Ian

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CAOyqgcWzLyD0gn9r25M6HTGoXgUJEwXz-y2qVX4C4%3DGXp8mouQ%40mail.gmail.com.

[Axel Wagner]

PS: I was curious, and https://play.golang.org/p/oLnRnuE3rQ_W (awkwardly) does seem to indicate that the current reflect map iterator indeed allocates on every iteration. Because the number of allocations goes up if the number of keys goes up. Of course I can't compare it to my suggested alternative reflect API without implementing that :)

[Ian Lance Taylor]

On Mon, Mar 8, 2021 at 1:26 PM Axel Wagner
<axel.wa...@googlemail.com> wrote:
>
> On Mon, Mar 8, 2021 at 10:07 PM Ian Lance Taylor <ia...@golang.org> wrote:
>>
>> > 2. When I need to iterate map.
>> > Using standard reflect package there is no way to iterate map[string]int without allocation of string header and int for every key-value pair. When
>> > key or values is struct, things become worse.
>>
>> That is how the language works also. Map keys and values are not
>> addressable.
>
>
> I'm not sure I fully understand the issue, but
> for k, v := range m
> only needs to allocate storage for k/v *once*. AIUI, `MapIter.Key` and `MapIter.Value` need to allocate on every iteration (or at least that's how I read the comment by Yura). However, if MapIter's methods would have signatures
> Key(v Value)
> Value(v Value)
> we could create *one* value of the key/value type. The methods would then call `v.Set` to copy the non-addressable internal value into the addressable iteration variables. This would be closer to the for loop - we'd still (maybe) need to heap-allocate the iteration variables, but that would only have to happen once per loop, not once per iteration.

That seems possible, and would help part of the problem. Thanks.

Ian

[Yan Titarenko]

On Mon, Mar 8, 2021 at 11:06 PM Ian Lance Taylor <ia...@golang.org> wrote:

On Wed, Mar 3, 2021 at 7:02 PM Sokolov Yura <funny....@gmail.com> wrote:
Thanks.  I filed https://golang.org/issue/44870 for this.

Cool, well done.

I shall share that with some potentially interested engineers.

 

> 2. When I need to iterate map.
> Using standard reflect package there is no way to iterate map[string]int without allocation of string header and int for every key-value pair. When
> key or values is struct, things become worse.

That is how the language works also.  Map keys and values are not
addressable.  Keeping an address for them is very deeply unsafe, and
could break irretrievably with future changes to the garbage
collector.  Or, to put it another way, supporting this functionality
imposes limitations on the garbage collector that I don't think we
want to impose.  I'm not sure what we can do here.  Go's map type
intentionally does not give programs complete control over memory.

Hm, would take some time to review the vision.

 

It's possible that generics will address some of these use cases.

Ackdowledged.

Yan

 

Ian

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/CAOyqgcWzLyD0gn9r25M6HTGoXgUJEwXz-y2qVX4C4%3DGXp8mouQ%40mail.gmail.com.