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

x/sys/unix: add ParseOneSocketControlMessage #54714

Closed
marten-seemann opened this issue Aug 28, 2022 · 11 comments
Closed

x/sys/unix: add ParseOneSocketControlMessage #54714

marten-seemann opened this issue Aug 28, 2022 · 11 comments
Assignees
@marten-seemann
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Milestone
Unreleased

Comments

@marten-seemann
Copy link
Contributor

marten-seemann commented Aug 28, 2022 •
edited by rsc

Note, Oct 12 2022 Current proposal is #54714 (comment)

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

go version go1.19 linux/arm64

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="arm64"
GOBIN=""
GOCACHE="/home/parallels/.cache/go-build"
GOENV="/home/parallels/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/parallels/src/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/parallels/src/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/home/parallels/bin/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/parallels/bin/go/pkg/tool/linux_arm64"
GOVCS=""
GOVERSION="go1.19"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/parallels/src/go/src/golang.org/x/sys/go.mod"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build1413276320=/tmp/go-build -gno-record-gcc-switches"

What did you do?

In quic-go, we read control messages to read the ECN bits from the IP header, and to read the network interface index.
We parse the OOB bytes using unix.ParseSocketControlMessage. See https://github.com/lucas-clemente/quic-go/blob/07412be8a02ef0e55580ebf8db9c38a759c0a0e5/sys_conn_oob.go#L166-L206 for the respective control message processing logic.

What did you expect to see?

Ideally, unix.ParseSocketControlMessage would not allocate.

What did you see instead?

Receiving 1 GB of data using QUIC creates a huge amount of allocations (as determined using the allocs function of pprof). Roughly 100 MB of those allocations come from unix.ParseSocketControlMessage.

A simple back-of-the-envelope shows that this is roughly what we'd expect:
A data transfer of 1 GB requires receiving roughly 860,000 received packets, assuming a payload size of 1250 bytes per QUIC packet.
The []SocketControlMessage slice allocates 24 bytes, and the size of each control message is 40 bytes. There are two control messages per packet (unix.IP_TOS and unix.IP_PKTINFO). Parsing the control messages therefore allocates of 104 bytes per packet, or 89 MB in total for the 1 GB transfer.

Proposal

It would be nice to have an API that allows parsing socket control message that doesn't allocate at all.

The following API would fulfill that property:

func ParseSocketControlMessageWithHandler(b []byte, handler func(message SocketControlMessage)) error

UPDATE: Submitted a fix https://go-review.googlesource.com/c/sys/+/425916.
@gopherbot gopherbot added this to the Unreleased milestone Aug 28, 2022
@gopherbot
Copy link

Change https://go.dev/cl/425916 mentions this issue: unix: add ParseSocketControlMessageWithHandler to parse control messages without allocating

@marten-seemann marten-seemann mentioned this issue Aug 28, 2022
Closed
8 tasks
@seankhliao seankhliao changed the title x/sys: unix.ParseSocketControlMessage allocates and creates large amounts of garbage proposal: x/sys/unix: ParseSocketControlMessageWithHandler to not allocate Aug 28, 2022
@seankhliao seankhliao modified the milestones: Unreleased, Proposal Aug 28, 2022
@ianlancetaylor
Copy link
Contributor

Another approach would be

// ParseOneSocketControlMessage parses a single a single socket control message from b, returning the message header,
// message data (a slice of b), and the remainder of b after that single message. When there are no remaining messages,
// len(remainder) == 0.
func ParseOneSocketControlMessage(b []byte) (hdr Cmsghdr, data []byte, remainder []byte, err error)

@gopherbot
Copy link

Change https://go.dev/cl/425917 mentions this issue: unix: add ParseOneSocketControlMessage to parse control messages without allocating

@marten-seemann
Copy link
Contributor Author

@ianlancetaylor I like your proposal, it's even quite a bit faster in my benchmark:

BenchmarkParseControlMessage-6              	173010147	        34.84 ns/op	      48 B/op	       1 allocs/op
BenchmarkParseControlMessageWithHandler-6   	1000000000	         5.823 ns/op	       0 B/op	       0 allocs/op
BenchmarkParseOneControlMessage-6           	1000000000	         3.436 ns/op	       0 B/op	       0 allocs/op

I submitted another CL with an implementation: https://go-review.googlesource.com/c/sys/+/425917.

@marten-seemann
Copy link
Contributor Author

Is there anything else needed to get https://go-review.googlesource.com/c/sys/+/425917 merged, @ianlancetaylor?

@ianlancetaylor
Copy link
Contributor

The proposal needs to be reviewed and approved.

@marten-seemann
Copy link
Contributor Author

@ianlancetaylor Is there any timeline for review and approval of this proposal? https://go-review.googlesource.com/c/sys/+/425917 should be ready to go.

@ianlancetaylor
Copy link
Contributor

There are many proposals, with no particular timeline. You can follow the proposals being reviewed at https://go.dev/issue/33502.

@rsc rsc changed the title proposal: x/sys/unix: ParseSocketControlMessageWithHandler to not allocate proposal: x/sys/unix: add ParseOneSocketControlMessage Oct 12, 2022
@rsc
Copy link
Contributor

rsc commented Oct 12, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Oct 26, 2022

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Nov 2, 2022

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: x/sys/unix: add ParseOneSocketControlMessage x/sys/unix: add ParseOneSocketControlMessage Nov 2, 2022
@rsc rsc modified the milestones: Proposal, Backlog Nov 2, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Nov 2, 2022
@dmitshur dmitshur modified the milestones: Backlog, Unreleased Jun 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@marten-seemann marten-seemann

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Proposal Proposal-Accepted Proposal-FinalCommentPeriod
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
6 participants
@rsc @marten-seemann @dmitshur @ianlancetaylor @gopherbot @seankhliao