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

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

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

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

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

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

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
Contributor

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
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
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>
@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

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).

@gopherbot
Copy link
Contributor

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
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>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/583298 mentions this issue: internal/binarylite: new package

gopherbot pushed a commit that referenced this issue May 10, 2024
Currently in a lot of packages we define functions for appending/decoding
mostly BigEndian data (see internal/chacha8rand, net/netip,
internal/boring/sha, hash/crc64, and probably more), because we don't
want to depend on encoding/binary, because of #54097.

This change introduces a new package internal/byteorder, that
will allow us to remove all of the functions and replace them with
internal/byteorder.

Updates #54097

Change-Id: I03e5ea1eb721dd98bdabdb25786f889cc5de54c5
GitHub-Last-Rev: 3f07d3d
GitHub-Pull-Request: #67183
Reviewed-on: https://go-review.googlesource.com/c/go/+/583298
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Commit-Queue: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/585017 mentions this issue: crypto: replace encoding/binary in favour of internal/byteorder

gopherbot pushed a commit that referenced this issue May 13, 2024
Updates #54097

Change-Id: I827a5efd1736ce057b76f079466f2d9ead225898
GitHub-Last-Rev: 40af104
GitHub-Pull-Request: #67321
Reviewed-on: https://go-review.googlesource.com/c/go/+/585017
Reviewed-by: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Auto-Submit: Keith Randall <khr@golang.org>
Commit-Queue: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/622497 mentions this issue: crypto/internal/hpke: use internal/byteorder instead of encoding/binary

gopherbot pushed a commit that referenced this issue Oct 28, 2024
crypto/internal/hpke is the only package under crypto that imports
encoding/binary. Other packages use internal/byteorder instead, which
notably doesn't depend on the reflect package.

Updates #54097

Change-Id: I77a3ac5f4588527a2f82574df4cb84d30630d73f
Reviewed-on: https://go-review.googlesource.com/c/go/+/622497
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Carlos Amedee <carlos@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
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. Performance
Projects
None yet
Development

No branches or pull requests

6 participants
@beoran @earthboundkid @dsnet @gopherbot @cherrymui and others