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

sysenc: dealing with []struct{ ... } forces allocations #1255

Open
lmb opened this issue Dec 5, 2023 · 0 comments
Open

sysenc: dealing with []struct{ ... } forces allocations #1255

lmb opened this issue Dec 5, 2023 · 0 comments
Assignees
Labels
help wanted Extra attention is needed

Comments

@lmb
Copy link
Collaborator

lmb commented Dec 5, 2023

We currently inherit an allocation problem from encoding/binary: binary.Size doesn't cache it's result for slices of structs. This means that our zero-alloc code path via sysenc isn't zero-alloc, since we end up hitting an allocation in reflect.

A benchmark to illustrate:

$ go test -run XX -bench 'BenchmarkUnmarshal/sysenc\.explicitPad' ./internal/sysenc/
goos: linux
goarch: amd64
pkg: github.com/cilium/ebpf/internal/sysenc
cpu: 12th Gen Intel(R) Core(TM) i7-1260P
BenchmarkUnmarshal/*sysenc.explicitPad-16         	25056632	       44.09 ns/op	      0 B/op	      0 allocs/op
BenchmarkUnmarshal/[]sysenc.explicitPad-16        	18953799	       64.01 ns/op	      8 B/op	      1 allocs/op
BenchmarkUnmarshal/[]sysenc.explicitPad#01-16     	18458588	       65.54 ns/op	      8 B/op	      1 allocs/op
BenchmarkUnmarshal/[]sysenc.explicitPad#02-16     	18186916	       67.05 ns/op	      8 B/op	      1 allocs/op
PASS
ok  	github.com/cilium/ebpf/internal/sysenc	5.075s

Going from a single explicitPad to a slice of them causes an allocation. The allocation happens in binary.dataSize:

Total: 152MB
ROUTINE ======================== encoding/binary.Size in /usr/local/go/src/encoding/binary/binary.go
         0      152MB (flat, cum)   100% of Total
         .          .    466:func Size(v any) int {
         .      152MB    467:	return dataSize(reflect.Indirect(reflect.ValueOf(v)))
         .          .    468:}
         .          .    469:
         .          .    470:var structSize sync.Map // map[reflect.Type]int
         .          .    471:
         .          .    472:// dataSize returns the number of bytes the actual data represented by v occupies in memory.
ROUTINE ======================== encoding/binary.dataSize in /usr/local/go/src/encoding/binary/binary.go
         0      152MB (flat, cum)   100% of Total
         .          .    476:func dataSize(v reflect.Value) int {
         .          .    477:	switch v.Kind() {
         .          .    478:	case reflect.Slice:
         .      152MB    479:		if s := sizeof(v.Type().Elem()); s >= 0 {
         .          .    480:			return s * v.Len()
         .          .    481:		}
         .          .    482:
         .          .    483:	case reflect.Struct:
         .          .    484:		t := v.Type()

In the past I've fixed the same problem for plain structs using a cache: golang/go@c9d89f6 I didn't take slices of structs into account since they are kind of esoteric. Turns out we now need to encode those when doing zero-alloc batch lookups, see #1254

One option is to fix this upstream, but it'll take a long time for this to reach us. I think we should fix upstream and in addition copy the implementation of binary.Size and fix the problem in our copy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants