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

errors: should have minimal dependency on reflectlite #54341

Open
dsnet opened this issue Aug 8, 2022 · 6 comments
Open

errors: should have minimal dependency on reflectlite #54341

dsnet opened this issue Aug 8, 2022 · 6 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@dsnet
Copy link
Member

dsnet commented Aug 8, 2022

On go1.19, A blank import of "errors" increases the binary size by ~15.5KiB due to the transitive dependency on reflectlite.

The hard dependency occurs because of the program initialize variable for:

var errorType = reflectlite.TypeOf((*error)(nil)).Elem()

We could consider a trick similar to 419757

This is relevant since many packages depend on errors only for errors.New, and we should not expect those packages to force reflectlite to be transitively linked in.

@dsnet
Copy link
Member Author

dsnet commented Aug 8, 2022

I also wonder if programs that depend on both reflect and reflectlite could just end up using the reflect package instead and save that ~15.5KiB. I don't know how to implement that without some linker magic.

@cherrymui
Copy link
Member

Perhaps the reflect package can can just import reflectlite and build on top of it, instead of duplicating code. (The reflectlite package may need to export more things for reflect to use, and that is fine, as it is an internal package.)

It would look nicer if the reflect package is self-contained. But probably okay if it is not.

@dsnet
Copy link
Member Author

dsnet commented Aug 8, 2022

It would look nicer if the reflect package is self-contained. But probably okay if it is not.

For "look nicer", are you referring to how the package appears in pkgsite? Implementing "reflect" in terms of "reflectlite" will probably require the primary types to be declared in reflectlite with type aliases left in "reflect". For an unrelated package split that I was doing, @mvdan mentioned that we might want to teach the pkgsite certain patterns where it hoists a type alias as if it were declared locally (in terms of presentation).

@cherrymui
Copy link
Member

cherrymui commented Aug 8, 2022

I was mostly thinking about reading the code. It would be nicer if the code stays together.

For documentation, reflectlite doesn't define many types for reflect to export, so it is probably not too bad. We probably could alias Kind and Type, or duplicate them, which shouldn't be too bad. I don't think we can alias Value as we need to define more methods on reflect.Value. reflect.Value could probably wrap reflectlite.Value.

@thanm thanm added this to the Backlog milestone Aug 9, 2022
@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 9, 2022
@qiulaidongfeng
Copy link
Contributor

The issue has been settled
go build the following code, with or without import errors, is 1.4M in size

package main

import _ "errors"

func main() {

}

@qiulaidongfeng
Copy link
Contributor

See #54341 (comment) , I think this issue can be closed.
@dsnet Could you please close this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

4 participants