From 6a083b50b41c3859dcd9b6d9211effd16b28c9d3 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Thu, 2 Oct 2025 11:40:35 +0200 Subject: [PATCH 1/3] Optimize exponential histogram builder for construction in order --- .../ExponentialHistogram.java | 14 ++ .../ExponentialHistogramBuilder.java | 195 +++++++++++++---- .../FixedCapacityExponentialHistogram.java | 36 +++- .../ExponentialHistogramBuilderTests.java | 197 ++++++++++++++++-- 4 files changed, 378 insertions(+), 64 deletions(-) diff --git a/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogram.java b/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogram.java index 3bb3856f0e97a..d8b0887c10cd0 100644 --- a/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogram.java +++ b/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogram.java @@ -144,6 +144,20 @@ interface Buckets { */ long valueCount(); + /** + * Returns the number of buckets. Note that this operation might require iterating over all buckets, and therefore is not cheap. + * @return the number of buckets + */ + default int bucketCount() { + int count = 0; + BucketIterator it = iterator(); + while (it.hasNext()) { + count++; + it.advance(); + } + return count; + } + } /** diff --git a/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramBuilder.java b/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramBuilder.java index 5ed3f37f16476..9691a82ce9afe 100644 --- a/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramBuilder.java +++ b/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramBuilder.java @@ -21,15 +21,17 @@ package org.elasticsearch.exponentialhistogram; +import org.elasticsearch.core.Releasable; import org.elasticsearch.core.Releasables; import java.util.TreeMap; /** * A builder for building a {@link ReleasableExponentialHistogram} directly from buckets. - * Note that this class is not optimized regarding memory allocations or performance, so it is not intended for high-throughput usage. */ -public class ExponentialHistogramBuilder { +public class ExponentialHistogramBuilder implements Releasable { + + private static final int DEFAULT_ESTIMATED_BUCKET_COUNT = 32; private final ExponentialHistogramCircuitBreaker breaker; @@ -39,8 +41,16 @@ public class ExponentialHistogramBuilder { private Double min; private Double max; - private final TreeMap negativeBuckets = new TreeMap<>(); - private final TreeMap positiveBuckets = new TreeMap<>(); + private int estimatedBucketCount = DEFAULT_ESTIMATED_BUCKET_COUNT; + + // If the buckets are provided in order, we directly build the histogram to avoid unnecessary copies and allocations + // If a bucket is received out of order, we fallback to storing the buckets in the TreeMaps and build the histogram at the end. + private FixedCapacityExponentialHistogram result; + // Visible for testing to ensure that the low-allocation path is taken for ordered buckets + TreeMap negativeBuckets; + TreeMap positiveBuckets; + + private boolean resultAlreadyReturned = false; ExponentialHistogramBuilder(int scale, ExponentialHistogramCircuitBreaker breaker) { this.breaker = breaker; @@ -53,6 +63,7 @@ public class ExponentialHistogramBuilder { sum(toCopy.sum()); min(toCopy.min()); max(toCopy.max()); + estimatedBucketCount(toCopy.negativeBuckets().bucketCount() + toCopy.positiveBuckets().bucketCount()); BucketIterator negBuckets = toCopy.negativeBuckets().iterator(); while (negBuckets.hasNext()) { setNegativeBucket(negBuckets.peekIndex(), negBuckets.peekCount()); @@ -65,6 +76,19 @@ public class ExponentialHistogramBuilder { } } + /** + * If known, sets the estimated total number of buckets to minimize unnecessary allocations. + * Only has an effect if invoked before the first call to + * {@link #setPositiveBucket(long, long)} and {@link #setNegativeBucket(long, long)}. + * + * @param totalBuckets the total number of buckets expected to be added + * @return the builder + */ + public ExponentialHistogramBuilder estimatedBucketCount(int totalBuckets) { + estimatedBucketCount = totalBuckets; + return this; + } + public ExponentialHistogramBuilder scale(int scale) { this.scale = scale; return this; @@ -106,69 +130,160 @@ public ExponentialHistogramBuilder max(double max) { } /** - * Sets the given bucket of the positive buckets. - * Buckets may be set in arbitrary order. If the bucket already exists, it will be replaced. + * Sets the given bucket of the positive buckets. If the bucket already exists, it will be replaced. + * Buckets may be set in arbitrary order. However, for best performance and minimal allocations, + * buckets should be set in order of increasing index and all negative buckets should be set before positive buckets. * * @param index the index of the bucket * @param count the count of the bucket, must be at least 1 * @return the builder */ public ExponentialHistogramBuilder setPositiveBucket(long index, long count) { - if (count < 1) { - throw new IllegalArgumentException("Bucket count must be at least 1"); - } - positiveBuckets.put(index, count); + setBucket(index, count, true); return this; } /** - * Sets the given bucket of the negative buckets. - * Buckets may be set in arbitrary order. If the bucket already exists, it will be replaced. + * Sets the given bucket of the negative buckets. If the bucket already exists, it will be replaced. + * Buckets may be set in arbitrary order. However, for best performance and minimal allocations, + * buckets should be set in order of increasing index and all negative buckets should be set before positive buckets. * * @param index the index of the bucket * @param count the count of the bucket, must be at least 1 * @return the builder */ public ExponentialHistogramBuilder setNegativeBucket(long index, long count) { + setBucket(index, count, false); + return this; + } + + private void setBucket(long index, long count, boolean isPositive) { if (count < 1) { throw new IllegalArgumentException("Bucket count must be at least 1"); } - negativeBuckets.put(index, count); - return this; + if (negativeBuckets == null && positiveBuckets == null) { + // so far, all received buckets were in order, try to directly build the result + if (result == null) { + // Initialize the result buffer if required + adjustResultCapacity(estimatedBucketCount, false); + } + if ((isPositive && result.wasLastAddedBucketPositive() == false) + || (isPositive == result.wasLastAddedBucketPositive() && index > result.getLastAddedBucketIndex())) { + // the new bucket is in order too, we can directly add the bucket + addBucketToResult(index, count, isPositive); + return; + } + } + // fallback to TreeMap if a bucket is received out of order + initializeBucketTreeMapsIfNeeded(); + if (isPositive) { + positiveBuckets.put(index, count); + } else { + negativeBuckets.put(index, count); + } + } + + private void initializeBucketTreeMapsIfNeeded() { + if (negativeBuckets == null) { + negativeBuckets = new TreeMap<>(); + positiveBuckets = new TreeMap<>(); + // copy existing buckets to the maps + if (result != null) { + BucketIterator it = result.negativeBuckets().iterator(); + while (it.hasNext()) { + negativeBuckets.put(it.peekIndex(), it.peekCount()); + it.advance(); + } + it = result.positiveBuckets().iterator(); + while (it.hasNext()) { + positiveBuckets.put(it.peekIndex(), it.peekCount()); + it.advance(); + } + } + } + } + + private void addBucketToResult(long index, long count, boolean isPositive) { + if (resultAlreadyReturned) { + // we cannot modify the result anymore, create a new one + adjustResultCapacity(result.getCapacity(), true); + } + assert resultAlreadyReturned == false; + boolean sufficientCapacity = result.tryAddBucket(index, count, isPositive); + if (sufficientCapacity == false) { + int newCapacity = Math.max(result.getCapacity() * 2, DEFAULT_ESTIMATED_BUCKET_COUNT); + adjustResultCapacity(newCapacity, true); + boolean bucketAdded = result.tryAddBucket(index, count, isPositive); + assert bucketAdded : "Output histogram should have enough capacity"; + } + } + + private void adjustResultCapacity(int newCapacity, boolean copyExistingBuckets) { + FixedCapacityExponentialHistogram newResult = FixedCapacityExponentialHistogram.create(newCapacity, breaker); + if (copyExistingBuckets && result != null) { + BucketIterator it = result.negativeBuckets().iterator(); + while (it.hasNext()) { + boolean added = newResult.tryAddBucket(it.peekIndex(), it.peekCount(), false); + assert added : "Output histogram should have enough capacity"; + it.advance(); + } + it = result.positiveBuckets().iterator(); + while (it.hasNext()) { + boolean added = newResult.tryAddBucket(it.peekIndex(), it.peekCount(), true); + assert added : "Output histogram should have enough capacity"; + it.advance(); + } + } + if (result != null && resultAlreadyReturned == false) { + Releasables.close(result); + } + resultAlreadyReturned = false; + result = newResult; } public ReleasableExponentialHistogram build() { - FixedCapacityExponentialHistogram result = FixedCapacityExponentialHistogram.create( - negativeBuckets.size() + positiveBuckets.size(), - breaker - ); - boolean success = false; - try { + if (resultAlreadyReturned) { + // result was already returned on a previous call, return a new instance + adjustResultCapacity(result.getCapacity(), true); + } + assert resultAlreadyReturned == false; + if (negativeBuckets != null) { + // copy buckets from tree maps into result + adjustResultCapacity(negativeBuckets.size() + positiveBuckets.size(), false); result.resetBuckets(scale); - result.setZeroBucket(zeroBucket); negativeBuckets.forEach((index, count) -> result.tryAddBucket(index, count, false)); positiveBuckets.forEach((index, count) -> result.tryAddBucket(index, count, true)); - - double sumVal = (sum != null) - ? sum - : ExponentialHistogramUtils.estimateSum(result.negativeBuckets().iterator(), result.positiveBuckets().iterator()); - double minVal = (min != null) - ? min - : ExponentialHistogramUtils.estimateMin(zeroBucket, result.negativeBuckets(), result.positiveBuckets()).orElse(Double.NaN); - double maxVal = (max != null) - ? max - : ExponentialHistogramUtils.estimateMax(zeroBucket, result.negativeBuckets(), result.positiveBuckets()).orElse(Double.NaN); - - result.setMin(minVal); - result.setMax(maxVal); - result.setSum(sumVal); - - success = true; - } finally { - if (success == false) { - Releasables.close(result); + } else { + if (result == null) { + // no buckets were added + adjustResultCapacity(0, false); } + result.setScale(scale); } + + result.setZeroBucket(zeroBucket); + double sumVal = (sum != null) + ? sum + : ExponentialHistogramUtils.estimateSum(result.negativeBuckets().iterator(), result.positiveBuckets().iterator()); + double minVal = (min != null) + ? min + : ExponentialHistogramUtils.estimateMin(zeroBucket, result.negativeBuckets(), result.positiveBuckets()).orElse(Double.NaN); + double maxVal = (max != null) + ? max + : ExponentialHistogramUtils.estimateMax(zeroBucket, result.negativeBuckets(), result.positiveBuckets()).orElse(Double.NaN); + + result.setMin(minVal); + result.setMax(maxVal); + result.setSum(sumVal); + + resultAlreadyReturned = true; return result; } + + @Override + public void close() { + if (resultAlreadyReturned == false) { + Releasables.close(result); + } + } } diff --git a/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/FixedCapacityExponentialHistogram.java b/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/FixedCapacityExponentialHistogram.java index f8412f7f0237b..8869724c07cc8 100644 --- a/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/FixedCapacityExponentialHistogram.java +++ b/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/FixedCapacityExponentialHistogram.java @@ -78,6 +78,10 @@ private FixedCapacityExponentialHistogram(int bucketCapacity, ExponentialHistogr reset(); } + int getCapacity() { + return bucketIndices.length; + } + /** * Resets this histogram to the same state as a newly constructed one with the same capacity. */ @@ -95,10 +99,9 @@ void reset() { * @param scale the scale to set for this histogram */ void resetBuckets(int scale) { - assert scale >= MIN_SCALE && scale <= MAX_SCALE : "scale must be in range [" + MIN_SCALE + ".." + MAX_SCALE + "]"; + setScale(scale); negativeBuckets.reset(); positiveBuckets.reset(); - bucketScale = scale; } @Override @@ -180,6 +183,11 @@ public int scale() { return bucketScale; } + void setScale(int scale) { + assert scale >= MIN_SCALE && scale <= MAX_SCALE : "scale must be in range [" + MIN_SCALE + ".." + MAX_SCALE + "]"; + bucketScale = scale; + } + @Override public ExponentialHistogram.Buckets negativeBuckets() { return negativeBuckets; @@ -190,6 +198,25 @@ public ExponentialHistogram.Buckets positiveBuckets() { return positiveBuckets; } + /** + * @return the index of the last bucket added successfully via {@link #tryAddBucket(long, long, boolean)}, + * or {@link Long#MIN_VALUE} if no buckets have been added yet. + */ + long getLastAddedBucketIndex() { + if (positiveBuckets.numBuckets + negativeBuckets.numBuckets > 0) { + return bucketIndices[negativeBuckets.numBuckets + positiveBuckets.numBuckets - 1]; + } else { + return Long.MIN_VALUE; + } + } + + /** + * @return true, if the last bucket added successfully via {@link #tryAddBucket(long, long, boolean)} was a positive one. + */ + boolean wasLastAddedBucketPositive() { + return positiveBuckets.numBuckets > 0; + } + @Override public void close() { if (closed) { @@ -276,6 +303,11 @@ public long valueCount() { } return cachedValueSum; } + + @Override + public int bucketCount() { + return numBuckets; + } } } diff --git a/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramBuilderTests.java b/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramBuilderTests.java index ddbf3ea20d0e1..3875774990486 100644 --- a/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramBuilderTests.java +++ b/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramBuilderTests.java @@ -21,39 +21,50 @@ package org.elasticsearch.exponentialhistogram; +import org.elasticsearch.core.Releasables; + import java.util.List; +import java.util.function.Consumer; import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.not; +import static org.hamcrest.Matchers.nullValue; +import static org.hamcrest.Matchers.sameInstance; public class ExponentialHistogramBuilderTests extends ExponentialHistogramTestCase { public void testBuildAndCopyWithAllFieldsSet() { ZeroBucket zeroBucket = ZeroBucket.create(1, 2); - ExponentialHistogramBuilder builder = ExponentialHistogram.builder(3, breaker()) - .zeroBucket(zeroBucket) - .sum(100.0) - .min(1.0) - .max(50.0) - .setPositiveBucket(2, 10) - .setPositiveBucket(0, 1) - .setPositiveBucket(5, 2) - .setNegativeBucket(-2, 5) - .setNegativeBucket(1, 2); + try (ExponentialHistogramBuilder builder = ExponentialHistogram.builder(3, breaker())) { + builder.zeroBucket(zeroBucket) + .sum(100.0) + .min(1.0) + .max(50.0) + .setPositiveBucket(2, 10) + .setPositiveBucket(0, 1) + .setPositiveBucket(5, 2) + .setNegativeBucket(-2, 5) + .setNegativeBucket(1, 2); - try (ReleasableExponentialHistogram histogram = builder.build()) { - assertThat(histogram.scale(), equalTo(3)); - assertThat(histogram.zeroBucket(), equalTo(zeroBucket)); - assertThat(histogram.sum(), equalTo(100.0)); - assertThat(histogram.min(), equalTo(1.0)); - assertThat(histogram.max(), equalTo(50.0)); - assertBuckets(histogram.positiveBuckets(), List.of(0L, 2L, 5L), List.of(1L, 10L, 2L)); - assertBuckets(histogram.negativeBuckets(), List.of(-2L, 1L), List.of(5L, 2L)); - - try (ReleasableExponentialHistogram copy = ExponentialHistogram.builder(histogram, breaker()).build()) { - assertThat(copy, equalTo(histogram)); + try (ReleasableExponentialHistogram histogram = builder.build()) { + assertThat(histogram.scale(), equalTo(3)); + assertThat(histogram.zeroBucket(), equalTo(zeroBucket)); + assertThat(histogram.sum(), equalTo(100.0)); + assertThat(histogram.min(), equalTo(1.0)); + assertThat(histogram.max(), equalTo(50.0)); + assertBuckets(histogram.positiveBuckets(), List.of(0L, 2L, 5L), List.of(1L, 10L, 2L)); + assertBuckets(histogram.negativeBuckets(), List.of(-2L, 1L), List.of(5L, 2L)); + + // Ensure copy is efficient with only a single allocation and no use of TreeMaps + AllocationCountingBreaker countingBreaker = new AllocationCountingBreaker(breaker()); + ExponentialHistogramBuilder copyBuilder = ExponentialHistogram.builder(histogram, countingBreaker); + assertThat(copyBuilder.negativeBuckets, nullValue()); + try (ReleasableExponentialHistogram copy = copyBuilder.build()) { + assertThat(copy, equalTo(histogram)); + assertThat(countingBreaker.numAllocations, equalTo(1)); + } } } - } public void testBuildWithEstimation() { @@ -85,6 +96,131 @@ public void testDuplicateBucketOverrides() { } } + public void testEfficientCreationForBucketsInOrder() { + Consumer generator = builder -> builder.setNegativeBucket(-2, 5) + .setNegativeBucket(1, 2) + .setPositiveBucket(0, 1) + .setPositiveBucket(2, 10) + .setPositiveBucket(5, 2); + + // Test with a correct count estimate + AllocationCountingBreaker countingBreaker = new AllocationCountingBreaker(breaker()); + try (ExponentialHistogramBuilder builder = ExponentialHistogram.builder(0, countingBreaker)) { + builder.estimatedBucketCount(5); + generator.accept(builder); + assertThat(builder.negativeBuckets, nullValue()); + assertThat(builder.positiveBuckets, nullValue()); + try (ReleasableExponentialHistogram histogram = builder.build()) { + assertThat(countingBreaker.numAllocations, equalTo(1)); + assertBuckets(histogram.positiveBuckets(), List.of(0L, 2L, 5L), List.of(1L, 10L, 2L)); + assertBuckets(histogram.negativeBuckets(), List.of(-2L, 1L), List.of(5L, 2L)); + } + } + // Test with a too small estimate, requiring a single resize + countingBreaker = new AllocationCountingBreaker(breaker()); + try (ExponentialHistogramBuilder builder = ExponentialHistogram.builder(0, countingBreaker)) { + builder.estimatedBucketCount(3); + generator.accept(builder); + assertThat(builder.negativeBuckets, nullValue()); + assertThat(builder.positiveBuckets, nullValue()); + try (ReleasableExponentialHistogram histogram = builder.build()) { + assertThat(countingBreaker.numAllocations, equalTo(2)); + assertBuckets(histogram.positiveBuckets(), List.of(0L, 2L, 5L), List.of(1L, 10L, 2L)); + assertBuckets(histogram.negativeBuckets(), List.of(-2L, 1L), List.of(5L, 2L)); + } + } + } + + public void testPositiveBucketsOutOfOrder() { + try (ExponentialHistogramBuilder builder = ExponentialHistogram.builder(0, breaker())) { + builder.setPositiveBucket(1, 2); + builder.setPositiveBucket(0, 1); + try (ReleasableExponentialHistogram histogram = builder.build()) { + assertBuckets(histogram.positiveBuckets(), List.of(0L, 1L), List.of(1L, 2L)); + } + } + } + + public void testNegativeBucketsOutOfOrder() { + try (ExponentialHistogramBuilder builder = ExponentialHistogram.builder(0, breaker())) { + builder.setNegativeBucket(1, 2); + builder.setNegativeBucket(0, 1); + try (ReleasableExponentialHistogram histogram = builder.build()) { + assertBuckets(histogram.negativeBuckets(), List.of(0L, 1L), List.of(1L, 2L)); + } + } + } + + public void testPositiveBucketsBeforeNegativeBuckets() { + try (ExponentialHistogramBuilder builder = ExponentialHistogram.builder(0, breaker())) { + builder.setPositiveBucket(0, 2); + builder.setNegativeBucket(1, 1); + try (ReleasableExponentialHistogram histogram = builder.build()) { + assertBuckets(histogram.negativeBuckets(), List.of(1L), List.of(1L)); + assertBuckets(histogram.positiveBuckets(), List.of(0L), List.of(2L)); + } + } + } + + public void testCloseWithoutBuildDoesNotLeak() { + try (ExponentialHistogramBuilder builder = ExponentialHistogram.builder(0, breaker())) { + builder.setPositiveBucket(0, 1); + } + } + + public void testMutationsAfterBuild() { + try (ExponentialHistogramBuilder builder = ExponentialHistogram.builder(3, breaker())) { + ZeroBucket zeroBucket = ZeroBucket.create(1, 2); + builder.zeroBucket(zeroBucket) + .sum(100.0) + .min(1.0) + .max(50.0) + .setPositiveBucket(2, 10) + .setPositiveBucket(0, 1) + .setPositiveBucket(5, 2) + .setNegativeBucket(-2, 5) + .setNegativeBucket(1, 2); + + ReleasableExponentialHistogram original = builder.build(); + ReleasableExponentialHistogram copy = builder.build(); + assertThat(original, equalTo(copy)); + assertThat(original, not(sameInstance(copy))); + + builder.zeroBucket(ZeroBucket.minimalEmpty()) + .scale(0) + .sum(10.0) + .min(0.0) + .max(2.0) + .setPositiveBucket(0, 1) + .setPositiveBucket(2, 1) + .setPositiveBucket(5, 1) + .setPositiveBucket(6, 1) + .setNegativeBucket(-2, 1) + .setNegativeBucket(-1, 1) + .setNegativeBucket(1, 1); + + ReleasableExponentialHistogram modified = builder.build(); + + assertThat(original.scale(), equalTo(3)); + assertThat(original.zeroBucket(), equalTo(zeroBucket)); + assertThat(original.sum(), equalTo(100.0)); + assertThat(original.min(), equalTo(1.0)); + assertThat(original.max(), equalTo(50.0)); + assertBuckets(original.positiveBuckets(), List.of(0L, 2L, 5L), List.of(1L, 10L, 2L)); + assertBuckets(original.negativeBuckets(), List.of(-2L, 1L), List.of(5L, 2L)); + + assertThat(modified.scale(), equalTo(0)); + assertThat(modified.zeroBucket(), equalTo(ZeroBucket.minimalEmpty())); + assertThat(modified.sum(), equalTo(10.0)); + assertThat(modified.min(), equalTo(0.0)); + assertThat(modified.max(), equalTo(2.0)); + assertBuckets(modified.positiveBuckets(), List.of(0L, 2L, 5L, 6L), List.of(1L, 1L, 1L, 1L)); + assertBuckets(modified.negativeBuckets(), List.of(-2L, -1L, 1L), List.of(1L, 1L, 1L)); + + Releasables.close(original, copy, modified); + } + } + private static void assertBuckets(ExponentialHistogram.Buckets buckets, List indices, List counts) { List actualIndices = new java.util.ArrayList<>(); List actualCounts = new java.util.ArrayList<>(); @@ -97,4 +233,21 @@ private static void assertBuckets(ExponentialHistogram.Buckets buckets, List 0) { + numAllocations++; + } + delegate.adjustBreaker(bytesAllocated); + } + } } From 292a6f6f5435eadbe3e220c94cfa5eb1104adeaa Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Thu, 2 Oct 2025 12:02:00 +0200 Subject: [PATCH 2/3] Add test for default bucketCount implementation --- .../ExponentialHistogramTests.java | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramTests.java diff --git a/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramTests.java b/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramTests.java new file mode 100644 index 0000000000000..63346d2494f78 --- /dev/null +++ b/libs/exponential-histogram/src/test/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramTests.java @@ -0,0 +1,60 @@ +/* + * Copyright Elasticsearch B.V., and/or licensed to Elasticsearch B.V. + * under one or more license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you 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. + * + * This file is based on a modification of https://github.com/open-telemetry/opentelemetry-java which is licensed under the Apache 2.0 License. + */ + +package org.elasticsearch.exponentialhistogram; + +import java.util.OptionalLong; + +import static org.hamcrest.Matchers.equalTo; + +public class ExponentialHistogramTests extends ExponentialHistogramTestCase { + + public void testDefaultBucketImplementation() { + try ( + ReleasableExponentialHistogram delegate = ExponentialHistogram.builder(0, breaker()) + .setPositiveBucket(0, 2) + .setPositiveBucket(10, 20) + .setPositiveBucket(20, 30) + .build() + ) { + + ExponentialHistogram.Buckets bucketsWithDefaultImplementation = new ExponentialHistogram.Buckets() { + @Override + public CopyableBucketIterator iterator() { + return delegate.positiveBuckets().iterator(); + } + + @Override + public OptionalLong maxBucketIndex() { + throw new UnsupportedOperationException(); + } + + @Override + public long valueCount() { + throw new UnsupportedOperationException(); + } + }; + assertThat(bucketsWithDefaultImplementation.bucketCount(), equalTo(3)); + } + + } + +} From 54f249fd2d96086e0e808d6cde662ccfa8779865 Mon Sep 17 00:00:00 2001 From: Jonas Kunz Date: Mon, 6 Oct 2025 09:37:10 +0200 Subject: [PATCH 3/3] Rename allocation method --- .../ExponentialHistogramBuilder.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramBuilder.java b/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramBuilder.java index 9691a82ce9afe..14c9bb065f9f1 100644 --- a/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramBuilder.java +++ b/libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramBuilder.java @@ -165,7 +165,7 @@ private void setBucket(long index, long count, boolean isPositive) { // so far, all received buckets were in order, try to directly build the result if (result == null) { // Initialize the result buffer if required - adjustResultCapacity(estimatedBucketCount, false); + reallocateResultWithCapacity(estimatedBucketCount, false); } if ((isPositive && result.wasLastAddedBucketPositive() == false) || (isPositive == result.wasLastAddedBucketPositive() && index > result.getLastAddedBucketIndex())) { @@ -206,21 +206,21 @@ private void initializeBucketTreeMapsIfNeeded() { private void addBucketToResult(long index, long count, boolean isPositive) { if (resultAlreadyReturned) { // we cannot modify the result anymore, create a new one - adjustResultCapacity(result.getCapacity(), true); + reallocateResultWithCapacity(result.getCapacity(), true); } assert resultAlreadyReturned == false; boolean sufficientCapacity = result.tryAddBucket(index, count, isPositive); if (sufficientCapacity == false) { int newCapacity = Math.max(result.getCapacity() * 2, DEFAULT_ESTIMATED_BUCKET_COUNT); - adjustResultCapacity(newCapacity, true); + reallocateResultWithCapacity(newCapacity, true); boolean bucketAdded = result.tryAddBucket(index, count, isPositive); assert bucketAdded : "Output histogram should have enough capacity"; } } - private void adjustResultCapacity(int newCapacity, boolean copyExistingBuckets) { + private void reallocateResultWithCapacity(int newCapacity, boolean copyBucketsFromPreviousResult) { FixedCapacityExponentialHistogram newResult = FixedCapacityExponentialHistogram.create(newCapacity, breaker); - if (copyExistingBuckets && result != null) { + if (copyBucketsFromPreviousResult && result != null) { BucketIterator it = result.negativeBuckets().iterator(); while (it.hasNext()) { boolean added = newResult.tryAddBucket(it.peekIndex(), it.peekCount(), false); @@ -244,19 +244,19 @@ private void adjustResultCapacity(int newCapacity, boolean copyExistingBuckets) public ReleasableExponentialHistogram build() { if (resultAlreadyReturned) { // result was already returned on a previous call, return a new instance - adjustResultCapacity(result.getCapacity(), true); + reallocateResultWithCapacity(result.getCapacity(), true); } assert resultAlreadyReturned == false; if (negativeBuckets != null) { // copy buckets from tree maps into result - adjustResultCapacity(negativeBuckets.size() + positiveBuckets.size(), false); + reallocateResultWithCapacity(negativeBuckets.size() + positiveBuckets.size(), false); result.resetBuckets(scale); negativeBuckets.forEach((index, count) -> result.tryAddBucket(index, count, false)); positiveBuckets.forEach((index, count) -> result.tryAddBucket(index, count, true)); } else { if (result == null) { // no buckets were added - adjustResultCapacity(0, false); + reallocateResultWithCapacity(0, false); } result.setScale(scale); }