process: reuse and preallocate memory

[florianl]

No description provided.

[rockdaboot]

Can you add memory allocations for before and after?

[florianl]

Updated benchmark on 4b538b5:

$ benchstat old.txt new.txt 
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/ebpf-profiler/process
cpu: 12th Gen Intel(R) Core(TM) i7-12700H
               │   old.txt   │               new.txt               │
               │   sec/op    │   sec/op     vs base                │
GetMappings-20   37.14µ ± 1%   38.01µ ± 1%  +2.34% (p=0.000 n=10)

               │    old.txt    │               new.txt               │
               │     B/op      │     B/op      vs base               │
GetMappings-20   10.574Ki ± 0%   9.605Ki ± 0%  -9.16% (p=0.000 n=10)

               │  old.txt   │              new.txt               │
               │ allocs/op  │ allocs/op   vs base                │
GetMappings-20   28.00 ± 0%   24.00 ± 0%  -14.29% (p=0.000 n=10)

BenchmarkGetMappings

func BenchmarkGetMappings(b *testing.B) {
	pr := New(libpf.PID(os.Getpid()))
	b.ResetTimer()
	b.ReportAllocs()

	for i := 0; i < b.N; i++ {
		mappings, err := pr.GetMappings()
		if err != nil {
			b.Fatal(err)
		}
		if len(mappings) == 0 {
			b.Fatal("expected non-empty mappings")
		}
	}
}

[florianl]

Updated benchmark on 4b538b5:

$ benchstat old.txt new.txt 
goos: linux
goarch: amd64
pkg: go.opentelemetry.io/ebpf-profiler/process
cpu: 12th Gen Intel(R) Core(TM) i7-12700H
                 │   old.txt   │              new.txt               │
                 │   sec/op    │   sec/op     vs base               │
ParseMappings-20   202.4µ ± 2%   204.4µ ± 5%  +0.97% (p=0.009 n=10)

                 │   old.txt    │               new.txt               │
                 │     B/op     │     B/op      vs base               │
ParseMappings-20   164.7Ki ± 0%   162.3Ki ± 0%  -1.48% (p=0.000 n=10)

                 │  old.txt   │              new.txt              │
                 │ allocs/op  │ allocs/op   vs base               │
ParseMappings-20   444.0 ± 0%   438.0 ± 0%  -1.35% (p=0.000 n=10)

BenchmarkParseMappings

func BenchmarkParseMappings(b *testing.B) {
    // Get sample data from current process
    mapsFile, err := os.Open(fmt.Sprintf("/proc/%d/maps", os.Getpid()))
    if err != nil {
        b.Fatal(err)
    }
    defer mapsFile.Close()
    
    // Read all content for reuse in benchmark
    content, err := io.ReadAll(mapsFile)
    if err != nil {
        b.Fatal(err)
    }

    b.ResetTimer()
    b.ReportAllocs()
    
    for i := 0; i < b.N; i++ {
        reader := bytes.NewReader(content)
        mappings, err := parseMappings(reader)
        if err != nil {
            b.Fatal(err)
        }
        if len(mappings) == 0 {
            b.Fatal("expected non-empty mappings")
        }
    }
}

[rockdaboot]

On a second thought... why clear the memory at all here? parseMapping() does not rely on the contents of the buffer retrieved from the pool. The scanner read data and only acts on the read data in the buffer. It doesn't matter if we fill the bufferswith 0 bytes or random bytes or leave it as is.

Suggested change

	// Reset memory and return it for reuse.
	for j := 0; j < len(*scanBuf); j++ {
	(*scanBuf)[j] = 0x0
	}

[florianl]

The scanner, that uses this buffer, uses the default split function ScanLines. By not clearing the buffer, we might introduce subtle bugs.

[rockdaboot]

we might introduce subtle bugs

Possibly yes. Can you add a test case that triggers such a bug. That makes it obvious to if someone ever touches that code or wonders if it is required.

[rockdaboot]

Hm, I can't see how bufio.Scanner.Scan() uses ScanLines().

[florianl]

As we don't set a specific Split function, the buffer uses the default https://pkg.go.dev/bufio#NewScanner.

[florianl]

I have reverted the applied suggestion, as I should not have applied in the first place. And I also reasked for review.
I have reverted the applied suggestion as resetting a buffer before reusing is something I will ask in a review, until it is shown that the operation can be done correctly with every state of the buffer. And this proposed change does not show that it will work as supposed with every state of the shared buffer.

[rockdaboot]

I still didn't see any argument why or what exactly can fail. Without an explicit statement it is not possible to create a test case. Can't we please go with the facts and read the code instead of having some vague "fear"?

[rockdaboot]

As I said, the std library looks sound to me and doesn't require resetting the buffer to 0. Additionally, we can prove that empirically with a comparison fuzzer:

diff --git a/process/process_test.go b/process/process_test.go
index 3cd2b7d..f64700c 100644
--- a/process/process_test.go
+++ b/process/process_test.go
@@ -4,6 +4,7 @@
 package process
 
 import (
+       "bytes"
        "debug/elf"
        "os"
        "strings"
@@ -105,3 +106,16 @@ func TestNewPIDOfSelf(t *testing.T) {
        require.NoError(t, err)
        assert.NotEmpty(t, mappings)
 }
+
+func FuzzParseMappings(f *testing.F) {
+       f.Fuzz(func(t *testing.T, input []byte) {
+               r1 := bytes.NewReader(input)
+               m1, err1 := parseMappings(r1)
+
+               r2 := bytes.NewReader(input)
+               m2, err2 := parseMappingsNoReset(r2)
+
+               require.Equal(t, err1, err2)
+               require.Equal(t, m1, m2)
+       })
+}

After ~30min, there is no indication that resetting the buffer makes any difference.

...
fuzz: elapsed: 31m45s, execs: 219880737 (97874/sec), new interesting: 77 (total: 149)
fuzz: elapsed: 31m48s, execs: 220278902 (132716/sec), new interesting: 77 (total: 149)
fuzz: elapsed: 31m51s, execs: 220549371 (90139/sec), new interesting: 77 (total: 149)

Unexpected result from running the fuzzer: The agent exits if it fails to parse fields to integer values, see #366

[florianl]

I will likely close this PR without merge, as there is no consensus.
For me, reused memory should always be cleared unless it can be shown that not clearing the memory does not interfere with expected behavior. And this PR does not show, that the function works as expected with every state of reused and not reseted memory.
Clearing memory before reusing it prevents a whole class of memory related bugs and I don't want to be the person, introducing such a change.

[rockdaboot]

I think we now have all the facts on the table. Why not wait for other reviews and then follow whatever the consensus is?

[rockdaboot]

Approving, we should create a follow-up issue or PR for the open discussion on whether we want resetting of the buffer.