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

net: UDPConn.WriteTo and UDPConn.ReadFromUDP both allocate #43451

Open
josharian opened this issue Dec 31, 2020 · 10 comments
Open

net: UDPConn.WriteTo and UDPConn.ReadFromUDP both allocate #43451

josharian opened this issue Dec 31, 2020 · 10 comments
Milestone

Comments

@josharian
Copy link
Contributor

@josharian josharian commented Dec 31, 2020

What version of Go are you using (go version)?

$ go version
go version devel +95ce805d14 Thu Dec 31 02:24:55 2020 +0000 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN="/Users/josh/bin"
GOCACHE="/Users/josh/Library/Caches/go-build"
GOENV="/Users/josh/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/josh/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/josh"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/Users/josh/go/tip"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/Users/josh/go/tip/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="devel +95ce805d14 Thu Dec 31 02:24:55 2020 +0000"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/Users/josh/go/tip/src/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/1t/n61cbvls5bl293bbb0zyypqw0000gn/T/go-build2869058686=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

I'd like to be able to write a program that uses UDPConn.WriteTo and UDPConn.ReadFromUDP without allocating per-packet.

This benchmark indicates one alloc per WriteTo and two allocs per ReadFromUDP.

func BenchmarkWriteToReadFromUDP(b *testing.B) {
	conn, err := ListenUDP("udp4", new(UDPAddr))
	if err != nil {
		b.Fatal(err)
	}
	addr := conn.LocalAddr()
	buf := make([]byte, 8)
	b.ResetTimer()
	b.ReportAllocs()
	for i := 0; i < b.N; i++ {
		_, err := conn.WriteTo(buf, addr)
		if err != nil {
			b.Fatal(err)
		}
		_, _, err = conn.ReadFromUDP(buf)
		if err != nil {
			b.Fatal(err)
		}
	}
}

Two of the allocs come from constructing syscall.Sockaddrs. Maybe this is fixable, but I don't see an easy way.

The last alloc is from constructing a *UDPAddr to return from ReadFromUDP. I fear the API may make this one unavoidable.

cc @bradfitz @danderson @zx2c4

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Dec 31, 2020

I have a vague impression of pointing this out to @FiloSottile 2 years ago but I don't remember the conclusion of our conversation. CCing in case he has a better recollection.

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Dec 31, 2020

I found some old notes. The conclusion from last I looked into this was that the API made it unavoidable. As a result I wound up making direct syscalls on Linux but didn't port that to all platforms.

I wonder if this warrants adding a new API. ReadFromUDPWithPrealocatedSockaddr() or something...

@gopherbot
Copy link

@gopherbot gopherbot commented Jan 1, 2021

Change https://golang.org/cl/280934 mentions this issue: syscall: cache and re-use inet sockaddrs in anyToSockaddr

@josharian
Copy link
Contributor Author

@josharian josharian commented Jan 1, 2021

IIUC, most sockaddrs will be re-used and they are never mutated. Given that, we could intern them. E.g. we could use a sync.Pool of maps to opportunistically re-use them. (This is the technique that https://github.com/josharian/intern uses for strings; it is fast and pretty good, but not perfect. There are other options, with their own trade-offs, like go4.org/intern.)

I threw together CL 280934 to illustrate using the sync.Pool of maps approach.

@josharian
Copy link
Contributor Author

@josharian josharian commented Jan 1, 2021

Ugh. Nope, that is not safe. We end up exposing the sockaddr memory to the caller in this line (udpsock_posix.go:50):

addr = &UDPAddr{IP: sa.Addr[0:], Port: sa.Port}

Avoiding that would require making a copy of the sa.Addr bytes to serve as the new net.IP. It'd be an allocation of only 4 bytes, which is better than the 32 bytes interning the sockaddr saves, but it's still an allocation.

We might still be able to intern the sockaddrs that are destined for the kernel at least.

If we (a) switched to inet.af/netaddr's IP type and (b) returned a UDPAddr instead of a *UDPAddr, that'd eliminate the non-sockaddr allocations. But (b) requires new API, and (a) is Go 2 material.

We could do both by letting people provide their own *UDPAddr to be filled in, with a preallocated net.IP that could be overwritten...but we already have too many ways to receive a UDP packet. We don't need another one. (At least not in the standard library. Maybe we could set up some of this in golang.org/x/net somehow?)

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Jan 1, 2021

Evil idea: if the buf passed in is pre-allocated memory that's larger than what's needed for the data, and can fit the sockaddr and returned udpaddr and ip, could we stuff that all in at the end of buf?

@josharian
Copy link
Contributor Author

@josharian josharian commented Jan 1, 2021

Hyrum's Law says no. (Plus the Go standard library generally doesn't go for such evil tricks, entertaining though they be.)

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Jan 1, 2021

Maybe a variant of that might be acceptable:

Right now people pass in a buffer of the maximum size of data they want:

data := make([]byte, 1472)
n, addr, err := conn.ReadFromUDP(data)
data = data[:n]

My initial idea was to place the sockaddr allocations in the region of data[n:...] that doesn't wind up getting used. You cited Hyrum.

It occurred to me that other Go APIs sometimes work by taking slice to append to and then return a new slice. The reasoning goes that the caller can preallocate by allocating a slice with a large capacity but a zero length, and then the appending operation is free. What if we use a related trick here:

data := make([]byte, 1472, 2000)
n, addr, err := conn.ReadFromUDP(data)
data = data[:n]

In this instance, rather than placing addr at data[n:...], it's placed after the 1472 bytes by using append -- the region data[len(data):...], which does not need to allocate for another 528 bytes, because the capacity has been preallocated. So we avoid the allocation by putting the sockaddr there.

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Jan 1, 2021

Mmm, looks like that can still lead to unexpected problems:

package main

import (
	"fmt"
)

func doTheAliasingTrick(slice []byte) *byte {
	for i := range slice {
		slice[i] = 41
	}
	return &append(slice, 42)[len(slice)]
}

func main() {
	data := make([]byte, 1472, 2000)
	x := doTheAliasingTrick(data)
	
	fmt.Printf("*x = %d\n", *x) // Prints 42
	_ = append(data, 43)
	fmt.Printf("*x = %d\n", *x) // Prints 43
}
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jan 5, 2021

Avoiding that would require making a copy of the sa.Addr bytes to serve as the new net.IP. It'd be an allocation of only 4 bytes, which is better than the 32 bytes interning the sockaddr saves, but it's still an allocation.

I think you can outline that. It will require some care on the caller side, but if someone needs it they can make sure they don't get in the way of the inliner.

@toothrot toothrot added this to the Backlog milestone Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.