Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reduce memory footprint of StepBucketHistogram #4955

Open
wants to merge 2 commits into
base: 1.11.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
Expand All @@ -73,28 +93,49 @@ else if (value > key)
return low < buckets.length ? low : -1;
}

Iterator<CountAtBucket> countsAtValues(Iterator<Double> values) {
Iterator<CountAtBucket> countsAtValues(Iterator<Double> buckets) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just use getCountsAtBucket() instead of this? This makes a call to countAtBucket() for every bucket which in turn does a binary search to find the bucket. It makes sense if we need to get the counts for only a subset of buckets, but we don't do that anywhere (why would we even do that?). But at the end, all we need is a continuous list of buckets and their count which we can get by iterating on the values directly.

There were a few issues solved around this area hence not changing anything at this point. But,
my claim would be the buckets would not change after the histogram is created so we should always rely on the buckets from the underlying implementation (in this case FixedBucketHistogram).

return new Iterator<CountAtBucket>() {
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;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -36,16 +34,14 @@ public class StepBucketHistogram extends StepValue<CountAtBucket[]> 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
Expand All @@ -66,13 +62,9 @@ public HistogramSnapshot takeSnapshot(long count, double total, double max) {
@Override
protected Supplier<CountAtBucket[]> valueSupplier() {
return () -> {
CountAtBucket[] countAtBuckets = new CountAtBucket[buckets.length];
CountAtBucket[] countAtBuckets;
synchronized (fixedBoundaryHistogram) {
final Iterator<CountAtBucket> 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;
Expand All @@ -81,7 +73,7 @@ protected Supplier<CountAtBucket[]> valueSupplier() {

@Override
protected CountAtBucket[] noValue() {
return getEmptyCounts(buckets);
return getEmptyCounts(this.fixedBoundaryHistogram.getBuckets());
}

private static CountAtBucket[] getEmptyCounts(double[] buckets) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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<Arguments> 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));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@

import static org.assertj.core.api.Assertions.assertThat;

class StepHistogramTest {
class StepBucketHistogramTest {

MockClock clock = new MockClock();

Expand Down