From 12a49e59102f0bec0d25aa6f0ecd96d73f044bc3 Mon Sep 17 00:00:00 2001 From: Lenin Jaganathan <32874349+lenin-jaganathan@users.noreply.github.com> Date: Mon, 7 Oct 2024 06:45:52 +0530 Subject: [PATCH] Reduce memory footprint of StepBucketHistogram (#4955) A FixedBoundaryHistogram is used by StepBucketHistogram, so there is no need for StepBucketHistogram to maintain a field of the buckets itself. This changes to get the buckets from the underlying FixedBoundaryHistogram when needed. See gh-4954 --- .../distribution/FixedBoundaryHistogram.java | 67 +++++++++++--- .../distribution/StepBucketHistogram.java | 20 ++--- .../FixedBoundaryHistogramTest.java | 89 +++++++++++++++++++ ...Test.java => StepBucketHistogramTest.java} | 2 +- 4 files changed, 150 insertions(+), 28 deletions(-) create mode 100644 micrometer-core/src/test/java/io/micrometer/core/instrument/distribution/FixedBoundaryHistogramTest.java rename micrometer-core/src/test/java/io/micrometer/core/instrument/distribution/{StepHistogramTest.java => StepBucketHistogramTest.java} (99%) diff --git a/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/FixedBoundaryHistogram.java b/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/FixedBoundaryHistogram.java index 3f6e0a5d2d..d740c3a21f 100644 --- a/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/FixedBoundaryHistogram.java +++ b/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/FixedBoundaryHistogram.java @@ -27,14 +27,32 @@ class FixedBoundaryHistogram { private final boolean isCumulativeBucketCounts; + /** + * Creates a FixedBoundaryHistogram which tracks the count of values for each bucket + * bound). + * @param buckets - sorted bucket boundaries + * @param isCumulativeBucketCounts - whether the count values should be cumulative + * count of lower buckets and current bucket. + */ FixedBoundaryHistogram(double[] buckets, boolean isCumulativeBucketCounts) { this.buckets = buckets; this.values = new AtomicLongArray(buckets.length); this.isCumulativeBucketCounts = isCumulativeBucketCounts; } - long countAtValue(double value) { - int index = Arrays.binarySearch(buckets, value); + double[] getBuckets() { + return this.buckets; + } + + /** + * Returns the number of values that was recorded between previous bucket and the + * queried bucket (upper bound inclusive) + * @param bucket - the bucket to find values for + * @return 0 if bucket is not a valid bucket otherwise number of values recorded + * between (index(bucket) - 1, bucket] + */ + private long countAtBucket(double bucket) { + int index = Arrays.binarySearch(buckets, bucket); if (index < 0) return 0; return values.get(index); @@ -53,18 +71,20 @@ void record(long value) { } /** - * The least bucket that is less than or equal to a sample. + * The least bucket that is less than or equal to a valueToRecord. Returns -1, if the + * valueToRecord is greater than the highest bucket. */ - int leastLessThanOrEqualTo(double key) { + // VisibleForTesting + int leastLessThanOrEqualTo(long valueToRecord) { int low = 0; int high = buckets.length - 1; while (low <= high) { int mid = (low + high) >>> 1; - double value = buckets[mid]; - if (value < key) + double bucket = buckets[mid]; + if (bucket < valueToRecord) low = mid + 1; - else if (value > key) + else if (bucket > valueToRecord) high = mid - 1; else return mid; // exact match @@ -73,28 +93,49 @@ else if (value > key) return low < buckets.length ? low : -1; } - Iterator countsAtValues(Iterator values) { + Iterator countsAtValues(Iterator buckets) { return new Iterator() { private double cumulativeCount = 0.0; @Override public boolean hasNext() { - return values.hasNext(); + return buckets.hasNext(); } @Override public CountAtBucket next() { - double value = values.next(); - double count = countAtValue(value); + double bucket = buckets.next(); + double count = countAtBucket(bucket); if (isCumulativeBucketCounts) { cumulativeCount += count; - return new CountAtBucket(value, cumulativeCount); + return new CountAtBucket(bucket, cumulativeCount); } else { - return new CountAtBucket(value, count); + return new CountAtBucket(bucket, count); } } }; } + /** + * Returns the list of {@link CountAtBucket} for each of the buckets tracked by this + * histogram. + */ + CountAtBucket[] getCountsAtBucket() { + CountAtBucket[] countAtBuckets = new CountAtBucket[this.buckets.length]; + long cumulativeCount = 0; + + for (int i = 0; i < this.buckets.length; i++) { + final long valueAtCurrentBucket = values.get(i); + if (isCumulativeBucketCounts) { + cumulativeCount += valueAtCurrentBucket; + countAtBuckets[i] = new CountAtBucket(buckets[i], cumulativeCount); + } + else { + countAtBuckets[i] = new CountAtBucket(buckets[i], valueAtCurrentBucket); + } + } + return countAtBuckets; + } + } diff --git a/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/StepBucketHistogram.java b/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/StepBucketHistogram.java index b5d15e52ca..5ce563def1 100644 --- a/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/StepBucketHistogram.java +++ b/micrometer-core/src/main/java/io/micrometer/core/instrument/distribution/StepBucketHistogram.java @@ -19,8 +19,6 @@ import io.micrometer.core.instrument.config.InvalidConfigurationException; import io.micrometer.core.instrument.step.StepValue; -import java.util.Arrays; -import java.util.Iterator; import java.util.NavigableSet; import java.util.Objects; import java.util.function.Supplier; @@ -36,16 +34,14 @@ public class StepBucketHistogram extends StepValue implements H private final FixedBoundaryHistogram fixedBoundaryHistogram; - private final double[] buckets; - public StepBucketHistogram(Clock clock, long stepMillis, DistributionStatisticConfig distributionStatisticConfig, boolean supportsAggregablePercentiles, boolean isCumulativeBucketCounts) { super(clock, stepMillis, getEmptyCounts( getBucketsFromDistributionStatisticConfig(distributionStatisticConfig, supportsAggregablePercentiles))); - this.buckets = getBucketsFromDistributionStatisticConfig(distributionStatisticConfig, - supportsAggregablePercentiles); - this.fixedBoundaryHistogram = new FixedBoundaryHistogram(buckets, isCumulativeBucketCounts); + this.fixedBoundaryHistogram = new FixedBoundaryHistogram( + getBucketsFromDistributionStatisticConfig(distributionStatisticConfig, supportsAggregablePercentiles), + isCumulativeBucketCounts); } @Override @@ -66,13 +62,9 @@ public HistogramSnapshot takeSnapshot(long count, double total, double max) { @Override protected Supplier valueSupplier() { return () -> { - CountAtBucket[] countAtBuckets = new CountAtBucket[buckets.length]; + CountAtBucket[] countAtBuckets; synchronized (fixedBoundaryHistogram) { - final Iterator iterator = fixedBoundaryHistogram - .countsAtValues(Arrays.stream(buckets).iterator()); - for (int i = 0; i < countAtBuckets.length; i++) { - countAtBuckets[i] = iterator.next(); - } + countAtBuckets = fixedBoundaryHistogram.getCountsAtBucket(); fixedBoundaryHistogram.reset(); } return countAtBuckets; @@ -81,7 +73,7 @@ protected Supplier valueSupplier() { @Override protected CountAtBucket[] noValue() { - return getEmptyCounts(buckets); + return getEmptyCounts(this.fixedBoundaryHistogram.getBuckets()); } private static CountAtBucket[] getEmptyCounts(double[] buckets) { diff --git a/micrometer-core/src/test/java/io/micrometer/core/instrument/distribution/FixedBoundaryHistogramTest.java b/micrometer-core/src/test/java/io/micrometer/core/instrument/distribution/FixedBoundaryHistogramTest.java new file mode 100644 index 0000000000..80126ab7f4 --- /dev/null +++ b/micrometer-core/src/test/java/io/micrometer/core/instrument/distribution/FixedBoundaryHistogramTest.java @@ -0,0 +1,89 @@ +/* + * Copyright 2024 VMware, Inc. + * + * 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 + * + * https://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 io.micrometer.core.instrument.distribution; + +import static org.assertj.core.api.Assertions.assertThat; + +import java.util.stream.Stream; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +class FixedBoundaryHistogramTest { + + private static final double[] BUCKET_BOUNDS = new double[] { 1, 10, 100 }; + + private FixedBoundaryHistogram fixedBoundaryHistogram; + + @BeforeEach + void setup() { + fixedBoundaryHistogram = new FixedBoundaryHistogram(BUCKET_BOUNDS, false); + } + + @Test + void testGetBuckets() { + assertThat(fixedBoundaryHistogram.getBuckets()).containsExactly(BUCKET_BOUNDS); + } + + @ParameterizedTest + @MethodSource("valuedIndexProvider") + void testLeastLessThanOrEqualTo(long value, int expectedIndex) { + assertThat(fixedBoundaryHistogram.leastLessThanOrEqualTo(value)).isEqualTo(expectedIndex); + } + + private static Stream valuedIndexProvider() { + return Stream.of(Arguments.of(0, 0), Arguments.of(1, 0), Arguments.of(2, 1), Arguments.of(5, 1), + Arguments.of(10, 1), Arguments.of(11, 2), Arguments.of(90, 2), Arguments.of(100, 2), + Arguments.of(101, -1), Arguments.of(Long.MAX_VALUE, -1)); + } + + @Test + void testReset() { + fixedBoundaryHistogram.record(1); + fixedBoundaryHistogram.record(10); + fixedBoundaryHistogram.record(100); + assertThat(fixedBoundaryHistogram.getCountsAtBucket()).allMatch(countAtBucket -> countAtBucket.count() == 1); + fixedBoundaryHistogram.reset(); + assertThat(fixedBoundaryHistogram.getCountsAtBucket()).allMatch(countAtBucket -> countAtBucket.count() == 0); + } + + @Test + void testCountsAtBucket() { + fixedBoundaryHistogram.record(1); + fixedBoundaryHistogram.record(10); + fixedBoundaryHistogram.record(100); + assertThat(fixedBoundaryHistogram.getCountsAtBucket()).allMatch(countAtBucket -> countAtBucket.count() == 1); + fixedBoundaryHistogram.reset(); + assertThat(fixedBoundaryHistogram.getCountsAtBucket()).allMatch(countAtBucket -> countAtBucket.count() == 0); + fixedBoundaryHistogram.record(0); + assertThat(fixedBoundaryHistogram.getCountsAtBucket()).containsExactly(new CountAtBucket(1.0, 1), + new CountAtBucket(10.0, 0), new CountAtBucket(100.0, 0)); + } + + @Test + void testCumulativeCounts() { + fixedBoundaryHistogram = new FixedBoundaryHistogram(BUCKET_BOUNDS, true); + fixedBoundaryHistogram.record(1); + fixedBoundaryHistogram.record(10); + fixedBoundaryHistogram.record(100); + assertThat(fixedBoundaryHistogram.getCountsAtBucket()).containsExactly(new CountAtBucket(1.0, 1), + new CountAtBucket(10.0, 2), new CountAtBucket(100.0, 3)); + } + +} diff --git a/micrometer-core/src/test/java/io/micrometer/core/instrument/distribution/StepHistogramTest.java b/micrometer-core/src/test/java/io/micrometer/core/instrument/distribution/StepBucketHistogramTest.java similarity index 99% rename from micrometer-core/src/test/java/io/micrometer/core/instrument/distribution/StepHistogramTest.java rename to micrometer-core/src/test/java/io/micrometer/core/instrument/distribution/StepBucketHistogramTest.java index e972d32788..3442f496ba 100644 --- a/micrometer-core/src/test/java/io/micrometer/core/instrument/distribution/StepHistogramTest.java +++ b/micrometer-core/src/test/java/io/micrometer/core/instrument/distribution/StepBucketHistogramTest.java @@ -23,7 +23,7 @@ import static org.assertj.core.api.Assertions.assertThat; -class StepHistogramTest { +class StepBucketHistogramTest { MockClock clock = new MockClock();