go icon indicating copy to clipboard operation
go copied to clipboard

encoding/binary: should have minimal dependency on reflect

Open dsnet opened this issue 2 years ago • 9 comments

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.

dsnet avatar Jul 27 '22 21:07 dsnet

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 avatar Jul 27 '22 22:07 cherrymui

cc @griesemer

cherrymui avatar Jul 27 '22 22:07 cherrymui

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.

dsnet avatar Jul 27 '22 22:07 dsnet

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

gopherbot avatar Jul 28 '22 06:07 gopherbot

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

beoran avatar Sep 19 '22 00:09 beoran

@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 https://github.com/golang/go/issues/42958#issuecomment-751428308

dsnet avatar Sep 19 '22 00:09 dsnet

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

beoran avatar Sep 19 '22 00:09 beoran

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.

earthboundkid avatar Sep 19 '22 01:09 earthboundkid

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

  • The transitive dependency on "errors" links in "reflectlite" (see #54341)
  • It has a dependency on "unicode", which links in the massive Unicode tables (related to #54098)

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 avatar Sep 20 '22 22:09 dsnet

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

gopherbot avatar Nov 19 '22 20:11 gopherbot

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

gopherbot avatar May 05 '24 15:05 gopherbot

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

gopherbot avatar May 11 '24 09:05 gopherbot