From 49613823f8cbcc63bce681acbf2388462bff54e7 Mon Sep 17 00:00:00 2001 From: Linas Medziunas Date: Sun, 26 Nov 2023 20:12:44 +0200 Subject: [PATCH] Reuse slices in [Float]Histogram.ReduceResolution Signed-off-by: Linas Medziunas --- model/histogram/float_histogram.go | 17 +++++++------- model/histogram/float_histogram_test.go | 5 ++++ model/histogram/generic.go | 18 +++++++++++++- model/histogram/generic_test.go | 31 +++++++++++++++++++++++-- model/histogram/histogram.go | 4 ++-- model/histogram/histogram_test.go | 2 ++ 6 files changed, 64 insertions(+), 13 deletions(-) diff --git a/model/histogram/float_histogram.go b/model/histogram/float_histogram.go index 286913628b..6fa2213547 100644 --- a/model/histogram/float_histogram.go +++ b/model/histogram/float_histogram.go @@ -94,8 +94,8 @@ func (h *FloatHistogram) CopyToSchema(targetSchema int32) *FloatHistogram { Sum: h.Sum, } - c.PositiveSpans, c.PositiveBuckets = reduceResolution(h.PositiveSpans, h.PositiveBuckets, h.Schema, targetSchema, false) - c.NegativeSpans, c.NegativeBuckets = reduceResolution(h.NegativeSpans, h.NegativeBuckets, h.Schema, targetSchema, false) + c.PositiveSpans, c.PositiveBuckets = reduceResolution(h.PositiveSpans, h.PositiveBuckets, h.Schema, targetSchema, false, false) + c.NegativeSpans, c.NegativeBuckets = reduceResolution(h.NegativeSpans, h.NegativeBuckets, h.Schema, targetSchema, false, false) return &c } @@ -278,8 +278,8 @@ func (h *FloatHistogram) Add(other *FloatHistogram) *FloatHistogram { if other.Schema < h.Schema { panic(fmt.Errorf("cannot add histogram with schema %d to %d", other.Schema, h.Schema)) } else if other.Schema > h.Schema { - otherPositiveSpans, otherPositiveBuckets = reduceResolution(otherPositiveSpans, otherPositiveBuckets, other.Schema, h.Schema, false) - otherNegativeSpans, otherNegativeBuckets = reduceResolution(otherNegativeSpans, otherNegativeBuckets, other.Schema, h.Schema, false) + otherPositiveSpans, otherPositiveBuckets = reduceResolution(otherPositiveSpans, otherPositiveBuckets, other.Schema, h.Schema, false, false) + otherNegativeSpans, otherNegativeBuckets = reduceResolution(otherNegativeSpans, otherNegativeBuckets, other.Schema, h.Schema, false, false) } h.PositiveSpans, h.PositiveBuckets = addBuckets(h.Schema, h.ZeroThreshold, false, h.PositiveSpans, h.PositiveBuckets, otherPositiveSpans, otherPositiveBuckets) @@ -305,8 +305,8 @@ func (h *FloatHistogram) Sub(other *FloatHistogram) *FloatHistogram { if other.Schema < h.Schema { panic(fmt.Errorf("cannot subtract histigram with schema %d to %d", other.Schema, h.Schema)) } else if other.Schema > h.Schema { - otherPositiveSpans, otherPositiveBuckets = reduceResolution(otherPositiveSpans, otherPositiveBuckets, other.Schema, h.Schema, false) - otherNegativeSpans, otherNegativeBuckets = reduceResolution(otherNegativeSpans, otherNegativeBuckets, other.Schema, h.Schema, false) + otherPositiveSpans, otherPositiveBuckets = reduceResolution(otherPositiveSpans, otherPositiveBuckets, other.Schema, h.Schema, false, false) + otherNegativeSpans, otherNegativeBuckets = reduceResolution(otherNegativeSpans, otherNegativeBuckets, other.Schema, h.Schema, false, false) } h.PositiveSpans, h.PositiveBuckets = addBuckets(h.Schema, h.ZeroThreshold, true, h.PositiveSpans, h.PositiveBuckets, otherPositiveSpans, otherPositiveBuckets) @@ -1136,8 +1136,9 @@ func (h *FloatHistogram) ReduceResolution(targetSchema int32) *FloatHistogram { panic(fmt.Errorf("cannot reduce resolution from schema %d to %d", h.Schema, targetSchema)) } - h.PositiveSpans, h.PositiveBuckets = reduceResolution(h.PositiveSpans, h.PositiveBuckets, h.Schema, targetSchema, false) - h.NegativeSpans, h.NegativeBuckets = reduceResolution(h.NegativeSpans, h.NegativeBuckets, h.Schema, targetSchema, false) + h.PositiveSpans, h.PositiveBuckets = reduceResolution(h.PositiveSpans, h.PositiveBuckets, h.Schema, targetSchema, false, true) + h.NegativeSpans, h.NegativeBuckets = reduceResolution(h.NegativeSpans, h.NegativeBuckets, h.Schema, targetSchema, false, true) + h.Schema = targetSchema return h } diff --git a/model/histogram/float_histogram_test.go b/model/histogram/float_histogram_test.go index cbcd2a1f08..b93a6d90d0 100644 --- a/model/histogram/float_histogram_test.go +++ b/model/histogram/float_histogram_test.go @@ -1734,7 +1734,10 @@ func TestFloatHistogramCopyToSchema(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { + inCopy := c.in.Copy() require.Equal(t, c.expected, c.in.CopyToSchema(c.targetSchema)) + // Check that the receiver histogram was not mutated: + require.Equal(t, inCopy, c.in) }) } } @@ -2489,5 +2492,7 @@ func TestFloatHistogramReduceResolution(t *testing.T) { for _, tc := range tcs { target := tc.origin.ReduceResolution(tc.target.Schema) require.Equal(t, tc.target, target) + // Check that receiver histogram was mutated: + require.Equal(t, tc.target, tc.origin) } } diff --git a/model/histogram/generic.go b/model/histogram/generic.go index d42bb24151..7e1cc4b605 100644 --- a/model/histogram/generic.go +++ b/model/histogram/generic.go @@ -605,7 +605,16 @@ var exponentialBounds = [][]float64{ // The target schema must be smaller than the original schema. // Set deltaBuckets to true if the provided buckets are // deltas. Set it to false if the buckets contain absolute counts. -func reduceResolution[IBC InternalBucketCount](originSpans []Span, originBuckets []IBC, originSchema, targetSchema int32, deltaBuckets bool) ([]Span, []IBC) { +// Set inplace to true to reuse input slices and avoid allocations (otherwise +// new slices will be allocated for result). +func reduceResolution[IBC InternalBucketCount]( + originSpans []Span, + originBuckets []IBC, + originSchema, + targetSchema int32, + deltaBuckets bool, + inplace bool, +) ([]Span, []IBC) { var ( targetSpans []Span // The spans in the target schema. targetBuckets []IBC // The bucket counts in the target schema. @@ -617,6 +626,13 @@ func reduceResolution[IBC InternalBucketCount](originSpans []Span, originBuckets lastTargetBucketCount IBC ) + if inplace { + // Slice reuse is safe because when reducing the resolution, + // target slices don't grow faster than origin slices are being read. + targetSpans = originSpans[:0] + targetBuckets = originBuckets[:0] + } + for _, span := range originSpans { // Determine the index of the first bucket in this span. bucketIdx += span.Offset diff --git a/model/histogram/generic_test.go b/model/histogram/generic_test.go index d24910d214..b4d6585a8b 100644 --- a/model/histogram/generic_test.go +++ b/model/histogram/generic_test.go @@ -15,6 +15,7 @@ package histogram import ( "math" + "slices" "testing" "github.com/stretchr/testify/require" @@ -140,9 +141,22 @@ func TestReduceResolutionHistogram(t *testing.T) { } for _, tc := range cases { - spans, buckets := reduceResolution(tc.spans, tc.buckets, tc.schema, tc.targetSchema, true) + spansCopy, bucketsCopy := slices.Clone(tc.spans), slices.Clone(tc.buckets) + spans, buckets := reduceResolution(tc.spans, tc.buckets, tc.schema, tc.targetSchema, true, false) require.Equal(t, tc.expectedSpans, spans) require.Equal(t, tc.expectedBuckets, buckets) + // Verify inputs were not mutated: + require.Equal(t, spansCopy, tc.spans) + require.Equal(t, bucketsCopy, tc.buckets) + + // Output slices reuse input slices: + const inplace = true + spans, buckets = reduceResolution(tc.spans, tc.buckets, tc.schema, tc.targetSchema, true, inplace) + require.Equal(t, tc.expectedSpans, spans) + require.Equal(t, tc.expectedBuckets, buckets) + // Verify inputs were mutated which is now expected: + require.Equal(t, spans, tc.spans[:len(spans)]) + require.Equal(t, buckets, tc.buckets[:len(buckets)]) } } @@ -175,8 +189,21 @@ func TestReduceResolutionFloatHistogram(t *testing.T) { } for _, tc := range cases { - spans, buckets := reduceResolution(tc.spans, tc.buckets, tc.schema, tc.targetSchema, false) + spansCopy, bucketsCopy := slices.Clone(tc.spans), slices.Clone(tc.buckets) + spans, buckets := reduceResolution(tc.spans, tc.buckets, tc.schema, tc.targetSchema, false, false) require.Equal(t, tc.expectedSpans, spans) require.Equal(t, tc.expectedBuckets, buckets) + // Verify inputs were not mutated: + require.Equal(t, spansCopy, tc.spans) + require.Equal(t, bucketsCopy, tc.buckets) + + // Output slices reuse input slices: + const inplace = true + spans, buckets = reduceResolution(tc.spans, tc.buckets, tc.schema, tc.targetSchema, false, inplace) + require.Equal(t, tc.expectedSpans, spans) + require.Equal(t, tc.expectedBuckets, buckets) + // Verify inputs were mutated which is now expected: + require.Equal(t, spans, tc.spans[:len(spans)]) + require.Equal(t, buckets, tc.buckets[:len(buckets)]) } } diff --git a/model/histogram/histogram.go b/model/histogram/histogram.go index 4a12498f22..fb0185a638 100644 --- a/model/histogram/histogram.go +++ b/model/histogram/histogram.go @@ -502,10 +502,10 @@ func (h *Histogram) ReduceResolution(targetSchema int32) *Histogram { } h.PositiveSpans, h.PositiveBuckets = reduceResolution( - h.PositiveSpans, h.PositiveBuckets, h.Schema, targetSchema, true, + h.PositiveSpans, h.PositiveBuckets, h.Schema, targetSchema, true, true, ) h.NegativeSpans, h.NegativeBuckets = reduceResolution( - h.NegativeSpans, h.NegativeBuckets, h.Schema, targetSchema, true, + h.NegativeSpans, h.NegativeBuckets, h.Schema, targetSchema, true, true, ) h.Schema = targetSchema return h diff --git a/model/histogram/histogram_test.go b/model/histogram/histogram_test.go index 5aa9ca6feb..d5aed112ae 100644 --- a/model/histogram/histogram_test.go +++ b/model/histogram/histogram_test.go @@ -1008,5 +1008,7 @@ func TestHistogramReduceResolution(t *testing.T) { for _, tc := range tcs { target := tc.origin.ReduceResolution(tc.target.Schema) require.Equal(t, tc.target, target) + // Check that receiver histogram was mutated: + require.Equal(t, tc.target, tc.origin) } }