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

Stop extra copy of exponential histogram buckets #5020

Merged
merged 1 commit into from Dec 7, 2022
Merged
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
Expand Up @@ -58,25 +58,20 @@ public AggregatorHandle<ExponentialHistogramAccumulation, DoubleExemplarData> cr
}

/**
* This function is an immutable merge. It firstly combines the sum and zero count. Then it
* performs a merge using the buckets from both accumulations, without modifying those
* accumulations.
*
* @param previous the previously captured accumulation
* @param current the newly captured (delta) accumulation
* @return the result of the merge of the given accumulations.
* Merge the exponential histogram accumulations. Mutates the {@link
* ExponentialHistogramAccumulation#getPositiveBuckets()} and {@link
* ExponentialHistogramAccumulation#getNegativeBuckets()} of {@code previous}. Mutating buckets is
* acceptable because copies are already made in {@link Handle#doAccumulateThenReset(List)}.
*/
@Override
public ExponentialHistogramAccumulation merge(
ExponentialHistogramAccumulation previous, ExponentialHistogramAccumulation current) {

// Create merged buckets
DoubleExponentialHistogramBuckets posBuckets =
DoubleExponentialHistogramBuckets.merge(
previous.getPositiveBuckets(), current.getPositiveBuckets());
merge(previous.getPositiveBuckets(), current.getPositiveBuckets());
DoubleExponentialHistogramBuckets negBuckets =
DoubleExponentialHistogramBuckets.merge(
previous.getNegativeBuckets(), current.getNegativeBuckets());
merge(previous.getNegativeBuckets(), current.getNegativeBuckets());

// resolve possible scale difference due to merge
int commonScale = Math.min(posBuckets.getScale(), negBuckets.getScale());
Expand All @@ -95,7 +90,7 @@ public ExponentialHistogramAccumulation merge(
max = current.getMax();
}
return ExponentialHistogramAccumulation.create(
posBuckets.getScale(),
commonScale,
previous.getSum() + current.getSum(),
previous.hasMinMax() || current.hasMinMax(),
min,
Expand All @@ -106,6 +101,21 @@ public ExponentialHistogramAccumulation merge(
current.getExemplars());
}

/**
* Merge the exponential histogram buckets. If {@code a} is empty, return {@code b}. If {@code b}
* is empty, return {@code a}. Else merge {@code b} into {@code a}.
*/
private static DoubleExponentialHistogramBuckets merge(
DoubleExponentialHistogramBuckets a, DoubleExponentialHistogramBuckets b) {
if (b.getTotalCount() == 0) {
return a;
} else if (a.getTotalCount() == 0) {
return b;
}
a.mergeInto(b);
return a;
}

@Override
public MetricData toMetricData(
Resource resource,
Expand Down
Expand Up @@ -132,30 +132,9 @@ void downscale(int by) {
this.exponentialHistogramIndexer = ExponentialHistogramIndexer.get(this.scale);
}

/**
* Immutable method for merging. This method copies the first set of buckets, performs the merge
* on the copy, and returns the copy.
*
* @param a first buckets
* @param b second buckets
* @return A new set of buckets, the result
*/
static DoubleExponentialHistogramBuckets merge(
DoubleExponentialHistogramBuckets a, DoubleExponentialHistogramBuckets b) {
if (b.counts.isEmpty()) {
return a;
} else if (a.counts.isEmpty()) {
return b;
}
DoubleExponentialHistogramBuckets copy = a.copy();
copy.mergeWith(b/* additive= */ );
return copy;
}

/**
* This method merges this instance with another set of buckets. It alters the underlying bucket
* counts and scale of this instance only, so it is to be used with caution. For immutability, use
* the static merge() method.
* counts and scale of this instance only, so it is to be used with caution.
*
* <p>The bucket counts of this instance will be added to or subtracted from depending on the
* additive parameter.
Expand All @@ -164,7 +143,7 @@ static DoubleExponentialHistogramBuckets merge(
*
* @param other the histogram that will be merged into this one
*/
private void mergeWith(DoubleExponentialHistogramBuckets other) {
void mergeInto(DoubleExponentialHistogramBuckets other) {
if (other.counts.isEmpty()) {
return;
}
Expand Down Expand Up @@ -195,7 +174,7 @@ private void mergeWith(DoubleExponentialHistogramBuckets other) {
// since we changed scale of this, we need to know the new difference between the two scales
deltaOther = other.scale - this.scale;

// do actual merging of other into this. Will decrement or increment depending on sign.
// do actual merging of other into this.
for (int i = other.getOffset(); i <= other.counts.getIndexEnd(); i++) {
if (!this.counts.increment(i >> deltaOther, other.counts.get(i))) {
// This should never occur if scales and windows are calculated without bugs
Expand Down