net/netip: Addr.AsSlice should be inlineable

[dsnet]

This method always allocates because it returns a slice that escapes to the return value.

If the method were inlineable, the compiler may prove in some cases that the return value does not escape and can be stack allocated.

Currently, the method is a cost of 222; way over the budget of 80.

One attempt to optimize this was:

func (ip Addr) AsSlice() []byte {
	ret := ip.As16()
	switch ip.z {
	case z0:
		return nil
	case z4:
		return ret[12:]
	default:
		return ret[:]
	}
}

However, this is a function cost of 83. Just slightly over the budget.

[OneOfOne]

Wasn't there talks about upping the inliner's budget at one point? whatever happened to that?

I can't find it anymore.

[mateusz834]

I recently tried to remove this allocation in CL428855, but I abandoned it because, in case when the slice is forced to be heap allocated it caused an 16B allocation of ipv4 addresses instead of 4B.

[mateusz834]

I thought about it again and found a solution to make it non-allocating.

// AsSlice returns an IPv4 or IPv6 address in its respective 4-byte or 16-byte representation.
func (ip Addr) AsSlice() []byte {
	var ret []byte
	if ip.z == z4 {
		ret = make([]byte, 4)
	} else {
		ret = make([]byte, 16)
	}
	return ip.asSlice(ret)
}

func (ip Addr) asSlice(ret []byte) []byte {
	switch ip.z {
	case z0:
		return nil
	case z4:
		bePutUint32(ret[:], uint32(ip.addr.lo))
		return ret
	default:
		bePutUint64(ret[:8], ip.addr.hi)
		bePutUint64(ret[8:], ip.addr.lo)
		return ret
	}
}

This method will fix the issue from comment above and allocate only 4B when ipv4.

BenchmarkAsSliceAddrv4-4                352722526                3.386 ns/op           0 B/op          0 allocs/op
BenchmarkAsSliceAddrv6-4                281346915                4.296 ns/op           0 B/op          0 allocs/op
BenchmarkAsSliceAddrv4Escapes-4         62823386                23.58 ns/op            4 B/op          1 allocs/op
BenchmarkAsSliceAddrv6Escapes-4         28781724                51.70 ns/op           16 B/op          1 allocs/op

commit. If we want this I will push a CL.

[renthraysk]

Problem seems to lie with As16(). If importing binary pkg is an option, using binary.BigEndian.PutUint64() will considerably lower the cost.

func (ip Addr) As16() (a16 [16]byte) {
	binary.BigEndian.PutUint64(a16[:8], ip.addr.hi)
	binary.BigEndian.PutUint64(a16[8:], ip.addr.lo)
	return a16
}

// AsSlice returns an IPv4 or IPv6 address in its respective 4-byte or 16-byte representation.
func (ip Addr) AsSlice() []byte {
	ret := ip.As16()
	switch ip.z {
	case z0:
		return nil
	case z4:
		return ret[12:]
	}
	return ret[:]
}

Has a cost of 47.

./netip.go:708:6: can inline Addr.AsSlice with cost 47 as: method(Addr) func() []byte { ret := Addr.As16(ip); switch statement; return ret[:] }

[mateusz834]

@renthraysk I think we should do something like that, to only allocate 4B for ipv4.

// AsSlice returns an IPv4 or IPv6 address in its respective 4-byte or 16-byte representation.
func (ip Addr) AsSlice() []byte {
	switch ip.z {
	case z0:
		return nil
	case z4:
		buf := make([]byte, 4)
		binary.BigEndian.PutUint32(buf, uint32(ip.addr.lo))
		return buf
	}
	buf := make([]byte, 16)
	binary.BigEndian.PutUint64(buf[:8], ip.addr.hi)
	binary.BigEndian.PutUint64(buf[8:], ip.addr.lo)
	return buf
}

[dsnet]

We should avoid the dependency on the "binary" package unless we can resolve #54097.

[mateusz834]

@dsnet so #56136 (comment) seems to be best option for now.

[renthraysk]

Could take the package path check out of the inliner or replace it with a check the code is within the standard library, and then local package versions could benefit from the cheap cost.

go/src/cmd/compile/internal/inline/inl.go

Lines 315 to 323 in 4a0ce46

	if base.Ctxt.Arch.CanMergeLoads && s.Pkg.Path == "encoding/binary" {
	switch s.Name {
	case "littleEndian.Uint64", "littleEndian.Uint32", "littleEndian.Uint16",
	"bigEndian.Uint64", "bigEndian.Uint32", "bigEndian.Uint16",
	"littleEndian.PutUint64", "littleEndian.PutUint32", "littleEndian.PutUint16",
	"bigEndian.PutUint64", "bigEndian.PutUint32", "bigEndian.PutUint16",
	"littleEndian.AppendUint64", "littleEndian.AppendUint32", "littleEndian.AppendUint16",
	"bigEndian.AppendUint64", "bigEndian.AppendUint32", "bigEndian.AppendUint16":
	cheap = true

[gopherbot]

Change https://go.dev/cl/584995 mentions this issue: net/netip: use internal/byteorder