Repository: cilium/ebpf
Pull request #1657 (2025-01-24 16:36:33)

btf: read all line info records at once in `parseLineInfoRecords`
When parsing line info records, we currently iterate and call `binary.Read` once per line info record. This is quite inefficient and `binary.Read` already supports out of the box being called directly on a slice and reading multiple structures at once. This is exactly what this PR does.

Benchmark:
```
                        │   old.txt   │               new.txt               │
                        │   sec/op    │   sec/op     vs base                │
ParseLineInfoRecords-10   288.6µ ± 3%   141.3µ ± 4%  -51.02% (p=0.000 n=10)

                        │   old.txt    │            new.txt             │
                        │     B/op     │     B/op      vs base          │
ParseLineInfoRecords-10   128.0Ki ± 0%   128.0Ki ± 0%  ~ (p=1.000 n=10)

                        │    old.txt    │              new.txt               │
                        │   allocs/op   │ allocs/op   vs base                │
ParseLineInfoRecords-10   4098.000 ± 0%   3.000 ± 0%  -99.93% (p=0.000 n=10)
```

ti-mo: Cool! Drive-by: whenever you optimize old code that doesn't change frequently, make sure to bolt it down with `testing.AllocsPerRun`. :wink: 
paulcacheux: I added a small test for the allocations, I'm a bit sad about it because it checks against 3:
- as opposed to all other allocation tests in the repo that test against 0
- it feels a bit fragile, since it's susceptible to changes in the stdlib that would improve or degrade the amount of allocations of `binary.Read`
Otherwise I could do something like `< 10` or something like that, WDYT ?
dylandreimerink: > it feels a bit fragile, since it's susceptible to changes in the stdlib that would improve or degrade the amount of allocations of binary.Read

Yea, I would have to agree with that, seems fragile. I imagine it would be frustrating if we have to start updating magic values in out tests when we bump Go / stdlib versions. I don't know if a <10 version hits the mark, like if we go to 11, would that be a cause for us to reconsider something?

On the other hand, I understand the desire to curb allocations and memory use caused by the lib. Not sure if this is the best way to do so however.
ti-mo: Discussed this offline with Dylan. I agree on all arguments raised, but:

- we have a few of these tests in the code base, and so far they've been quiet
- let's assume that amount of allocs in stdlib functions shrinks over time instead of growing, given the amount of scrutiny on them
- these alloc measurements are enforced over small blocks of deterministic code without side effects
- we can always remove them if they cause too much noise (that would be on me :slightly_smiling_face:)

I think pinning it down to 7 allocs is the best we can do in absence of a continuous benchmarking setup or a solution to track allocs of certain spans of code over time. These alternatives are (much) more expensive to run and not maintenance-free by any means.

Also, since this is a wire format defined in a (sort of) spec that doesn't change on a whim, my tendency would be to write a custom marshaler for these things, but I've had trouble convincing Lorenz of this in the past. If we really need this to not allocate, it's still the way to go IMO.
paulcacheux: Ok, I can change the test to 7 for this PR. And work on a follow up PR with a real unmarshaller

Patch:
diff --git a/btf/ext_info.go b/btf/ext_info.go
index 2c684fe..71dbba8 100644
--- a/btf/ext_info.go
+++ b/btf/ext_info.go
@@ -666,20 +666,19 @@ func parseLineInfos(r io.Reader, bo binary.ByteOrder, strings *stringTable) (map
 // These records appear after a btf_ext_info_sec header in the line_info
 // sub-section of .BTF.ext.
 func parseLineInfoRecords(r io.Reader, bo binary.ByteOrder, recordSize uint32, recordNum uint32, offsetInBytes bool) ([]bpfLineInfo, error) {
-	var li bpfLineInfo
-
-	if exp, got := uint32(binary.Size(li)), recordSize; exp != got {
+	if exp, got := uint32(binary.Size(bpfLineInfo{})), recordSize; exp != got {
 		// BTF blob's record size is longer than we know how to parse.
 		return nil, fmt.Errorf("expected LineInfo record size %d, but BTF blob contains %d", exp, got)
 	}
 
-	out := make([]bpfLineInfo, 0, recordNum)
-	for i := uint32(0); i < recordNum; i++ {
-		if err := binary.Read(r, bo, &li); err != nil {
-			return nil, fmt.Errorf("can't read line info: %v", err)
-		}
+	out := make([]bpfLineInfo, recordNum)
+	if err := binary.Read(r, bo, out); err != nil {
+		return nil, fmt.Errorf("can't read line info: %v", err)
+	}
 
-		if offsetInBytes {
+	if offsetInBytes {
+		for i := range out {
+			li := &out[i]
 			if li.InsnOff%asm.InstructionSize != 0 {
 				return nil, fmt.Errorf("offset %v is not aligned with instruction size", li.InsnOff)
 			}
@@ -688,8 +687,6 @@ func parseLineInfoRecords(r io.Reader, bo binary.ByteOrder, recordSize uint32, r
 			// Convert as early as possible.
 			li.InsnOff /= asm.InstructionSize
 		}
-
-		out = append(out, li)
 	}
 
 	return out, nil


Test patch:
diff --git a/btf/ext_info_test.go b/btf/ext_info_test.go
index f702667..581231a 100644
--- a/btf/ext_info_test.go
+++ b/btf/ext_info_test.go
@@ -2,10 +2,13 @@ package btf
 
 import (
 	"bytes"
+	"encoding/binary"
 	"strings"
 	"testing"
 
 	"github.com/cilium/ebpf/internal"
+
+	"github.com/go-quicktest/qt"
 )
 
 func TestParseExtInfoBigRecordSize(t *testing.T) {
@@ -23,3 +26,31 @@ func TestParseExtInfoBigRecordSize(t *testing.T) {
 		t.Error("Parsing line info with large record size doesn't return an error")
 	}
 }
+
+func BenchmarkParseLineInfoRecords(b *testing.B) {
+	size := uint32(binary.Size(bpfLineInfo{}))
+	count := uint32(4096)
+	buf := make([]byte, size*count)
+
+	b.ReportAllocs()
+	b.ResetTimer()
+
+	for i := 0; i < b.N; i++ {
+		parseLineInfoRecords(bytes.NewReader(buf), internal.NativeEndian, size, count, true)
+	}
+}
+
+func TestParseLineInfoRecordsAllocations(t *testing.T) {
+	size := uint32(binary.Size(bpfLineInfo{}))
+	count := uint32(4096)
+	buf := make([]byte, size*count)
+
+	allocs := testing.AllocsPerRun(5, func() {
+		parseLineInfoRecords(bytes.NewReader(buf), internal.NativeEndian, size, count, true)
+	})
+
+	// 7 is the number of allocations on go 1.22
+	// what we want to test is that we are not allocating
+	// once per record
+	qt.Assert(t, qt.IsTrue(allocs <= 7))
+}