Reuse CompensatedSum object in agg collect loop #49548
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
|
So @iverase kindly pointed out that all the ceremony around the BigArray/DoubleArray stuff is unnecessary, and we really only need the "reset" functionality and each agg can keep it's own scratch E.g. wildly over-complicated. :) Gonna re-work this |
polyfractal
force-pushed
the
compensated_sum_double_array
branch
from
3566bd0 to
9e5d3be
Compare
|
Pushed an update, now 200% simpler. Thanks @iverase :) |
polyfractal
changed the title
polyfractal
added a commit
that referenced
this pull request
Nov 25, 2019
The new CompensatedSum is a nice DRY refactor, but had the unanticipated side effect of creating a lot of object allocation in the aggregation hot collection loop: one object per visited document, per aggregator. In some places it created two per-doc-per-agg (weighted avg, geo centroids, etc) since there were multiple compensations being maintained. This PR moves the object creation out of the hot loop so that it is now created once per segment, and resets the internal state each time through the loop
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The new
CompensatedSumis a nice DRY refactor, but had the unanticipated side effect of creating a lot of object allocation in the aggregation hot collection loop: one object per visited document, per aggregator. In some places it created two per-doc-per-agg (weighted avg, geo centroids, etc) since there were multiple compensations being maintained.This PR moves the object creation out of the hot loop so that it is now created once per segment, and resets the internal state each time through the loop
Closes #49506
Note:
CompensatedSumis also used for the various Internal* classes, and these have been left alone since they are not performance critical and the usage works great there.Old text:
This is a refactor to introduce a new "compensated" DoubleArray abstraction. It allows aggregators to use kahan summation with no extra object garbage. Internally this uses the newCompensatedSumobject, but just continually resets the state for each new bucket instead of allocating a new object.The syntax loosely follows DoubleArray, but is different in places. A user will initialize/grow the array as normal, but instead of getting/setting they will reset/add/commit values. Because of these changes it doesn't currently implement DoubleArray or any of the relatives.Still a bit of a WIP. I want to run some benchmarks to see if it makes a noticeable difference, and I'm not quite satisfied with the interface/abstraction yet.