diff --git a/pkg/pprof/fix_go_profile.go b/pkg/pprof/fix_go_profile.go index e01b80f17a..e073f9ae21 100644 --- a/pkg/pprof/fix_go_profile.go +++ b/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() } diff --git a/pkg/pprof/merge.go b/pkg/pprof/merge.go index 335823af83..b0da12f061 100644 --- a/pkg/pprof/merge.go +++ b/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,16 +36,6 @@ 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 } @@ -40,7 +43,7 @@ func (m *ProfileMerge) merge(p *profilev1.Profile, clone bool) error { sanitizeProfile(p) var initial bool if m.profile == nil { - m.init(p, clone) + m.init(p) initial = true } @@ -111,7 +114,7 @@ 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), @@ -119,35 +122,21 @@ func (m *ProfileMerge) init(x *profilev1.Profile, clone bool) { 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 diff --git a/pkg/pprof/merge_test.go b/pkg/pprof/merge_test.go index 2396fa34f0..e10e1002f4 100644 --- a/pkg/pprof/merge_test.go +++ b/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,8 +191,8 @@ 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 { @@ -192,6 +200,7 @@ func Test_Merge_Self(t *testing.T) { } } 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)