fix(godeltaprof): incorrect function names for generics functions (#106)

* fix(godeltaprof): port fix for golang/go#64528 * fix(godeltaprof): port fix for golang/go#64641 * add go 1.22 for testing
grafana · May 22, 2024 · ffd2615 · ffd2615
1 parent 1b1f973
commit ffd2615
Show file tree

Hide file tree

Showing 3 changed files with 161 additions and 5 deletions.
diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml
@@ -12,7 +12,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        go: ['1.16', '1.17', '1.18', '1.19', '1.20', '1.21', 'tip']
+        go: ['1.16', '1.17', '1.18', '1.19', '1.20', '1.21', '1.22', 'tip']
     steps:
       - name: Checkout
         uses: actions/checkout@v3

diff --git a/godeltaprof/compat/generics_go21_test.go b/godeltaprof/compat/generics_go21_test.go
@@ -5,8 +5,13 @@ package compat
 
 import (
 	"bytes"
+	"fmt"
+	"github.com/google/pprof/profile"
+	"io"
 	"runtime"
 	"runtime/pprof"
+	"slices"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -194,3 +199,153 @@ func TestMutex(t *testing.T) {
 		expectStackFrames(t, buffer, expectedRealShape, 19)
 	})
 }
+
+func profileToStrings(p *profile.Profile) []string {
+	var res []string
+	for _, s := range p.Sample {
+		res = append(res, sampleToString(s))
+	}
+	return res
+}
+
+func sampleToString(s *profile.Sample) string {
+	var funcs []string
+	for i := len(s.Location) - 1; i >= 0; i-- {
+		loc := s.Location[i]
+		funcs = locationToStrings(loc, funcs)
+	}
+	return fmt.Sprintf("%s %v", strings.Join(funcs, ";"), s.Value)
+}
+
+func locationToStrings(loc *profile.Location, funcs []string) []string {
+	for j := range loc.Line {
+		line := loc.Line[len(loc.Line)-1-j]
+		funcs = append(funcs, line.Function.Name)
+	}
+	return funcs
+}
+
+// This is a regression test for https://go.dev/issue/64528 .
+func TestGenericsHashKeyInPprofBuilder(t *testing.T) {
+	previousRate := runtime.MemProfileRate
+	runtime.MemProfileRate = 1
+	defer func() {
+		runtime.MemProfileRate = previousRate
+	}()
+	for _, sz := range []int{128, 256} {
+		genericAllocFunc[uint32](sz / 4)
+	}
+	for _, sz := range []int{32, 64} {
+		genericAllocFunc[uint64](sz / 8)
+	}
+
+	runtime.GC()
+	buf := bytes.NewBuffer(nil)
+	if err := WriteHeapProfile(buf); err != nil {
+		t.Fatalf("writing profile: %v", err)
+	}
+	p, err := profile.Parse(buf)
+	if err != nil {
+		t.Fatalf("profile.Parse: %v", err)
+	}
+
+	actual := profileToStrings(p)
+	expected := []string{
+		"testing.tRunner;github.com/grafana/pyroscope-go/godeltaprof/compat.TestGenericsHashKeyInPprofBuilder;github.com/grafana/pyroscope-go/godeltaprof/compat.genericAllocFunc[go.shape.uint32] [1 128 0 0]",
+		"testing.tRunner;github.com/grafana/pyroscope-go/godeltaprof/compat.TestGenericsHashKeyInPprofBuilder;github.com/grafana/pyroscope-go/godeltaprof/compat.genericAllocFunc[go.shape.uint32] [1 256 0 0]",
+		"testing.tRunner;github.com/grafana/pyroscope-go/godeltaprof/compat.TestGenericsHashKeyInPprofBuilder;github.com/grafana/pyroscope-go/godeltaprof/compat.genericAllocFunc[go.shape.uint64] [1 32 0 0]",
+		"testing.tRunner;github.com/grafana/pyroscope-go/godeltaprof/compat.TestGenericsHashKeyInPprofBuilder;github.com/grafana/pyroscope-go/godeltaprof/compat.genericAllocFunc[go.shape.uint64] [1 64 0 0]",
+	}
+
+	for _, l := range expected {
+		if !slices.Contains(actual, l) {
+			t.Errorf("profile = %v\nwant = %v", strings.Join(actual, "\n"), l)
+		}
+	}
+}
+
+type opAlloc struct {
+	buf [128]byte
+}
+
+type opCall struct {
+}
+
+var sink []byte
+
+func storeAlloc() {
+	sink = make([]byte, 16)
+}
+
+func nonRecursiveGenericAllocFunction[CurrentOp any, OtherOp any](alloc bool) {
+	if alloc {
+		storeAlloc()
+	} else {
+		nonRecursiveGenericAllocFunction[OtherOp, CurrentOp](true)
+	}
+}
+
+func TestGenericsInlineLocations(t *testing.T) {
+	if OptimizationOff() {
+		t.Skip("skipping test with optimizations disabled")
+	}
+
+	previousRate := runtime.MemProfileRate
+	runtime.MemProfileRate = 1
+	defer func() {
+		runtime.MemProfileRate = previousRate
+		sink = nil
+	}()
+
+	nonRecursiveGenericAllocFunction[opAlloc, opCall](true)
+	nonRecursiveGenericAllocFunction[opCall, opAlloc](false)
+
+	runtime.GC()
+
+	buf := bytes.NewBuffer(nil)
+	if err := WriteHeapProfile(buf); err != nil {
+		t.Fatalf("writing profile: %v", err)
+	}
+	p, err := profile.Parse(buf)
+	if err != nil {
+		t.Fatalf("profile.Parse: %v", err)
+	}
+
+	const expectedSample = "testing.tRunner;github.com/grafana/pyroscope-go/godeltaprof/compat.TestGenericsInlineLocations;github.com/grafana/pyroscope-go/godeltaprof/compat.nonRecursiveGenericAllocFunction[go.shape.struct {},go.shape.struct { github.com/grafana/pyroscope-go/godeltaprof/compat.buf [128]uint8 }];github.com/grafana/pyroscope-go/godeltaprof/compat.nonRecursiveGenericAllocFunction[go.shape.struct { github.com/grafana/pyroscope-go/godeltaprof/compat.buf [128]uint8 },go.shape.struct {}];github.com/grafana/pyroscope-go/godeltaprof/compat.storeAlloc [1 16 1 16]"
+	const expectedLocation = "github.com/grafana/pyroscope-go/godeltaprof/compat.nonRecursiveGenericAllocFunction[go.shape.struct {},go.shape.struct { github.com/grafana/pyroscope-go/godeltaprof/compat.buf [128]uint8 }];github.com/grafana/pyroscope-go/godeltaprof/compat.nonRecursiveGenericAllocFunction[go.shape.struct { github.com/grafana/pyroscope-go/godeltaprof/compat.buf [128]uint8 },go.shape.struct {}];github.com/grafana/pyroscope-go/godeltaprof/compat.storeAlloc"
+	const expectedLocationNewInliner = "github.com/grafana/pyroscope-go/godeltaprof/compat.TestGenericsInlineLocations;" + expectedLocation
+	var s *profile.Sample
+	for _, sample := range p.Sample {
+		if sampleToString(sample) == expectedSample {
+			s = sample
+			break
+		}
+	}
+	if s == nil {
+		t.Fatalf("expected \n%s\ngot\n%s", expectedSample, strings.Join(profileToStrings(p), "\n"))
+	}
+	loc := s.Location[0]
+	actual := strings.Join(locationToStrings(loc, nil), ";")
+	if expectedLocation != actual && expectedLocationNewInliner != actual {
+		t.Errorf("expected a location with at least 3 functions\n%s\ngot\n%s\n", expectedLocation, actual)
+	}
+}
+
+func OptimizationOff() bool {
+	optimizationMarker := func() uintptr {
+		pc, _, _, _ := runtime.Caller(0)
+		return pc
+	}
+	pc := optimizationMarker()
+	f := runtime.FuncForPC(runtime.FuncForPC(pc).Entry())
+	return f.Name() == "github.com/grafana/pyroscope-go/godeltaprof/compat.OptimizationOff.func1"
+}
+
+func WriteHeapProfile(w io.Writer) error {
+	runtime.GC()
+	dh := godeltaprof.NewHeapProfilerWithOptions(godeltaprof.ProfileOptions{
+		GenericsFrames: true,
+		LazyMappings:   true,
+	})
+	return dh.Profile(w)
+}
diff --git a/godeltaprof/internal/pprof/proto.go b/godeltaprof/internal/pprof/proto.go
@@ -474,7 +474,7 @@ func (d *pcDeck) tryAdd(pc uintptr, frames []runtime.Frame, symbolizeResult symb
 		if last.Entry != newFrame.Entry { // newFrame is for a different function.
 			return false
 		}
-		if last.Function == newFrame.Function { // maybe recursion.
+		if runtime_FrameSymbolName(&last) == runtime_FrameSymbolName(&newFrame) { // maybe recursion.
 			return false
 		}
 	}
@@ -524,13 +524,14 @@ func (b *profileBuilder) emitLocation() uint64 {
 	b.pb.uint64Opt(tagLocation_Address, uint64(firstFrame.PC))
 	for _, frame := range b.deck.frames {
 		// Write out each line in frame expansion.
-		funcID := uint64(b.funcs[frame.Function])
+		funcName := runtime_FrameSymbolName(&frame)
+		funcID := uint64(b.funcs[funcName])
 		if funcID == 0 {
 			funcID = uint64(len(b.funcs)) + 1
-			b.funcs[frame.Function] = int(funcID)
+			b.funcs[funcName] = int(funcID)
 			var name string
 			if b.opt.GenericsFrames {
-				name = runtime_FrameSymbolName(&frame)
+				name = funcName
 			} else {
 				name = frame.Function
 			}