golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.72k stars 17.62k forks source link

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

Open dsnet opened 2 years ago

dsnet commented 2 years ago

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

As a breakdown of all binary usages:

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 commented 2 years ago

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 commented 2 years ago

cc @griesemer

dsnet commented 2 years ago

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 commented 2 years ago

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

beoran commented 2 years ago

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

dsnet commented 2 years ago

@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

beoran commented 2 years ago

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

earthboundkid commented 2 years ago

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 commented 2 years ago

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 commented 1 year ago

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

gopherbot commented 5 months ago

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

gopherbot commented 5 months ago

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

gopherbot commented 7 hours ago

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