Improve reflection #1727
Merged
Improve reflection #1727
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This might be interesting for running Vecty on TinyGo webassembly (#1282, hexops/vecty/issues/269) CC @slimsag @marwan-at-work. |
|
@johanbrandhorst it'll probably help with Vecty support, but I think support won't be complete yet. For example, Things I'd like to work on after this PR is merged:
|
|
@aykevl thank you and cannot wait for TinyGo to support Vecty :) |
aykevl
force-pushed
the
reflect-type-itf
branch
from
cfc07f1
to
9fb5849
Compare
|
Rebased after #1612 was merged. |
aykevl
force-pushed
the
reflect-type-itf
branch
from
9fb5849
to
77210cc
Compare
niaow
requested changes
Mar 23, 2021
This distinction was useful before when reflect wasn't properly supported. Back then it made sense to only include method sets that were actually used in an interface. But now that it is possible to get to other values (for example, by extracting fields from structs) and it is possible to turn them back into interfaces, it is necessary to preserve all method sets that can possibly be used in the program in a type assert, interface assert or interface method call. In the future, this logic will need to be revisited again when reflect.New or reflect.Zero gets implemented. Code size increases a bit in some cases, but usually in a very limited way (except for one outlier in the drivers smoke tests). The next commit will improve the situation significantly.
Previously there was code to avoid impossible type asserts but it wasn't
great and in fact was too aggressive when combined with reflection.
This commit improves this by checking all types that exist in the
program that may appear in an interface (even struct fields and the
like) but without creating runtime.typecodeID objects with the type
assert. This has two advantages:
* As mentioned, it optimizes impossible type asserts away.
* It allows methods on types that were only asserted on (in
runtime.typeAssert) but never used in an interface to be optimized
away using GlobalDCE. This may have a cascading effect so that other
parts of the code can be further optimized.
This sometimes massively improves code size and mostly negates the code
size regression of the previous commit.
The errors package has a call like this in the package initializer. This commit adds support for running it at compile time, avoiding the call at runtime. This doesn't always help (the call is already optimized away in many small programs) but it does help to shave off some binary size in larger programs. Perhaps more importantly, it will avoid a penalty in code size when the reflect package will convert reflect.Type from a regular type to an interface type.
This is necessary so that when reflect.Type is converted from a concrete type to an interface type, the errors package can still be interpreted. Without this change, basically every program would grow in size by a few bytes.
This matches the main Go implementation and (among others) fixes a compatibility issue with the encoding/json package. The encoding/json package compares reflect.Type variables against nil, which does not work as long as reflect.Type is of integer type. This also adds a reflect.RawType() function (like reflect.Type()) that makes it easier to avoid working with interfaces in the runtime package. It is internal only, but exported to let the runtime package use it. This change introduces a small code size increase when working with the reflect package, but I've tried to keep it to a minimum. Most programs that don't make extensive use of the reflect package (and don't use package like fmt) should not be impacted by this.
aykevl
force-pushed
the
reflect-type-itf
branch
from
77210cc
to
9ad9da2
Compare
|
Updated with suggestion from @niaow and rebased on the dev branch to fix a merge conflict. |
|
This is very exciting PR right here. 🔥 Now merging. |
|
@deadprogram @aykevl thank you both ❤️ Is it worth just waiting until reflect.New is implemented before thoroughly testing Vecty? Thanks! |
|
@marwan-at-work not sure, I haven't really done anything with Vecty yet. You can of course try it. |
5 tasks
This pull request was closed.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a big PR (based on #1612) that improves reflect support significantly:
compiler: merge runtime.typecodeID and runtime.typeInInterface: this commit makes reflect support more correct, see testdata/reflect.go for a case where it behaved incorrectly before. It increases code size in many cases.compiler: do not check for impossible type asserts: reduces the code size in many cases introduced in the previous commit.interp: handle (reflect.Type).Elem(): prepare for the last commit. Shaves off a few bytes in many cases.interp: add support for runtime.interfaceMethod: also prepares for the last commit.reflect: let reflect.Type be of interface type: this is the big one. The encoding/json package relies on being able to comparereflect.Typevariables with nil, which this commit makes possible by changingreflect.Typeto an interface type (like standard Go). All the previous commits are needed to reduce the associated code size increase. There is still some code size increase, but it's only for some bigger programs (for example, that use the fmt package).One side effect of this PR (in commit 2) is that it improves (but does not fix) issue #1415 when you're printing
errorvalues (e.g.err := ...; println(err)).This is a draft until #1612 is merged. I think it would be possible to separate this PR from #1612 but that might make the first commit more complicated (it was causing linker errors until I rebased on top of #1612).