Skip to content

Commit aa04178

Browse files
authored
Remove Todo for exponential histogram merging optimization. (#135345)
1 parent 8a1aa7c commit aa04178

File tree

1 file changed

+10
-5
lines changed

1 file changed

+10
-5
lines changed

libs/exponential-histogram/src/main/java/org/elasticsearch/exponentialhistogram/ExponentialHistogramMerger.java

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -137,11 +137,16 @@ public ReleasableExponentialHistogram getAndClear() {
137137
return retVal;
138138
}
139139

140-
// TODO(b/128622): this algorithm is very efficient if b has roughly as many buckets as a
141-
// However, if b is much smaller we still have to iterate over all buckets of a which is very wasteful.
142-
// This can be optimized by buffering multiple histograms to accumulate first,
143-
// then in O(log(n)) turn them into a single, merged histogram.
144-
// (n is the number of buffered buckets)
140+
// This algorithm is very efficient if B has roughly as many buckets as A.
141+
// However, if B is much smaller we still have to iterate over all buckets of A.
142+
// This can be optimized by buffering the buckets of small histograms and only merging them when we have enough buckets.
143+
// The buffered histogram buckets would first be merged with each other, and then be merged with accumulator.
144+
//
145+
// However, benchmarks of a PoC implementation have shown that this only brings significant improvements
146+
// if the accumulator size is 500+ and the merged histograms are smaller than 50 buckets
147+
// and otherwise slows down the merging.
148+
// It would be possible to only enable the buffering for small histograms,
149+
// but the optimization seems not worth the added complexity at this point.
145150

146151
/**
147152
* Merges the given histogram into the current result.

0 commit comments

Comments
 (0)