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.

Sign up for GitHub

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

Jump to bottom

encoding/binary: should have minimal dependency on reflect #54097

Open
dsnet opened this issue Jul 27, 2022 · 10 comments
Open

encoding/binary: should have minimal dependency on reflect #54097

dsnet opened this issue Jul 27, 2022 · 10 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Milestone
Backlog

Comments

@dsnet
Copy link
Member

dsnet commented Jul 27, 2022

A vast majority of binary package usages is only for BigEndian and LittleEndian.

As a breakdown of all binary usages:

  • 75% is for endian-based operations (does not depend on reflection)
  • 23% is for Read/Write operations (depends on reflection)
  • 2% is for varint operations (does not depend on reflection)

For the 77% of use cases where the logic does not touch binary.{Read,Write,Size}, the resulting binary should not be forced to also link in the reflect package.
@cherrymui
Copy link
Member

Could you explain more about the problem of importing the reflect package? What functions/variables, if not used, cannot be pruned by the linker? Thanks.

@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jul 27, 2022
@cherrymui cherrymui added this to the Backlog milestone Jul 27, 2022
@cherrymui
Copy link
Member

cc @griesemer

@dsnet
Copy link
Member Author

dsnet commented Jul 27, 2022 •
edited

The following is a list of declarations that appear in the binary for a blank import of "encoding/binary":

reflect.resolveNameOff
reflect.resolveTypeOff
reflect.name.name
reflect.name.readVarint
reflect.name.data
reflect.add
reflect.Kind.String
reflect.(*rtype).uncommon
reflect.(*rtype).Kind
reflect.(*rtype).String
reflect.(*rtype).nameOff
reflect.(*rtype).exportedMethods
reflect.(*uncommonType).exportedMethods
reflect.ChanDir.String
reflect.(*ValueError).Error
reflect.Value.String
reflect.flag.kind
reflect.Value.Type
reflect.(*rtype).typeOff
reflect.init
reflect.TypeOf
reflect.toType
reflect.(*ChanDir).String
reflect.(*Kind).String
type..eq.reflect.uncommonType
reflect.(*Value).String
type..eq.reflect.Method
type..eq.reflect.ValueError
reflect.resolveNameOff
reflect.resolveTypeOff
reflect.name.name
reflect.Kind.String
reflect.(*rtype).uncommon
reflect.(*rtype).String
reflect.(*rtype).exportedMethods
reflect.ChanDir.String
reflect.(*ValueError).Error
reflect.Value.String
reflect.Value.Type
reflect.init
reflect.(*ChanDir).String
reflect.(*Kind).String
type..eq.reflect.uncommonType
reflect.(*Value).String
type..eq.reflect.Method
type..eq.reflect.ValueError
reflect..inittask
reflect.kindNames
reflect.uint8Type
reflect.stringType

I didn't yet peek into the "reflect" package why a blank import of it results in anything being linked in.

@gopherbot
Copy link

Change https://go.dev/cl/419757 mentions this issue: reflect: avoid TypeOf in init

gopherbot pushed a commit that referenced this issue Aug 8, 2022
@dsnet
reflect: avoid TypeOf in init
ebf182c 
Calling TypeOf to initialize variables forces any import of "reflect"
to link in the declared types of "reflect" even if they are unused.
TypeOf operates on Type and which will pull in
all transitive dependencies of Type, which includes Value as well.
Avoid this problem by declaring a rtypeOf function that
directly extracts the *rtype from an interface value
without going through Type as an intermediate type.

For a program that blank imports "reflect",
this reduces the binary size by ~34 KiB.

Updates #54097

Change-Id: I8dc7d8da8fedc48cc0dd842b69f510d17144827e
Reviewed-on: https://go-review.googlesource.com/c/go/+/419757
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
@dsnet @jproberts
reflect: avoid TypeOf in init
9762122 
Calling TypeOf to initialize variables forces any import of "reflect"
to link in the declared types of "reflect" even if they are unused.
TypeOf operates on Type and which will pull in
all transitive dependencies of Type, which includes Value as well.
Avoid this problem by declaring a rtypeOf function that
directly extracts the *rtype from an interface value
without going through Type as an intermediate type.

For a program that blank imports "reflect",
this reduces the binary size by ~34 KiB.

Updates golang#54097

Change-Id: I8dc7d8da8fedc48cc0dd842b69f510d17144827e
Reviewed-on: https://go-review.googlesource.com/c/go/+/419757
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Bryan Mills <bcmills@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@dsnet dsnet mentioned this issue Sep 16, 2022
Closed
@beoran
Copy link

beoran commented Sep 19, 2022

This seems to suggest that the endianness functions in this package should be split off in a separate package, perhaps named "encoding/endian".

@dsnet
Copy link
Member Author

dsnet commented Sep 19, 2022

@beoran: Perhaps. One possibility is to put such functionality in the math/bits package:

func LoadUint32LE(v [4]byte) uint32
func StoreUint32LE(v uint32) [4]byte

See #42958 (comment)

@beoran
Copy link

beoran commented Sep 19, 2022

@dsnet Yes, perhaps, although an API that is backwards compatible with encoding/binary would be easier to use.

@earthboundkid
Copy link
Contributor

Are there compiler optimizations (including escape analysis) that apply to func([4]byte) uint32 that can't be done to func([]byte) uint32 (or func(*[4]byte) uint32)? If so, that's a good enough reason to use it.

@dsnet
Copy link
Member Author

dsnet commented Sep 20, 2022 •
edited

After https://go.dev/cl/419757, a blank import of "reflect" bloats a binary primarily in two ways:

If those two were both addressed, a blank import of "reflect" only increases a binary size by ~9KiB (down from 157KiB when this issue was filed).

@dsnet dsnet mentioned this issue Oct 11, 2022
Open
@gopherbot
Copy link

Change https://go.dev/cl/452176 mentions this issue: compress/zlib: use binary.BigEndian consistently

gopherbot pushed a commit that referenced this issue Feb 27, 2023
@dsnet @gopherbot
compress/zlib: use binary.BigEndian consistently
9a199d4 
One major reason to avoid binary.BigEndian is because
the binary package includes a transitive dependency on reflect.
See #54097.

Given that writer.go already depends on the binary package,
embrace use of it consistently where sensible.
We should either embrace use of binary or fully avoid it.

Change-Id: I5f2d27d0ed8cab5ac54be02362c7d33276dd4b9a
Reviewed-on: https://go-review.googlesource.com/c/go/+/452176
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Run-TryBot: Joseph Tsai <joetsai@digital-static.net>
Auto-Submit: Joseph Tsai <joetsai@digital-static.net>
Reviewed-by: Ian Lance Taylor <iant@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Performance
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
5 participants
@beoran @earthboundkid @dsnet @gopherbot @cherrymui