Refactored M3 reporting to remove any unnecessary allocations

core/src/main/java/com/uber/m3/tally/AbstractBuckets.java

@@ -21,30 +21,39 @@
 package com.uber.m3.tally;
 
 import com.uber.m3.util.Duration;
+import com.uber.m3.util.ImmutableList;
 
-import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Iterator;
 import java.util.List;
 import java.util.ListIterator;
 
 /**
- * Abstract {@link Buckets} implementation for common functionality.
+ * @deprecated DO NOT USE
+ *
+ * Please use {@link ImmutableBuckets} instead
  */
-public abstract class AbstractBuckets<T> implements Buckets<T> {
+@Deprecated
+public abstract class AbstractBuckets<T extends Comparable<T>> implements Buckets<T> {
     protected List<T> buckets;
 
     AbstractBuckets(T[] buckets) {
         if (buckets == null) {
-            this.buckets = new ArrayList<>();
-        } else {
-            this.buckets = new ArrayList<>(Arrays.asList(buckets));
+            throw new IllegalArgumentException("provided buckets could not be null");
         }
+
+        validate(buckets);
+
+        this.buckets = new ImmutableList<>(Arrays.asList(buckets));
     }
 
-    AbstractBuckets() {
-        this(null);
+    public void validate(T[] buckets) {
+        for (int i = 1; i < buckets.length; ++i) {
+            if (buckets[i - 1].compareTo(buckets[i]) > 0) {
+                throw new IllegalArgumentException("buckets should be in a non-decreasing order");
+            }
+        }
     }
 
     @Override

core/src/main/java/com/uber/m3/tally/BucketPairImpl.java

@@ -22,12 +22,10 @@
 
 import com.uber.m3.util.Duration;
 
-import java.util.Collections;
-
 /**
  * Default implementation of a {@link BucketPair}
  *
- * @deprecated will be removed
+ * @deprecated DO NOT USE, WILL BE REMOVED IN THE NEXT VERSION
  */
 @Deprecated
 public class BucketPairImpl implements BucketPair {
@@ -60,8 +58,6 @@ public static BucketPair[] bucketPairs(Buckets buckets) {
             };
         }
 
-        Collections.sort(buckets);
-
         Double[] asValueBuckets = buckets.asValues();
         Duration[] asDurationBuckets = buckets.asDurations();
         BucketPair[] pairs = new BucketPair[buckets.size() + 1];

core/src/main/java/com/uber/m3/tally/Buckets.java

@@ -25,18 +25,22 @@
 import java.util.List;
 
 /**
- * Buckets of a {@link Histogram}.
+ * @deprecated DO NOT USE
+ *
+ * Please use {@link ImmutableBuckets} instead
  */
-public interface Buckets<E> extends List<E> {
+@Deprecated
+public interface Buckets<E> extends ImmutableBuckets, List<E> {
     /**
-     * Returns these buckets as {@code double}s.
-     * @return an array of {@code double}s representing these buckets
+     * @deprecated DO NOT USE
      */
+    @Deprecated
     Double[] asValues();
 
     /**
-     * Returns these buckets as {@link Duration}s.
-     * @return an array of {@link Duration}s representing these buckets
+     * @deprecated DO NOT USE
      */
+    @Deprecated
     Duration[] asDurations();
+
 }

core/src/main/java/com/uber/m3/tally/DurationBuckets.java

@@ -22,18 +22,62 @@
 
 import com.uber.m3.util.Duration;
 
+import java.util.Collections;
+import java.util.List;
+
 /**
  * {@link Buckets} implementation backed by {@link Duration}s.
  */
 public class DurationBuckets extends AbstractBuckets<Duration> {
+
     public DurationBuckets(Duration[] durations) {
         super(durations);
     }
 
-    public DurationBuckets() {
-        super();
+    @Override
+    public Duration getDurationLowerBoundFor(int bucketIndex) {
+        return bucketIndex == 0 ? Duration.MIN_VALUE : buckets.get(bucketIndex - 1);
+    }
+
+    @Override
+    public Duration getDurationUpperBoundFor(int bucketIndex) {
+        return bucketIndex < buckets.size() ? buckets.get(bucketIndex) : Duration.MAX_VALUE;
+    }
+
+    @Override
+    public int getBucketIndexFor(Duration value) {
+        return HistogramImpl.toBucketIndex(Collections.binarySearch(buckets, value));
+    }
+
+    @Override
+    public List<Duration> getDurationUpperBounds() {
+        return Collections.unmodifiableList(buckets);
+    }
+
+    @Override
+    public double getValueLowerBoundFor(int bucketIndex) {
+        throw new UnsupportedOperationException("not supported");
+    }
+
+    @Override
+    public double getValueUpperBoundFor(int bucketIndex) {
+        throw new UnsupportedOperationException("not supported");
+    }
+
+    @Override
+    public int getBucketIndexFor(double value) {
+        throw new UnsupportedOperationException("not supported");
     }
 
+    @Override
+    public List<Double> getValueUpperBounds() {
+        throw new UnsupportedOperationException("not supported");
+    }
+
+    /**
+     * @deprecated DO NOT USE
+     */
+    @Deprecated
     @Override
     public Double[] asValues() {
         Double[] values = new Double[buckets.size()];
@@ -45,6 +89,10 @@ public Double[] asValues() {
         return values;
     }
 
+    /**
+     * @deprecated DO NOT USE
+     */
+    @Deprecated
     @Override
     public Duration[] asDurations() {
         return buckets.toArray(new Duration[buckets.size()]);

core/src/main/java/com/uber/m3/tally/HistogramImpl.java

@@ -23,25 +23,25 @@
 import com.uber.m3.util.Duration;
 import com.uber.m3.util.ImmutableMap;
 
-import java.util.Arrays;
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.List;
 import java.util.Map;
 
 /**
  * Default implementation of a {@link Histogram}.
  */
 class HistogramImpl extends MetricBase implements Histogram, StopwatchRecorder {
     private final Type type;
+
     private final ImmutableMap<String, String> tags;
-    private final Buckets specification;
+
+    private final ImmutableBuckets specification;
 
     // NOTE: Bucket counters are lazily initialized. Since ref updates are atomic in JMM,
     // no dedicated synchronization is used on the read path, only on the write path
     private final CounterImpl[] bucketCounters;
 
-    private final double[] lookupByValue;
-    private final Duration[] lookupByDuration;
-
     private final ScopeImpl scope;
 
     HistogramImpl(
@@ -60,25 +60,17 @@ class HistogramImpl extends MetricBase implements Histogram, StopwatchRecorder {
         // Each bucket value, serves as a boundary de-marking upper bound
         // for the bucket to the left, and lower bound for the bucket to the right
         this.bucketCounters = new CounterImpl[buckets.asValues().length + 1];
-
-        this.lookupByValue =
-                Arrays.stream(buckets.asValues())
-                    .mapToDouble(x -> x)
-                    .toArray();
-
-        this.lookupByDuration =
-                Arrays.copyOf(buckets.asDurations(), buckets.asDurations().length);
     }
 
     @Override
     public void recordValue(double value) {
-        int index = toBucketIndex(Arrays.binarySearch(lookupByValue, value));
+        int index = toBucketIndex(Collections.binarySearch(specification.getValueUpperBounds(), value));
         getOrCreateCounter(index).inc(1);
     }
 
     @Override
     public void recordDuration(Duration duration) {
-        int index = toBucketIndex(Arrays.binarySearch(lookupByDuration, duration));
+        int index = toBucketIndex(Collections.binarySearch(specification.getDurationUpperBounds(), duration));
         getOrCreateCounter(index).inc(1);
     }
 
@@ -87,9 +79,14 @@ private CounterImpl getOrCreateCounter(int index) {
             return bucketCounters[index];
         }
 
+        List<?> bucketsBounds =
+                this.type == Type.VALUE
+                        ? specification.getValueUpperBounds()
+                        : specification.getDurationUpperBounds();
+
         // To maintain lock granularity we synchronize only on a
         // particular bucket leveraging bucket's boundary as a sync target
-        synchronized (lookupByDuration[Math.min(index, lookupByDuration.length - 1)]) {
+        synchronized (bucketsBounds.get(Math.min(index, bucketsBounds.size() - 1))) {
             // Check whether bucket has been already set,
             // while we were waiting for lock
             if (bucketCounters[index] != null) {
@@ -101,7 +98,7 @@ private CounterImpl getOrCreateCounter(int index) {
         }
     }
 
-    private int toBucketIndex(int binarySearchResult) {
+    static int toBucketIndex(int binarySearchResult) {
         // Buckets are defined in the following way:
         //      - Each bucket is inclusive of its lower bound, and exclusive of the upper: [lower, upper)
         //      - All buckets are defined by upper bounds: [2, 4, 8, 16, 32, ...]: therefore i
@@ -133,19 +130,19 @@ ImmutableMap<String, String> getTags() {
     }
 
     private Duration getUpperBoundDurationForBucket(int bucketIndex) {
-        return bucketIndex < lookupByDuration.length ? lookupByDuration[bucketIndex] : Duration.MAX_VALUE;
+        return bucketIndex < specification.getDurationUpperBounds().size() ? specification.getDurationUpperBounds().get(bucketIndex) : Duration.MAX_VALUE;
     }
 
-    private double getUpperBoundValueForBucket(int bucketIndex) {
-        return bucketIndex < lookupByValue.length ? lookupByValue[bucketIndex] : Double.MAX_VALUE;
+    private Duration getLowerBoundDurationForBucket(int bucketIndex) {
+        return bucketIndex == 0 ? Duration.MIN_VALUE : specification.getDurationUpperBounds().get(bucketIndex - 1);
     }
 
-    private Duration getLowerBoundDurationForBucket(int bucketIndex) {
-        return bucketIndex == 0 ? Duration.MIN_VALUE : lookupByDuration[bucketIndex - 1];
+    private double getUpperBoundValueForBucket(int bucketIndex) {
+        return bucketIndex < specification.getValueUpperBounds().size() ? specification.getValueUpperBounds().get(bucketIndex) : Double.MAX_VALUE;
     }
 
     private double getLowerBoundValueForBucket(int bucketIndex) {
-        return bucketIndex == 0 ? Double.MIN_VALUE : lookupByValue[bucketIndex - 1];
+        return bucketIndex == 0 ? Double.MIN_VALUE : specification.getValueUpperBounds().get(bucketIndex - 1);
     }
 
     private long getCounterValue(int index) {
@@ -218,7 +215,7 @@ public void report(ImmutableMap<String, String> tags, StatsReporter reporter) {
                     reporter.reportHistogramValueSamples(
                         getQualifiedName(),
                         tags,
-                        specification,
+                        (Buckets) specification,
                         getLowerBoundValueForBucket(bucketIndex),
                         getUpperBoundValueForBucket(bucketIndex),
                         inc
@@ -228,7 +225,7 @@ public void report(ImmutableMap<String, String> tags, StatsReporter reporter) {
                     reporter.reportHistogramDurationSamples(
                         getQualifiedName(),
                         tags,
-                        specification,
+                        (Buckets) specification,
                         getLowerBoundDurationForBucket(bucketIndex),
                         getUpperBoundDurationForBucket(bucketIndex),
                         inc

core/src/main/java/com/uber/m3/tally/ImmutableBuckets.java

@@ -0,0 +1,87 @@
+// Copyright (c) 2020 Uber Technologies, Inc.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in
+// all copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+// THE SOFTWARE.
+
+package com.uber.m3.tally;
+
+import com.uber.m3.util.Duration;
+
+import java.util.List;
+
+/**
+ * Abstracts buckets used in {@link Histogram} metrics,
+ *
+ * Buckets are defined by the list of upper-bounds in the following way:
+ *
+ *  <blockquote>
+ *  <pre>
+ *      double bounds[] = new double[] { 1, 2, 4, 8, 16 };
+ *
+ *      // For the given set of bounds:
+ *      //    first bucket      [-inf, 1)
+ *      //    second bucket     [1, 2)
+ *      //    ...
+ *      //    last bucket       [16, +inf)
+ *  </pre>
+ *  </blockquote>
+ */
+public interface ImmutableBuckets {
+
+    /**
+     * Gets corresponding bucket lower bound
+     */
+    double getValueLowerBoundFor(int bucketIndex);
+
+    /**
+     * Gets corresponding bucket upper bound
+     */
+    double getValueUpperBoundFor(int bucketIndex);
+
+    /**
+     * Gets corresponding bucket lower bound
+     */
+    Duration getDurationLowerBoundFor(int bucketIndex);
+
+    /**
+     * Gets corresponding bucket upper bound
+     */
+    Duration getDurationUpperBoundFor(int bucketIndex);
+
+    /**
+     * Gets index of the corresponding bucket this value would fall under
+     */
+    int getBucketIndexFor(double value);
+
+    /**
+     * Gets index of the corresponding bucket this value would fall under
+     */
+    int getBucketIndexFor(Duration value);
+
+    /**
+     * Returns defined buckets' upper-bound values as {@link Double}s.
+     * @return an immutable list of {@code double}s representing these buckets
+     */
+    List<Double> getValueUpperBounds();
+
+    /**
+     * Returns defined buckets' upper-bound values as {@link Duration}s.
+     * @return an immutable list of {@link Duration}s representing these buckets
+     */
+    List<Duration> getDurationUpperBounds();
+}