fix: deprecate pprof MergeNoClone (#3251)

pkg/pprof/fix_go_profile.go

@@ -56,7 +56,7 @@ func DropGoTypeParameters(p *profilev1.Profile) *profilev1.Profile {
 	var m ProfileMerge
 	// We safely ignore the error as the only case when it can
 	// happen is when merged profiles are of different types.
-	_ = m.MergeNoClone(p)
+	_ = m.Merge(p)
 	return m.Profile()
 }
 

pkg/pprof/merge.go

@@ -9,6 +9,19 @@ import (
 	"github.com/grafana/pyroscope/pkg/slices"
 )
 
+// TODO(kolesnikovae):
+//   Add a function that incorporates Merge and Normalize.
+//   Both functions perform some sanity checks but none of them
+//   is enough to "vet" the profile completely.
+//   Specifically:
+//    - it's possible that unreferenced objects will remain in the
+//      profile, and therefore will be written to the storage.
+//    - Normalize does not remove duplicates and unreferenced objects
+//      except samples.
+//    - Merge does not remove unreferenced objects at all.
+//    - Merge is fairly expensive: allocated capacities should be
+//      reused and the number of allocs decreased.
+
 type ProfileMerge struct {
 	profile *profilev1.Profile
 	tmp     []uint32
@@ -23,24 +36,14 @@ type ProfileMerge struct {
 // Merge adds p to the profile merge, cloning new objects.
 // Profile p is modified in place but not retained by the function.
 func (m *ProfileMerge) Merge(p *profilev1.Profile) error {
-	return m.merge(p, true)
-}
-
-// MergeNoClone adds p to the profile merge, borrowing objects.
-// Profile p is modified in place and retained by the function.
-func (m *ProfileMerge) MergeNoClone(p *profilev1.Profile) error {
-	return m.merge(p, false)
-}
-
-func (m *ProfileMerge) merge(p *profilev1.Profile, clone bool) error {
 	if p == nil || len(p.Sample) == 0 || len(p.StringTable) < 2 {
 		return nil
 	}
 
 	sanitizeProfile(p)
 	var initial bool
 	if m.profile == nil {
-		m.init(p, clone)
+		m.init(p)
 		initial = true
 	}
 
@@ -111,43 +114,29 @@ func (m *ProfileMerge) Profile() *profilev1.Profile {
 	return m.profile
 }
 
-func (m *ProfileMerge) init(x *profilev1.Profile, clone bool) {
+func (m *ProfileMerge) init(x *profilev1.Profile) {
 	factor := 2
 	m.stringTable = NewRewriteTable(
 		factor*len(x.StringTable),
 		func(s string) string { return s },
 		func(s string) string { return s },
 	)
 
-	if clone {
-		m.functionTable = NewRewriteTable[FunctionKey, *profilev1.Function, *profilev1.Function](
-			factor*len(x.Function), GetFunctionKey, cloneVT[*profilev1.Function])
+	m.functionTable = NewRewriteTable[FunctionKey, *profilev1.Function, *profilev1.Function](
+		factor*len(x.Function), GetFunctionKey, cloneVT[*profilev1.Function])
 
-		m.mappingTable = NewRewriteTable[MappingKey, *profilev1.Mapping, *profilev1.Mapping](
-			factor*len(x.Mapping), GetMappingKey, cloneVT[*profilev1.Mapping])
+	m.mappingTable = NewRewriteTable[MappingKey, *profilev1.Mapping, *profilev1.Mapping](
+		factor*len(x.Mapping), GetMappingKey, cloneVT[*profilev1.Mapping])
 
-		m.locationTable = NewRewriteTable[LocationKey, *profilev1.Location, *profilev1.Location](
-			factor*len(x.Location), GetLocationKey, cloneVT[*profilev1.Location])
+	m.locationTable = NewRewriteTable[LocationKey, *profilev1.Location, *profilev1.Location](
+		factor*len(x.Location), GetLocationKey, cloneVT[*profilev1.Location])
 
-		m.sampleTable = NewRewriteTable[SampleKey, *profilev1.Sample, *profilev1.Sample](
-			factor*len(x.Sample), GetSampleKey, func(sample *profilev1.Sample) *profilev1.Sample {
-				c := sample.CloneVT()
-				slices.Clear(c.Value)
-				return c
-			})
-	} else {
-		m.functionTable = NewRewriteTable[FunctionKey, *profilev1.Function, *profilev1.Function](
-			factor*len(x.Function), GetFunctionKey, noClone[*profilev1.Function])
-
-		m.mappingTable = NewRewriteTable[MappingKey, *profilev1.Mapping, *profilev1.Mapping](
-			factor*len(x.Mapping), GetMappingKey, noClone[*profilev1.Mapping])
-
-		m.locationTable = NewRewriteTable[LocationKey, *profilev1.Location, *profilev1.Location](
-			factor*len(x.Location), GetLocationKey, noClone[*profilev1.Location])
-
-		m.sampleTable = NewRewriteTable[SampleKey, *profilev1.Sample, *profilev1.Sample](
-			factor*len(x.Sample), GetSampleKey, noClone[*profilev1.Sample])
-	}
+	m.sampleTable = NewRewriteTable[SampleKey, *profilev1.Sample, *profilev1.Sample](
+		factor*len(x.Sample), GetSampleKey, func(sample *profilev1.Sample) *profilev1.Sample {
+			c := sample.CloneVT()
+			slices.Clear(c.Value)
+			return c
+		})
 
 	m.profile = &profilev1.Profile{
 		SampleType: make([]*profilev1.ValueType, len(x.SampleType)),
@@ -166,8 +155,6 @@ func (m *ProfileMerge) init(x *profilev1.Profile, clone bool) {
 	}
 }
 
-func noClone[T any](t T) T { return t }
-
 func cloneVT[T interface{ CloneVT() T }](t T) T { return t.CloneVT() }
 
 // combineHeaders checks that all profiles can be merged and returns

pkg/pprof/merge_test.go

@@ -7,6 +7,7 @@ import (
 	"net"
 	"os"
 	"path/filepath"
+	"sort"
 	"strings"
 	"sync"
 	"testing"
@@ -23,10 +24,17 @@ func Test_Merge_Single(t *testing.T) {
 	p, err := OpenFile("testdata/go.cpu.labels.pprof")
 	require.NoError(t, err)
 	var m ProfileMerge
-	require.NoError(t, m.Merge(p.Profile))
+	require.NoError(t, m.Merge(p.Profile.CloneVT()))
+	sortLabels(p.Profile)
 	testhelper.EqualProto(t, p.Profile, m.Profile())
 }
 
+func sortLabels(p *profilev1.Profile) {
+	for _, s := range p.Sample {
+		sort.Sort(LabelsByKeyValue(s.Label))
+	}
+}
+
 type fuzzEvent byte
 
 const (
@@ -183,15 +191,16 @@ func Test_Merge_Self(t *testing.T) {
 	p, err := OpenFile("testdata/go.cpu.labels.pprof")
 	require.NoError(t, err)
 	var m ProfileMerge
-	require.NoError(t, m.Merge(p.Profile))
-	require.NoError(t, m.Merge(p.Profile))
+	require.NoError(t, m.Merge(p.Profile.CloneVT()))
+	require.NoError(t, m.Merge(p.Profile.CloneVT()))
 	for i := range p.Sample {
 		s := p.Sample[i]
 		for j := range s.Value {
 			s.Value[j] *= 2
 		}
 	}
 	p.DurationNanos *= 2
+	sortLabels(p.Profile)
 	testhelper.EqualProto(t, p.Profile, m.Profile())
 }
 
@@ -211,8 +220,10 @@ func Test_Merge_Halves(t *testing.T) {
 
 	// Merge with self for normalisation.
 	var sm ProfileMerge
-	require.NoError(t, sm.Merge(p.Profile))
+	require.NoError(t, sm.Merge(p.Profile.CloneVT()))
 	p.DurationNanos *= 2
+
+	sortLabels(p.Profile)
 	testhelper.EqualProto(t, p.Profile, m.Profile())
 }
 
@@ -588,23 +599,12 @@ func TestMergeEmpty(t *testing.T) {
 	require.NoError(t, err)
 }
 
-// Benchmark_Merge_self/pprof.MergeNoClone-10         	    4174	    290190 ns/op
 // Benchmark_Merge_self/pprof.Merge-10                	    2722	    421419 ns/op
 // Benchmark_Merge_self/profile.Merge-10              	     802	   1417907 ns/op
 func Benchmark_Merge_self(b *testing.B) {
 	d, err := os.ReadFile("testdata/go.cpu.labels.pprof")
 	require.NoError(b, err)
 
-	b.Run("pprof.MergeNoClone", func(b *testing.B) {
-		p, err := RawFromBytes(d)
-		require.NoError(b, err)
-		b.ResetTimer()
-		for i := 0; i < b.N; i++ {
-			var m ProfileMerge
-			require.NoError(b, m.MergeNoClone(p.Profile.CloneVT()))
-		}
-	})
-
 	b.Run("pprof.Merge", func(b *testing.B) {
 		p, err := RawFromBytes(d)
 		require.NoError(b, err)