Save memory when histogram agg is not on top #57277
Merged
Conversation
This file contains hidden or 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
This saves some memory when the `histogram` aggregation is not a top level aggregation by dropping `asMultiBucketAggregator` in favor of natively implementing multi-bucket storage in the aggregator. For the most part this just uses the `LongKeyedBucketOrds` that we built the first time we did this.
Collaborator
|
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
elasticmachine
added
the
Team:Analytics
Member
Author
|
I'm going to add a test for the new debug information. |
nik9000
commented
May 28, 2020
| * Base class for functionality shared between aggregators for this | ||
| * {@code histogram} aggregation. | ||
| */ | ||
| public abstract class AbstractHistogramAggregator extends BucketsAggregator { |
Member
Author
There was a problem hiding this comment.
There was a TODO that the range and numeric version of the aggregator shared a ton of code. Now they share a superclass that provides all that code.
| ValuesSource valuesSource, DocValueFormat formatter, SearchContext context, | ||
| Aggregator parent, Map<String, Object> metadata) throws IOException { | ||
| ValuesSource.Range rangeValueSource = (ValuesSource.Range) valuesSource; | ||
| if (rangeValueSource.rangeType().isNumeric() == false) { |
Member
Author
There was a problem hiding this comment.
I moved this check into the ctor so I can use the ctor reference that we've been doing elsewhere.
| Aggregator parent, | ||
| boolean collectsFromSingleBucket, | ||
| Map<String, Object> metadata) throws IOException { | ||
| if (collectsFromSingleBucket == false) { |
Member
Author
There was a problem hiding this comment.
this is the important line!
| fieldType.setName("field"); | ||
| try (IndexReader reader = w.getReader()) { | ||
| IndexSearcher searcher = new IndexSearcher(reader); | ||
| InternalHistogram histogram = search(searcher, new MatchAllDocsQuery(), aggBuilder, fieldType); |
Member
Author
There was a problem hiding this comment.
We do this kind of thing all over the place so I figured I'd make a utility method for it.
16 tasks
talevy
approved these changes
May 28, 2020
| Releasables.close(releasables); | ||
| releasables.clear(); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
nice touch. likely usable across many future tests
Member
Author
|
I'm pulling some performance numbers for this. I'll likely merge before they get done and update with them once they come in. I'm fairly confident in it though. |
Member
Author
|
About a 38% performance gain in the test that I ran: Before: After: |
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
May 29, 2020
This saves some memory when the `histogram` aggregation is not a top level aggregation by dropping `asMultiBucketAggregator` in favor of natively implementing multi-bucket storage in the aggregator. For the most part this just uses the `LongKeyedBucketOrds` that we built the first time we did this.
nik9000
added a commit
that referenced
this pull request
May 29, 2020
…7377) This saves some memory when the `histogram` aggregation is not a top level aggregation by dropping `asMultiBucketAggregator` in favor of natively implementing multi-bucket storage in the aggregator. For the most part this just uses the `LongKeyedBucketOrds` that we built the first time we did this.
nik9000
added a commit
to nik9000/elasticsearch
that referenced
this pull request
May 29, 2020
nik9000
added a commit
that referenced
this pull request
May 29, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
This saves some memory when the
histogramaggregation is not a toplevel aggregation by dropping
asMultiBucketAggregatorin favor ofnatively implementing multi-bucket storage in the aggregator. For the
most part this just uses the
LongKeyedBucketOrdsthat we built thefirst time we did this.