Remove Todo for exponential histogram merging optimization. #135345
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a ToDo in the exponential histogram merging algorithm for a potential optimization for merging small histograms.
The current algorithm merges all histograms into an accumulator with the desired bucket count limit (e.g. 320 would be the OpenTelemetry default). During this merge process, the merger has to iterate through all buckets of the accumulator and the accumulated histogram. If the latter is much smaller, a lot of time might be wasted iterating the accumulator.
A potential optimization for this is to
O(n logn), but is amortized across the accumulated histograms) when the buffer is fullI did a PoC implementation for this and ran some benchmarks.
Benchmark results without optimization
Benchmark results with optimization
The
mergedHistoSizeFactoris the factor how much smaller the merged histograms are than the accumulator and the duration is the time to merge a single histogram into the accumulator.As it turns out, the optimization is only a net positive for bucket counts >= 500 and if the merged histograms are at least 90% smaller than the accumulator. Otherwise it even slows the merging down due to additional buffering / copying.
We could optimize for this (e.g. only buffer histograms with a size below a threshold), however it does not seem worth it at this point. If we use something around the opentelemetry default as bucket limit (e.g.
320), we don't see significant gains for reasonably sized inputs. However, at the same time the optimization certainly increases the complexity of the code.Therefore with this propose I'd propose to document this in the code, remove the ToDo, but not add the optimization into the code as it does not seem worth it right now.