From b63cf49bbaf02312504f6ec5cc8b68ed3cd5ec5e Mon Sep 17 00:00:00 2001 From: jmacd Date: Thu, 21 May 2020 02:08:08 -0700 Subject: [PATCH 1/8] Use sort.Search to locate histogram bucket --- sdk/metric/aggregator/histogram/histogram.go | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/sdk/metric/aggregator/histogram/histogram.go b/sdk/metric/aggregator/histogram/histogram.go index ccb0c2d3c..66bd8000b 100644 --- a/sdk/metric/aggregator/histogram/histogram.go +++ b/sdk/metric/aggregator/histogram/histogram.go @@ -127,13 +127,9 @@ func emptyState(boundaries []metric.Number) state { func (c *Aggregator) Update(_ context.Context, number metric.Number, desc *metric.Descriptor) error { kind := desc.NumberKind() - bucketID := len(c.boundaries) - for i, boundary := range c.boundaries { - if number.CompareNumber(kind, boundary) < 0 { - bucketID = i - break - } - } + bucketID := sort.Search(len(c.boundaries), func(i int) bool { + return number.CompareNumber(kind, c.boundaries[i]) < 0 + }) c.lock.Lock() defer c.lock.Unlock() From dacebd643053f3c62482f8a4d91adffb63bdfd61 Mon Sep 17 00:00:00 2001 From: jmacd Date: Thu, 21 May 2020 10:29:03 -0700 Subject: [PATCH 2/8] Use []float64 for boundaries --- exporters/metric/prometheus/prometheus.go | 10 +-- .../metric/prometheus/prometheus_test.go | 2 +- exporters/metric/test/test.go | 2 +- sdk/export/metric/aggregator/aggregator.go | 4 +- sdk/metric/aggregator/histogram/histogram.go | 52 ++++-------- .../aggregator/histogram/histogram_test.go | 83 ++++++++++--------- sdk/metric/histogram_stress_test.go | 4 +- sdk/metric/selector/simple/simple.go | 4 +- sdk/metric/selector/simple/simple_test.go | 2 +- 9 files changed, 74 insertions(+), 89 deletions(-) diff --git a/exporters/metric/prometheus/prometheus.go b/exporters/metric/prometheus/prometheus.go index d76a1faf3..6dd1b0271 100644 --- a/exporters/metric/prometheus/prometheus.go +++ b/exporters/metric/prometheus/prometheus.go @@ -54,7 +54,7 @@ type Exporter struct { onError func(error) defaultSummaryQuantiles []float64 - defaultHistogramBoundaries []metric.Number + defaultHistogramBoundaries []float64 } var _ http.Handler = &Exporter{} @@ -85,7 +85,7 @@ type Config struct { // DefaultHistogramBoundaries defines the default histogram bucket // boundaries. - DefaultHistogramBoundaries []metric.Number + DefaultHistogramBoundaries []float64 // OnError is a function that handle errors that may occur while exporting metrics. // TODO: This should be refactored or even removed once we have a better error handling mechanism. @@ -330,12 +330,12 @@ func (c *collector) exportHistogram(ch chan<- prometheus.Metric, hist aggregator // The bucket with upper-bound +inf is not included. counts := make(map[float64]uint64, len(buckets.Boundaries)) for i := range buckets.Boundaries { - boundary := buckets.Boundaries[i].CoerceToFloat64(kind) - totalCount += buckets.Counts[i].AsUint64() + boundary := buckets.Boundaries[i] + totalCount += uint64(buckets.Counts[i]) counts[boundary] = totalCount } // Include the +inf bucket in the total count. - totalCount += buckets.Counts[len(buckets.Counts)-1].AsUint64() + totalCount += uint64(buckets.Counts[len(buckets.Counts)-1]) m, err := prometheus.NewConstHistogram(desc, totalCount, sum.CoerceToFloat64(kind), counts, labels...) if err != nil { diff --git a/exporters/metric/prometheus/prometheus_test.go b/exporters/metric/prometheus/prometheus_test.go index db87e0313..dbd08aba9 100644 --- a/exporters/metric/prometheus/prometheus_test.go +++ b/exporters/metric/prometheus/prometheus_test.go @@ -34,7 +34,7 @@ import ( func TestPrometheusExporter(t *testing.T) { exporter, err := prometheus.NewExportPipeline(prometheus.Config{ - DefaultHistogramBoundaries: []metric.Number{metric.NewFloat64Number(-0.5), metric.NewFloat64Number(1)}, + DefaultHistogramBoundaries: []float64{-0.5, 1}, }) require.NoError(t, err) diff --git a/exporters/metric/test/test.go b/exporters/metric/test/test.go index 9fd639493..7a788efd5 100644 --- a/exporters/metric/test/test.go +++ b/exporters/metric/test/test.go @@ -97,7 +97,7 @@ func (p *CheckpointSet) AddValueRecorder(desc *metric.Descriptor, v float64, lab p.updateAggregator(desc, array.New(), v, labels...) } -func (p *CheckpointSet) AddHistogramValueRecorder(desc *metric.Descriptor, boundaries []metric.Number, v float64, labels ...kv.KeyValue) { +func (p *CheckpointSet) AddHistogramValueRecorder(desc *metric.Descriptor, boundaries []float64, v float64, labels ...kv.KeyValue) { p.updateAggregator(desc, histogram.New(desc, boundaries), v, labels...) } diff --git a/sdk/export/metric/aggregator/aggregator.go b/sdk/export/metric/aggregator/aggregator.go index f0b6409e6..52fe13435 100644 --- a/sdk/export/metric/aggregator/aggregator.go +++ b/sdk/export/metric/aggregator/aggregator.go @@ -68,8 +68,8 @@ type ( // For a Histogram with N defined boundaries, e.g, [x, y, z]. // There are N+1 counts: [-inf, x), [x, y), [y, z), [z, +inf] Buckets struct { - Boundaries []metric.Number - Counts []metric.Number + Boundaries []float64 + Counts []float64 } // Histogram returns the count of events in pre-determined buckets. diff --git a/sdk/metric/aggregator/histogram/histogram.go b/sdk/metric/aggregator/histogram/histogram.go index 66bd8000b..bc52f9872 100644 --- a/sdk/metric/aggregator/histogram/histogram.go +++ b/sdk/metric/aggregator/histogram/histogram.go @@ -36,7 +36,7 @@ type ( lock sync.Mutex current state checkpoint state - boundaries []metric.Number + boundaries []float64 kind metric.NumberKind } @@ -44,7 +44,7 @@ type ( // the sum and counts for all observed values and // the less than equal bucket count for the pre-determined boundaries. state struct { - bucketCounts []metric.Number + bucketCounts []float64 count metric.Number sum metric.Number } @@ -63,17 +63,19 @@ var _ aggregator.Histogram = &Aggregator{} // Note that this aggregator maintains each value using independent // atomic operations, which introduces the possibility that // checkpoints are inconsistent. -func New(desc *metric.Descriptor, boundaries []metric.Number) *Aggregator { +func New(desc *metric.Descriptor, boundaries []float64) *Aggregator { // Boundaries MUST be ordered otherwise the histogram could not // be properly computed. - sortedBoundaries := numbers{ - numbers: make([]metric.Number, len(boundaries)), - kind: desc.NumberKind(), - } + // metric.SortNumbers(desc.NumberKind(), boundaries) + // sortedBoundaries := numbers{ + // numbers: make([]metric.Number, len(boundaries)), + // kind: desc.NumberKind(), + // } + sort.Float64s(boundaries) - copy(sortedBoundaries.numbers, boundaries) - sort.Sort(&sortedBoundaries) - boundaries = sortedBoundaries.numbers + // copy(sortedBoundaries.numbers, boundaries) + // sort.Sort(&sortedBoundaries) + // boundaries = sortedBoundaries.numbers return &Aggregator{ kind: desc.NumberKind(), @@ -117,9 +119,9 @@ func (c *Aggregator) Checkpoint(ctx context.Context, desc *metric.Descriptor) { c.lock.Unlock() } -func emptyState(boundaries []metric.Number) state { +func emptyState(boundaries []float64) state { return state{ - bucketCounts: make([]metric.Number, len(boundaries)+1), + bucketCounts: make([]float64, len(boundaries)+1), } } @@ -128,7 +130,7 @@ func (c *Aggregator) Update(_ context.Context, number metric.Number, desc *metri kind := desc.NumberKind() bucketID := sort.Search(len(c.boundaries), func(i int) bool { - return number.CompareNumber(kind, c.boundaries[i]) < 0 + return number.CoerceToFloat64(kind) < c.boundaries[i] }) c.lock.Lock() @@ -136,7 +138,7 @@ func (c *Aggregator) Update(_ context.Context, number metric.Number, desc *metri c.current.count.AddInt64(1) c.current.sum.AddNumber(kind, number) - c.current.bucketCounts[bucketID].AddUint64(1) + c.current.bucketCounts[bucketID]++ return nil } @@ -152,27 +154,7 @@ func (c *Aggregator) Merge(oa export.Aggregator, desc *metric.Descriptor) error c.checkpoint.count.AddNumber(metric.Uint64NumberKind, o.checkpoint.count) for i := 0; i < len(c.checkpoint.bucketCounts); i++ { - c.checkpoint.bucketCounts[i].AddNumber(metric.Uint64NumberKind, o.checkpoint.bucketCounts[i]) + c.checkpoint.bucketCounts[i] += o.checkpoint.bucketCounts[i] } return nil } - -// numbers is an auxiliary struct to order histogram bucket boundaries (slice of kv.Number) -type numbers struct { - numbers []metric.Number - kind metric.NumberKind -} - -var _ sort.Interface = (*numbers)(nil) - -func (n *numbers) Len() int { - return len(n.numbers) -} - -func (n *numbers) Less(i, j int) bool { - return -1 == n.numbers[i].CompareNumber(n.kind, n.numbers[j]) -} - -func (n *numbers) Swap(i, j int) { - n.numbers[i], n.numbers[j] = n.numbers[j], n.numbers[i] -} diff --git a/sdk/metric/aggregator/histogram/histogram_test.go b/sdk/metric/aggregator/histogram/histogram_test.go index c1541ee2b..a34b72486 100644 --- a/sdk/metric/aggregator/histogram/histogram_test.go +++ b/sdk/metric/aggregator/histogram/histogram_test.go @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -package histogram +package histogram_test import ( "context" @@ -24,6 +24,7 @@ import ( "github.com/stretchr/testify/require" "go.opentelemetry.io/otel/api/metric" + "go.opentelemetry.io/otel/sdk/metric/aggregator/histogram" "go.opentelemetry.io/otel/sdk/metric/aggregator/test" ) @@ -57,36 +58,33 @@ var ( }, } - boundaries = map[metric.NumberKind][]metric.Number{ - metric.Float64NumberKind: {metric.NewFloat64Number(500), metric.NewFloat64Number(250), metric.NewFloat64Number(750)}, - metric.Int64NumberKind: {metric.NewInt64Number(500), metric.NewInt64Number(250), metric.NewInt64Number(750)}, - } + boundaries = []float64{500, 250, 750} ) func TestHistogramAbsolute(t *testing.T) { test.RunProfiles(t, func(t *testing.T, profile test.Profile) { - histogram(t, profile, positiveOnly) + testHistogram(t, profile, positiveOnly) }) } func TestHistogramNegativeOnly(t *testing.T) { test.RunProfiles(t, func(t *testing.T, profile test.Profile) { - histogram(t, profile, negativeOnly) + testHistogram(t, profile, negativeOnly) }) } func TestHistogramPositiveAndNegative(t *testing.T) { test.RunProfiles(t, func(t *testing.T, profile test.Profile) { - histogram(t, profile, positiveAndNegative) + testHistogram(t, profile, positiveAndNegative) }) } // Validates count, sum and buckets for a given profile and policy -func histogram(t *testing.T, profile test.Profile, policy policy) { +func testHistogram(t *testing.T, profile test.Profile, policy policy) { ctx := context.Background() descriptor := test.NewAggregatorTest(metric.ValueRecorderKind, profile.NumberKind) - agg := New(descriptor, boundaries[profile.NumberKind]) + agg := histogram.New(descriptor, boundaries) all := test.NewNumbers(profile.NumberKind) @@ -107,18 +105,21 @@ func histogram(t *testing.T, profile test.Profile, policy policy) { asum.CoerceToFloat64(profile.NumberKind), 0.000000001, "Same sum - "+policy.name) - require.Nil(t, err) + require.NoError(t, err) count, err := agg.Count() require.Equal(t, all.Count(), count, "Same count -"+policy.name) - require.Nil(t, err) + require.NoError(t, err) - require.Equal(t, len(agg.checkpoint.bucketCounts), len(boundaries[profile.NumberKind])+1, "There should be b + 1 counts, where b is the number of boundaries") + buckets, err := agg.Histogram() + require.NoError(t, err) + + require.Equal(t, len(buckets.Counts), len(boundaries)+1, "There should be b + 1 counts, where b is the number of boundaries") counts := calcBuckets(all.Points(), profile) for i, v := range counts { - bCount := agg.checkpoint.bucketCounts[i].AsUint64() - require.Equal(t, v, bCount, "Wrong bucket #%d count: %v != %v", i, counts, agg.checkpoint.bucketCounts) + bCount := uint64(buckets.Counts[i]) + require.Equal(t, v, bCount, "Wrong bucket #%d count: %v != %v", i, counts, buckets.Counts) } } @@ -126,12 +127,12 @@ func TestHistogramInitial(t *testing.T) { test.RunProfiles(t, func(t *testing.T, profile test.Profile) { descriptor := test.NewAggregatorTest(metric.ValueRecorderKind, profile.NumberKind) - agg := New(descriptor, boundaries[profile.NumberKind]) + agg := histogram.New(descriptor, boundaries) buckets, err := agg.Histogram() require.NoError(t, err) - require.Equal(t, len(buckets.Counts), len(boundaries[profile.NumberKind])+1) - require.Equal(t, len(buckets.Boundaries), len(boundaries[profile.NumberKind])) + require.Equal(t, len(buckets.Counts), len(boundaries)+1) + require.Equal(t, len(buckets.Boundaries), len(boundaries)) }) } @@ -141,8 +142,8 @@ func TestHistogramMerge(t *testing.T) { test.RunProfiles(t, func(t *testing.T, profile test.Profile) { descriptor := test.NewAggregatorTest(metric.ValueRecorderKind, profile.NumberKind) - agg1 := New(descriptor, boundaries[profile.NumberKind]) - agg2 := New(descriptor, boundaries[profile.NumberKind]) + agg1 := histogram.New(descriptor, boundaries) + agg2 := histogram.New(descriptor, boundaries) all := test.NewNumbers(profile.NumberKind) @@ -171,18 +172,21 @@ func TestHistogramMerge(t *testing.T) { asum.CoerceToFloat64(profile.NumberKind), 0.000000001, "Same sum - absolute") - require.Nil(t, err) + require.NoError(t, err) count, err := agg1.Count() require.Equal(t, all.Count(), count, "Same count - absolute") - require.Nil(t, err) + require.NoError(t, err) - require.Equal(t, len(agg1.checkpoint.bucketCounts), len(boundaries[profile.NumberKind])+1, "There should be b + 1 counts, where b is the number of boundaries") + buckets, err := agg1.Histogram() + require.NoError(t, err) + + require.Equal(t, len(buckets.Counts), len(boundaries)+1, "There should be b + 1 counts, where b is the number of boundaries") counts := calcBuckets(all.Points(), profile) for i, v := range counts { - bCount := agg1.checkpoint.bucketCounts[i].AsUint64() - require.Equal(t, v, bCount, "Wrong bucket #%d count: %v != %v", i, counts, agg1.checkpoint.bucketCounts) + bCount := uint64(buckets.Counts[i]) + require.Equal(t, v, bCount, "Wrong bucket #%d count: %v != %v", i, counts, buckets.Counts) } }) } @@ -193,38 +197,37 @@ func TestHistogramNotSet(t *testing.T) { test.RunProfiles(t, func(t *testing.T, profile test.Profile) { descriptor := test.NewAggregatorTest(metric.ValueRecorderKind, profile.NumberKind) - agg := New(descriptor, boundaries[profile.NumberKind]) + agg := histogram.New(descriptor, boundaries) agg.Checkpoint(ctx, descriptor) asum, err := agg.Sum() require.Equal(t, metric.Number(0), asum, "Empty checkpoint sum = 0") - require.Nil(t, err) + require.NoError(t, err) count, err := agg.Count() require.Equal(t, int64(0), count, "Empty checkpoint count = 0") - require.Nil(t, err) + require.NoError(t, err) - require.Equal(t, len(agg.checkpoint.bucketCounts), len(boundaries[profile.NumberKind])+1, "There should be b + 1 counts, where b is the number of boundaries") - for i, bCount := range agg.checkpoint.bucketCounts { - require.Equal(t, uint64(0), bCount.AsUint64(), "Bucket #%d must have 0 observed values", i) + buckets, err := agg.Histogram() + require.NoError(t, err) + + require.Equal(t, len(buckets.Counts), len(boundaries)+1, "There should be b + 1 counts, where b is the number of boundaries") + for i, bCount := range buckets.Counts { + require.Equal(t, uint64(0), uint64(bCount), "Bucket #%d must have 0 observed values", i) } }) } func calcBuckets(points []metric.Number, profile test.Profile) []uint64 { - sortedBoundaries := numbers{ - numbers: make([]metric.Number, len(boundaries[profile.NumberKind])), - kind: profile.NumberKind, - } + sortedBoundaries := make([]float64, len(boundaries)) - copy(sortedBoundaries.numbers, boundaries[profile.NumberKind]) - sort.Sort(&sortedBoundaries) - boundaries := sortedBoundaries.numbers + copy(sortedBoundaries, boundaries) + sort.Float64s(sortedBoundaries) - counts := make([]uint64, len(boundaries)+1) + counts := make([]uint64, len(sortedBoundaries)+1) idx := 0 for _, p := range points { - for idx < len(boundaries) && p.CompareNumber(profile.NumberKind, boundaries[idx]) != -1 { + for idx < len(sortedBoundaries) && p.CoerceToFloat64(profile.NumberKind) >= boundaries[idx] { idx++ } counts[idx]++ diff --git a/sdk/metric/histogram_stress_test.go b/sdk/metric/histogram_stress_test.go index d05536622..f4a1ca687 100644 --- a/sdk/metric/histogram_stress_test.go +++ b/sdk/metric/histogram_stress_test.go @@ -26,7 +26,7 @@ import ( func TestStressInt64Histogram(t *testing.T) { desc := metric.NewDescriptor("some_metric", metric.ValueRecorderKind, metric.Int64NumberKind) - h := histogram.New(&desc, []metric.Number{metric.NewInt64Number(25), metric.NewInt64Number(50), metric.NewInt64Number(75)}) + h := histogram.New(&desc, []float64{25, 50, 75}) ctx, cancelFunc := context.WithCancel(context.Background()) defer cancelFunc() @@ -51,7 +51,7 @@ func TestStressInt64Histogram(t *testing.T) { var realCount int64 for _, c := range b.Counts { - v := c.AsInt64() + v := int64(c) realCount += v } diff --git a/sdk/metric/selector/simple/simple.go b/sdk/metric/selector/simple/simple.go index b64a5e971..a5956e4f3 100644 --- a/sdk/metric/selector/simple/simple.go +++ b/sdk/metric/selector/simple/simple.go @@ -31,7 +31,7 @@ type ( config *ddsketch.Config } selectorHistogram struct { - boundaries []metric.Number + boundaries []float64 } ) @@ -75,7 +75,7 @@ func NewWithExactDistribution() export.AggregationSelector { // histogram, and histogram aggregators for the three kinds of metric. This // selector uses more memory than the NewWithInexpensiveDistribution because it // uses a counter per bucket. -func NewWithHistogramDistribution(boundaries []metric.Number) export.AggregationSelector { +func NewWithHistogramDistribution(boundaries []float64) export.AggregationSelector { return selectorHistogram{boundaries: boundaries} } diff --git a/sdk/metric/selector/simple/simple_test.go b/sdk/metric/selector/simple/simple_test.go index 018d49efa..35b2526b0 100644 --- a/sdk/metric/selector/simple/simple_test.go +++ b/sdk/metric/selector/simple/simple_test.go @@ -56,7 +56,7 @@ func TestExactDistribution(t *testing.T) { } func TestHistogramDistribution(t *testing.T) { - ex := simple.NewWithHistogramDistribution([]metric.Number{}) + ex := simple.NewWithHistogramDistribution(nil) require.NotPanics(t, func() { _ = ex.AggregatorFor(&testCounterDesc).(*sum.Aggregator) }) require.NotPanics(t, func() { _ = ex.AggregatorFor(&testValueRecorderDesc).(*histogram.Aggregator) }) require.NotPanics(t, func() { _ = ex.AggregatorFor(&testValueObserverDesc).(*histogram.Aggregator) }) From 9548817e7eeb5cb3674864b08e82d208f613febc Mon Sep 17 00:00:00 2001 From: jmacd Date: Thu, 21 May 2020 10:45:26 -0700 Subject: [PATCH 3/8] Add a benchmark --- .../aggregator/histogram/benchmark_test.go | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 sdk/metric/aggregator/histogram/benchmark_test.go diff --git a/sdk/metric/aggregator/histogram/benchmark_test.go b/sdk/metric/aggregator/histogram/benchmark_test.go new file mode 100644 index 000000000..7eff730f0 --- /dev/null +++ b/sdk/metric/aggregator/histogram/benchmark_test.go @@ -0,0 +1,129 @@ +// Copyright The OpenTelemetry Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package histogram_test + +import ( + "context" + "math/rand" + "testing" + + "go.opentelemetry.io/otel/api/metric" + "go.opentelemetry.io/otel/sdk/metric/aggregator/histogram" + "go.opentelemetry.io/otel/sdk/metric/aggregator/test" +) + +const inputRange = 1e6 + +func benchmarkHistogramSearchFloat64(b *testing.B, size int) { + boundaries := make([]float64, size) + + for i := range boundaries { + boundaries[i] = rand.Float64() * inputRange + } + + values := make([]float64, b.N) + for i := range values { + values[i] = rand.Float64() * inputRange + } + desc := test.NewAggregatorTest(metric.ValueRecorderKind, metric.Float64NumberKind) + agg := histogram.New(desc, boundaries) + ctx := context.Background() + + b.ReportAllocs() + b.ResetTimer() + + for i := 0; i < b.N; i++ { + agg.Update(ctx, metric.NewFloat64Number(rand.Float64()*inputRange), desc) + } +} + +func BenchmarkHistogramSearchFloat64_1(b *testing.B) { + benchmarkHistogramSearchFloat64(b, 1) +} +func BenchmarkHistogramSearchFloat64_2(b *testing.B) { + benchmarkHistogramSearchFloat64(b, 2) +} +func BenchmarkHistogramSearchFloat64_3(b *testing.B) { + benchmarkHistogramSearchFloat64(b, 3) +} +func BenchmarkHistogramSearchFloat64_4(b *testing.B) { + benchmarkHistogramSearchFloat64(b, 4) +} +func BenchmarkHistogramSearchFloat64_8(b *testing.B) { + benchmarkHistogramSearchFloat64(b, 8) +} +func BenchmarkHistogramSearchFloat64_12(b *testing.B) { + benchmarkHistogramSearchFloat64(b, 12) +} +func BenchmarkHistogramSearchFloat64_16(b *testing.B) { + benchmarkHistogramSearchFloat64(b, 16) +} +func BenchmarkHistogramSearchFloat64_32(b *testing.B) { + benchmarkHistogramSearchFloat64(b, 32) +} +func BenchmarkHistogramSearchFloat64_64(b *testing.B) { + benchmarkHistogramSearchFloat64(b, 64) +} + +func benchmarkHistogramSearchInt64(b *testing.B, size int) { + boundaries := make([]float64, size) + + for i := range boundaries { + boundaries[i] = rand.Float64() * inputRange + } + + values := make([]int64, b.N) + for i := range values { + values[i] = int64(rand.Float64() * inputRange) + } + desc := test.NewAggregatorTest(metric.ValueRecorderKind, metric.Int64NumberKind) + agg := histogram.New(desc, boundaries) + ctx := context.Background() + + b.ReportAllocs() + b.ResetTimer() + + for i := 0; i < b.N; i++ { + agg.Update(ctx, metric.NewInt64Number(int64(rand.Float64()*inputRange)), desc) + } +} + +func BenchmarkHistogramSearchInt64_1(b *testing.B) { + benchmarkHistogramSearchInt64(b, 1) +} +func BenchmarkHistogramSearchInt64_2(b *testing.B) { + benchmarkHistogramSearchInt64(b, 2) +} +func BenchmarkHistogramSearchInt64_3(b *testing.B) { + benchmarkHistogramSearchInt64(b, 3) +} +func BenchmarkHistogramSearchInt64_4(b *testing.B) { + benchmarkHistogramSearchInt64(b, 4) +} +func BenchmarkHistogramSearchInt64_8(b *testing.B) { + benchmarkHistogramSearchInt64(b, 8) +} +func BenchmarkHistogramSearchInt64_12(b *testing.B) { + benchmarkHistogramSearchInt64(b, 12) +} +func BenchmarkHistogramSearchInt64_16(b *testing.B) { + benchmarkHistogramSearchInt64(b, 16) +} +func BenchmarkHistogramSearchInt64_32(b *testing.B) { + benchmarkHistogramSearchInt64(b, 32) +} +func BenchmarkHistogramSearchInt64_64(b *testing.B) { + benchmarkHistogramSearchInt64(b, 64) +} From db993ec5cc63331b3d2e4bb35b942c3c07047b42 Mon Sep 17 00:00:00 2001 From: jmacd Date: Thu, 21 May 2020 10:46:22 -0700 Subject: [PATCH 4/8] Cleanup sort --- sdk/metric/aggregator/histogram/histogram.go | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/sdk/metric/aggregator/histogram/histogram.go b/sdk/metric/aggregator/histogram/histogram.go index bc52f9872..cf964adb6 100644 --- a/sdk/metric/aggregator/histogram/histogram.go +++ b/sdk/metric/aggregator/histogram/histogram.go @@ -66,22 +66,16 @@ var _ aggregator.Histogram = &Aggregator{} func New(desc *metric.Descriptor, boundaries []float64) *Aggregator { // Boundaries MUST be ordered otherwise the histogram could not // be properly computed. - // metric.SortNumbers(desc.NumberKind(), boundaries) - // sortedBoundaries := numbers{ - // numbers: make([]metric.Number, len(boundaries)), - // kind: desc.NumberKind(), - // } - sort.Float64s(boundaries) + sortedBoundaries := make([]float64, len(boundaries)) - // copy(sortedBoundaries.numbers, boundaries) - // sort.Sort(&sortedBoundaries) - // boundaries = sortedBoundaries.numbers + copy(sortedBoundaries, boundaries) + sort.Float64s(sortedBoundaries) return &Aggregator{ kind: desc.NumberKind(), - boundaries: boundaries, - current: emptyState(boundaries), - checkpoint: emptyState(boundaries), + boundaries: sortedBoundaries, + current: emptyState(sortedBoundaries), + checkpoint: emptyState(sortedBoundaries), } } From 2aa0f1496e5c51bbde33010da2227dae4a346678 Mon Sep 17 00:00:00 2001 From: jmacd Date: Thu, 21 May 2020 11:09:10 -0700 Subject: [PATCH 5/8] Comment on linear vs binary search --- .../aggregator/histogram/benchmark_test.go | 48 +++++++++---------- sdk/metric/aggregator/histogram/histogram.go | 22 +++++++-- .../aggregator/histogram/histogram_test.go | 2 +- sdk/metric/aggregator/test/test.go | 2 + 4 files changed, 46 insertions(+), 28 deletions(-) diff --git a/sdk/metric/aggregator/histogram/benchmark_test.go b/sdk/metric/aggregator/histogram/benchmark_test.go index 7eff730f0..628d5ffb0 100644 --- a/sdk/metric/aggregator/histogram/benchmark_test.go +++ b/sdk/metric/aggregator/histogram/benchmark_test.go @@ -52,21 +52,9 @@ func benchmarkHistogramSearchFloat64(b *testing.B, size int) { func BenchmarkHistogramSearchFloat64_1(b *testing.B) { benchmarkHistogramSearchFloat64(b, 1) } -func BenchmarkHistogramSearchFloat64_2(b *testing.B) { - benchmarkHistogramSearchFloat64(b, 2) -} -func BenchmarkHistogramSearchFloat64_3(b *testing.B) { - benchmarkHistogramSearchFloat64(b, 3) -} -func BenchmarkHistogramSearchFloat64_4(b *testing.B) { - benchmarkHistogramSearchFloat64(b, 4) -} func BenchmarkHistogramSearchFloat64_8(b *testing.B) { benchmarkHistogramSearchFloat64(b, 8) } -func BenchmarkHistogramSearchFloat64_12(b *testing.B) { - benchmarkHistogramSearchFloat64(b, 12) -} func BenchmarkHistogramSearchFloat64_16(b *testing.B) { benchmarkHistogramSearchFloat64(b, 16) } @@ -76,6 +64,18 @@ func BenchmarkHistogramSearchFloat64_32(b *testing.B) { func BenchmarkHistogramSearchFloat64_64(b *testing.B) { benchmarkHistogramSearchFloat64(b, 64) } +func BenchmarkHistogramSearchFloat64_128(b *testing.B) { + benchmarkHistogramSearchFloat64(b, 128) +} +func BenchmarkHistogramSearchFloat64_256(b *testing.B) { + benchmarkHistogramSearchFloat64(b, 256) +} +func BenchmarkHistogramSearchFloat64_512(b *testing.B) { + benchmarkHistogramSearchFloat64(b, 512) +} +func BenchmarkHistogramSearchFloat64_1024(b *testing.B) { + benchmarkHistogramSearchFloat64(b, 1024) +} func benchmarkHistogramSearchInt64(b *testing.B, size int) { boundaries := make([]float64, size) @@ -103,21 +103,9 @@ func benchmarkHistogramSearchInt64(b *testing.B, size int) { func BenchmarkHistogramSearchInt64_1(b *testing.B) { benchmarkHistogramSearchInt64(b, 1) } -func BenchmarkHistogramSearchInt64_2(b *testing.B) { - benchmarkHistogramSearchInt64(b, 2) -} -func BenchmarkHistogramSearchInt64_3(b *testing.B) { - benchmarkHistogramSearchInt64(b, 3) -} -func BenchmarkHistogramSearchInt64_4(b *testing.B) { - benchmarkHistogramSearchInt64(b, 4) -} func BenchmarkHistogramSearchInt64_8(b *testing.B) { benchmarkHistogramSearchInt64(b, 8) } -func BenchmarkHistogramSearchInt64_12(b *testing.B) { - benchmarkHistogramSearchInt64(b, 12) -} func BenchmarkHistogramSearchInt64_16(b *testing.B) { benchmarkHistogramSearchInt64(b, 16) } @@ -127,3 +115,15 @@ func BenchmarkHistogramSearchInt64_32(b *testing.B) { func BenchmarkHistogramSearchInt64_64(b *testing.B) { benchmarkHistogramSearchInt64(b, 64) } +func BenchmarkHistogramSearchInt64_128(b *testing.B) { + benchmarkHistogramSearchInt64(b, 128) +} +func BenchmarkHistogramSearchInt64_256(b *testing.B) { + benchmarkHistogramSearchInt64(b, 256) +} +func BenchmarkHistogramSearchInt64_512(b *testing.B) { + benchmarkHistogramSearchInt64(b, 512) +} +func BenchmarkHistogramSearchInt64_1024(b *testing.B) { + benchmarkHistogramSearchInt64(b, 1024) +} diff --git a/sdk/metric/aggregator/histogram/histogram.go b/sdk/metric/aggregator/histogram/histogram.go index cf964adb6..a8e3a76de 100644 --- a/sdk/metric/aggregator/histogram/histogram.go +++ b/sdk/metric/aggregator/histogram/histogram.go @@ -122,10 +122,26 @@ func emptyState(boundaries []float64) state { // Update adds the recorded measurement to the current data set. func (c *Aggregator) Update(_ context.Context, number metric.Number, desc *metric.Descriptor) error { kind := desc.NumberKind() + asFloat := number.CoerceToFloat64(kind) - bucketID := sort.Search(len(c.boundaries), func(i int) bool { - return number.CoerceToFloat64(kind) < c.boundaries[i] - }) + bucketID := len(c.boundaries) + for i, boundary := range c.boundaries { + if asFloat < boundary { + bucketID = i + break + } + } + // Note: Binary-search was compared using the benchmarks. The following + // code is equivalent to the linear search above: + // + // bucketID := sort.Search(len(c.boundaries), func(i int) bool { + // return asFloat < c.boundaries[i] + // }) + // + // The binary search wins for very large boundary sets, but + // the linear search performs better up through arrays between + // 256 and 512 elements, which is a relatively large histogram, so we + // continue to prefer linear search. c.lock.Lock() defer c.lock.Unlock() diff --git a/sdk/metric/aggregator/histogram/histogram_test.go b/sdk/metric/aggregator/histogram/histogram_test.go index a34b72486..13a1fcb52 100644 --- a/sdk/metric/aggregator/histogram/histogram_test.go +++ b/sdk/metric/aggregator/histogram/histogram_test.go @@ -227,7 +227,7 @@ func calcBuckets(points []metric.Number, profile test.Profile) []uint64 { counts := make([]uint64, len(sortedBoundaries)+1) idx := 0 for _, p := range points { - for idx < len(sortedBoundaries) && p.CoerceToFloat64(profile.NumberKind) >= boundaries[idx] { + for idx < len(sortedBoundaries) && p.CoerceToFloat64(profile.NumberKind) >= sortedBoundaries[idx] { idx++ } counts[idx]++ diff --git a/sdk/metric/aggregator/test/test.go b/sdk/metric/aggregator/test/test.go index edb4a1d73..53cf31959 100644 --- a/sdk/metric/aggregator/test/test.go +++ b/sdk/metric/aggregator/test/test.go @@ -81,6 +81,8 @@ func TestMain(m *testing.M) { os.Exit(m.Run()) } +// TODO: Expose Numbers in api/metric for sorting support + type Numbers struct { // numbers has to be aligned for 64-bit atomic operations. numbers []metric.Number From 17b8543050192989a17685b3a2fdf77736bed411 Mon Sep 17 00:00:00 2001 From: jmacd Date: Thu, 21 May 2020 11:18:47 -0700 Subject: [PATCH 6/8] Lint --- sdk/metric/aggregator/histogram/benchmark_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/metric/aggregator/histogram/benchmark_test.go b/sdk/metric/aggregator/histogram/benchmark_test.go index 628d5ffb0..7b99b725d 100644 --- a/sdk/metric/aggregator/histogram/benchmark_test.go +++ b/sdk/metric/aggregator/histogram/benchmark_test.go @@ -45,7 +45,7 @@ func benchmarkHistogramSearchFloat64(b *testing.B, size int) { b.ResetTimer() for i := 0; i < b.N; i++ { - agg.Update(ctx, metric.NewFloat64Number(rand.Float64()*inputRange), desc) + _ = agg.Update(ctx, metric.NewFloat64Number(rand.Float64()*inputRange), desc) } } @@ -96,7 +96,7 @@ func benchmarkHistogramSearchInt64(b *testing.B, size int) { b.ResetTimer() for i := 0; i < b.N; i++ { - agg.Update(ctx, metric.NewInt64Number(int64(rand.Float64()*inputRange)), desc) + _ = agg.Update(ctx, metric.NewInt64Number(int64(rand.Float64()*inputRange)), desc) } } From 9d2e78ae2b2402202078808174858d7804b2969b Mon Sep 17 00:00:00 2001 From: jmacd Date: Thu, 21 May 2020 11:21:13 -0700 Subject: [PATCH 7/8] Comment --- sdk/export/metric/aggregator/aggregator.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/sdk/export/metric/aggregator/aggregator.go b/sdk/export/metric/aggregator/aggregator.go index 52fe13435..581760412 100644 --- a/sdk/export/metric/aggregator/aggregator.go +++ b/sdk/export/metric/aggregator/aggregator.go @@ -68,8 +68,14 @@ type ( // For a Histogram with N defined boundaries, e.g, [x, y, z]. // There are N+1 counts: [-inf, x), [x, y), [y, z), [z, +inf] Buckets struct { + // Boundaries are floating point numbers, even when + // aggregating integers. Boundaries []float64 - Counts []float64 + + // Counts are floating point numbers to account for + // the possibility of sampling which allows for + // non-integer count values. + Counts []float64 } // Histogram returns the count of events in pre-determined buckets. From 4f3188ab955ea1021d2664d4f7be2674dbc526df Mon Sep 17 00:00:00 2001 From: jmacd Date: Thu, 21 May 2020 11:36:52 -0700 Subject: [PATCH 8/8] Fix use of values in benchmark --- sdk/metric/aggregator/histogram/benchmark_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sdk/metric/aggregator/histogram/benchmark_test.go b/sdk/metric/aggregator/histogram/benchmark_test.go index 7b99b725d..ff99cbdf6 100644 --- a/sdk/metric/aggregator/histogram/benchmark_test.go +++ b/sdk/metric/aggregator/histogram/benchmark_test.go @@ -45,7 +45,7 @@ func benchmarkHistogramSearchFloat64(b *testing.B, size int) { b.ResetTimer() for i := 0; i < b.N; i++ { - _ = agg.Update(ctx, metric.NewFloat64Number(rand.Float64()*inputRange), desc) + _ = agg.Update(ctx, metric.NewFloat64Number(values[i]), desc) } } @@ -96,7 +96,7 @@ func benchmarkHistogramSearchInt64(b *testing.B, size int) { b.ResetTimer() for i := 0; i < b.N; i++ { - _ = agg.Update(ctx, metric.NewInt64Number(int64(rand.Float64()*inputRange)), desc) + _ = agg.Update(ctx, metric.NewInt64Number(values[i]), desc) } }