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

Jump to bottom

net/netip: Addr.AsSlice should be inlineable #56136

Open
dsnet opened this issue Oct 10, 2022 · 8 comments
Open

net/netip: Addr.AsSlice should be inlineable #56136

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

Comments

@dsnet
Copy link
Member

dsnet commented Oct 10, 2022 •
edited

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

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

I can't find it anymore.

@mateusz834
Copy link
Member

mateusz834 commented Oct 11, 2022 •
edited

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
Copy link
Member

mateusz834 commented Oct 11, 2022 •
edited

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
Copy link

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
Copy link
Member

@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
Copy link
Member Author

dsnet commented Oct 11, 2022

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

@mateusz834
Copy link
Member

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

@renthraysk
Copy link

renthraysk commented Oct 13, 2022 •
edited

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

@joedian joedian added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Oct 14, 2022
@seankhliao seankhliao added this to the Unplanned milestone Nov 19, 2022
@kevinGC kevinGC mentioned this issue Jun 15, 2023
Closed
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
Unplanned
Development

No branches or pull requests
6 participants
@OneOfOne @dsnet @seankhliao @joedian @mateusz834 @renthraysk