Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: reflect: treat reflect.Type as a pointer where possible for generics & maps #52267

Closed
twmb opened this issue Apr 10, 2022 · 9 comments

Comments

@twmb
Copy link
Contributor

twmb commented Apr 10, 2022

reflect.Type cannot be used in any comparable generic code. As well, using reflect.Type as a key in maps compiles to ifaceeq when it could be compiled to pointer / number comparisons.

Because reflect.Type has two internal methods, there cannot be any implementation outside of the reflect package. Inside reflect, Type is always *rtype which corresponds to a runtime type.

Can we solve #51179 by specially compiling reflect.Type to its *rtype representation, which makes it comparable? As well, can this force the discussion on #32303 by committing to "No, custom type implementations are not supported"?

Lastly, storing pointers in maps rather than reflect.Type can help speed up essentially any serializer that caches some computation of reflect.Type => {reflect.StructField}. Notably, there is a map[reflect.Type]something in encoding/binary, encoding/gob, encoding/json, and encoding/xml. I noticed this myself when writing a serializer for avro (I know others already exist). For the small toy struct that is commonly used in avro tests, serialization speeds up from 350ns to 300ns. In benchmarks, looking up reflect.Type is ~12ns, vs. ~2ns for unsafe.Pointer -- it's a small increase, but it does add up, especially when using arrays.

@twmb twmb added the Proposal label Apr 10, 2022
@gopherbot gopherbot added this to the Proposal milestone Apr 10, 2022
@uluyol
Copy link
Contributor

uluyol commented Apr 10, 2022

It is possible to wrap a reflect.Type and override methods.

@twmb
Copy link
Contributor Author

twmb commented Apr 10, 2022

Interesting wrinkle in my hopes, I think wrappers may make any intrinsic "this is *rtype a bit muddled. Fundamentally a wrapped reflect.Type is still an *rtype, but any added fields mean we aren't comparing two *rtypes anymore.

The other angle I was thinking to propose: is it worth considering adding something like reflect.UnsafeTypePointer to receive the underlying type pointer?

@ianlancetaylor ianlancetaylor changed the title proposal: reflect & cmd/compile: treat reflect.Type as a pointer where possible for generics & maps proposal: reflect: treat reflect.Type as a pointer where possible for generics & maps Apr 11, 2022
@ianlancetaylor
Copy link
Member

ianlancetaylor commented Apr 11, 2022

My personal preference here is to adopt #51338 and change reflect.Type to embed comparable. Then no special handling is required.

@randall77

This comment was marked as resolved.

@ianlancetaylor

This comment was marked as resolved.

@twmb
Copy link
Contributor Author

twmb commented Apr 12, 2022

#51338 is great and largely covers it, albeit @Merovius brought up the same wrinkle. I think #51338 is a better general approach, whereas this issue would be an optimization regardless of what's chosen. If the wrinkle cannot be solved, is it worth it to consider an alternative proposal of extracting the *rtype as a unsafe.Pointer somehow?

@rsc
Copy link
Contributor

rsc commented Apr 13, 2022

It sounds like we should decline this in favor of #51338.

@rsc
Copy link
Contributor

rsc commented Apr 13, 2022

Based on the discussion above, this proposal seems like a likely decline.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented May 4, 2022

No change in consensus, so declined.
— rsc for the proposal review group

@rsc rsc closed this as completed May 4, 2022
@rsc rsc moved this to Declined in Proposals Aug 10, 2022
@rsc rsc added this to Proposals Aug 10, 2022
@golang golang locked and limited conversation to collaborators May 4, 2023
@rsc rsc removed this from Proposals May 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants