Save memory when numeric terms agg is not top by nik9000 · Pull Request #55873 · elastic/elasticsearch

nik9000 · 2020-04-28T16:30:41Z

Right now all implementations of the terms agg allocate a new
Aggregator per bucket. This uses a bunch of memory. Exactly how much
isn't clear but each Aggregator ends up making its own objects to read
doc values which have non-trivial buffers. And it forces all of it
sub-aggregations to do the same. We allocate a new Aggregator per
bucket for two reasons:

We didn't have an appropriate data structure to track the
sub-ordinals of each parent bucket.
You can only make a single call to runDeferredCollections(long...)
per Aggregator which was the only way to delay collection of
sub-aggregations.

This change adds a way to return "deferred" aggregations from any
bucket aggregation and undefers them as part of regular collections.
This mechanism allows you to defer without
runDeferredCollections(long...).

This change also adds a fairly simplistic data structure to track the
sub-ordinals for long-keyed buckets.

It uses both of those to power numeric terms aggregations and removes
the per-bucket allocation of their Aggregator. This fairly
substantially reduces memory consumption of numeric terms aggregations
that are not the "top level", especially when those aggregations contain
many sub-aggregations.

I picked numeric terms aggregations because those have the simplest
implementation. At least, I could kind of fit it in my head. And I
haven't fully understood the "bytes"-based terms aggregations, but I
imagine I'll be able to make similar optimizations to them in follow up
changes.

Right now all implementations of the `terms` agg allocate a new `Aggregator` per bucket. This uses a bunch of memory. Exactly how much isn't clear but each `Aggregator` ends up making its own objects to read doc values which have non-trivial buffers. And it forces all of it sub-aggregations to do the same. We allocate a new `Aggregator` per bucket for two reasons: 1. We didn't have an appropriate data structure to track the sub-ordinals of each parent bucket. 2. You can only make a single call to `runDeferredCollections(long...)` per `Aggregator` which was the only way to delay collection of sub-aggregations. This change adds a way to return "deferred" aggregations from any bucket aggregation and undefers them as part of regular collections. This mechanism allows you to defer without `runDeferredCollections(long...)`. This change also adds a fairly simplistic data structure to track the sub-ordinals for `long`-keyed buckets. It uses both of those to power numeric `terms` aggregations and removes the per-bucket allocation of their `Aggregator`. This fairly substantially reduces memory consumption of numeric `terms` aggregations that are not the "top level", especially when those aggregations contain many sub-aggregations. I picked numeric `terms` aggregations because those have the simplest implementation. At least, I could kind of fit it in my head. And I haven't fully understood the "bytes"-based terms aggregations, but I imagine I'll be able to make similar optimizations to them in follow up changes.

docs/reference/search/profile.asciidoc

nik9000 · 2020-04-28T17:59:44Z

@elasticmachine, test this pleas

Adds a rally track specifically for testing the performance of the terms agg as I'll be doing some work on it. In particular this focuses on numeric terms because the first phase of my work only touches them. Relates to elastic/elasticsearch#55873

nik9000 · 2020-04-29T14:37:47Z

This looks like it might actually speed up nested numeric terms.

Before:

|90th percentile service time | keyword_terms_numeric_terms |     3567.84 |     ms |
|90th percentile service time | numeric_terms_numeric_terms |     2461.33 |     ms |
|90th percentile service time |    date_histo_numeric_terms |     3950.39 |     ms |

After:

|90th percentile service time | keyword_terms_numeric_terms |     3344.94 |     ms |
|90th percentile service time | numeric_terms_numeric_terms |     2090.58 |     ms |
|90th percentile service time |    date_histo_numeric_terms |     2623.48 |     ms |

I kind of figured that it'd be a little faster. But these numbers are more than I'd though.... They look real though.

nik9000 · 2020-04-30T20:16:54Z

server/src/main/java/org/elasticsearch/search/aggregations/InternalMultiBucketAggregation.java

@@ -158,19 +158,27 @@ public final InternalAggregation reducePipelines(

    @Override
    public InternalAggregation copyWithRewritenBuckets(Function<InternalAggregations, InternalAggregations> rewriter) {


I can revert this.

nik9000 · 2020-05-03T16:20:51Z

I've rebuilt this, replacing delaying building results with reworking agg results to be built for the entire aggregator at once. I've push some code that looks to mostly work and will be cleaning it up soon!

nik9000 · 2020-05-03T17:24:50Z

server/src/main/java/org/elasticsearch/search/aggregations/bucket/BucketsAggregator.java


    @Override
    public BucketComparator bucketComparator(String key, SortOrder order) {
+        if (false == this instanceof SingleBucketAggregator) {


This was missing from some work that I did previously and removing the wrapping of LongTermsAggregator revealed it.

Without this we get strange test failures around sorting.

Saves memory when the `geotile_grid` and `geohash_grid` are not on the top level by using the `LongKeyedBucketOrds` we built in #55873.

Adds a rally track specifically for testing the performance of the bucket aggs when they are "sub" aggs. Relates to elastic/elasticsearch#55873

Reworks the `parent` and `child` aggregation are not at the top level using the optimization from elastic#55873. Instead of wrapping all non-top-level `parent` and `child` aggregators we now handle being a child aggregator in the aggregator, specifically by adding recording which global ordinals show up in the parent and then checking if they match the child.

Reworks the `parent` and `child` aggregation are not at the top level using the optimization from #55873. Instead of wrapping all non-top-level `parent` and `child` aggregators we now handle being a child aggregator in the aggregator, specifically by adding recording which global ordinals show up in the parent and then checking if they match the child.

Reworks the `parent` and `child` aggregation are not at the top level using the optimization from elastic#55873. Instead of wrapping all non-top-level `parent` and `child` aggregators we now handle being a child aggregator in the aggregator, specifically by adding recording which global ordinals show up in the parent and then checking if they match the child.

Reworks the `parent` and `child` aggregation are not at the top level using the optimization from #55873. Instead of wrapping all non-top-level `parent` and `child` aggregators we now handle being a child aggregator in the aggregator, specifically by adding recording which global ordinals show up in the parent and then checking if they match the child.

This uses the optimization that we started making in elastic#55873 for `rare_terms` to save a bit of memory when that aggregation is not on the top level.

This uses the optimization that we started making in #55873 for `rare_terms` to save a bit of memory when that aggregation is not on the top level.

This uses the optimization that we started making in elastic#55873 for `rare_terms` to save a bit of memory when that aggregation is not on the top level.

This uses the optimization that we started making in #55873 for `rare_terms` to save a bit of memory when that aggregation is not on the top level.

This merges the aggregator for `significant_text` into `significant_terms`, applying the optimization built in elastic#55873 to save memory when the aggregation is not on top. The `significant_text` aggregation is pretty memory intensive all on its own and this doesn't particularly help with that, but it'll help with the memory usage of any sub-aggregations.

This merges the aggregator for `significant_text` into `significant_terms`, applying the optimization built in #55873 to save memory when the aggregation is not on top. The `significant_text` aggregation is pretty memory intensive all on its own and this doesn't particularly help with that, but it'll help with the memory usage of any sub-aggregations.

This merges the aggregator for `significant_text` into `significant_terms`, applying the optimization built in elastic#55873 to save memory when the aggregation is not on top. The `significant_text` aggregation is pretty memory intensive all on its own and this doesn't particularly help with that, but it'll help with the memory usage of any sub-aggregations.

This merges the aggregator for `significant_text` into `significant_terms`, applying the optimization built in #55873 to save memory when the aggregation is not on top. The `significant_text` aggregation is pretty memory intensive all on its own and this doesn't particularly help with that, but it'll help with the memory usage of any sub-aggregations.

nik9000 added >enhancement :Analytics/Aggregations Aggregations v8.0.0 v7.8.0 labels Apr 28, 2020

nik9000 requested review from polyfractal and talevy April 28, 2020 16:30

nik9000 commented Apr 28, 2020

View reviewed changes

docs/reference/search/profile.asciidoc Outdated Show resolved Hide resolved

nik9000 added 3 commits April 28, 2020 14:39

Fix funny test

f3d9476

Merge branch 'master' into sneaky_delay

3786e8e

Oh rollup

aad9c1a

nik9000 mentioned this pull request Apr 29, 2020

Add track for sub-bucket-aggs elastic/rally-tracks#114

Merged

nik9000 added 4 commits April 29, 2020 10:55

Merge branch 'master' into sneaky_delay

dcc76b9

Merge branch 'master' into sneaky_delay

9dbbfbf

WIP

9f29f36

huh

1f5ed9d

nik9000 commented Apr 30, 2020

View reviewed changes

nik9000 added 4 commits May 3, 2020 12:29

Cleanup

22ad5c4

Drop docs change

da44476

Doc method

e064c4c

WIP

c8c83b8

nik9000 commented May 3, 2020

View reviewed changes

nik9000 added 4 commits May 3, 2020 17:55

Merge branch 'master' into sneaky_delay

1fedfd6

Remove useless method

7ae8396

Merge branch 'master' into sneaky_delay

107c3b5

Add tests for nested and reverse nested

e872b23

nik9000 mentioned this pull request Jun 2, 2020

Same memory when geo aggregations are not on top (backport of #57483) #57551

Merged

nik9000 added a commit that referenced this pull request Jun 2, 2020

Same memory when geo aggregations are not on top (#57483) (#57551)

2a27c41

Saves memory when the `geotile_grid` and `geohash_grid` are not on the top level by using the `LongKeyedBucketOrds` we built in #55873.

hub-cap pushed a commit to elastic/rally-tracks that referenced this pull request Jun 3, 2020

Add track for sub-bucket-aggs (#114)

1403e2b

Adds a rally track specifically for testing the performance of the bucket aggs when they are "sub" aggs. Relates to elastic/elasticsearch#55873

hub-cap pushed a commit to elastic/rally-tracks that referenced this pull request Jun 3, 2020

Add track for sub-bucket-aggs (#114)

7352037

Adds a rally track specifically for testing the performance of the bucket aggs when they are "sub" aggs. Relates to elastic/elasticsearch#55873

hub-cap pushed a commit to elastic/rally-tracks that referenced this pull request Jun 3, 2020

Add track for sub-bucket-aggs (#114)

cb670f1

Adds a rally track specifically for testing the performance of the bucket aggs when they are "sub" aggs. Relates to elastic/elasticsearch#55873

nik9000 mentioned this pull request Jun 9, 2020

Save memory when parent and child are not on top #57892

Merged

nik9000 mentioned this pull request Jun 10, 2020

Save memory when parent and child are not on top (backport #57892) #57944

Merged

nik9000 mentioned this pull request Jun 10, 2020

Save memory when rare_terms is not on top #57948

Merged

nik9000 added a commit that referenced this pull request Jun 12, 2020

Save memory when rare_terms is not on top (#57948)

933565d

This uses the optimization that we started making in #55873 for `rare_terms` to save a bit of memory when that aggregation is not on the top level.

nik9000 mentioned this pull request Jun 12, 2020

Save memory when rare_terms is not on top (backport of #57948) #58069

Merged

nik9000 mentioned this pull request Jun 15, 2020

Save memory when significant_text is not on top #58145

Merged

nik9000 mentioned this pull request Jun 18, 2020

Save memory when significant_text is not on top (#58145) #58364

Merged

jimczi mentioned this pull request Sep 10, 2020

Should we continue to account 5kb per bucket in the request circuit breaker #62240

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save memory when numeric terms agg is not top#55873

Save memory when numeric terms agg is not top#55873
nik9000 merged 21 commits intoelastic:masterfrom
nik9000:sneaky_delay

nik9000 commented Apr 28, 2020

Uh oh!

Uh oh!

nik9000 commented Apr 28, 2020

Uh oh!

nik9000 commented Apr 29, 2020

Uh oh!

nik9000 Apr 30, 2020

Uh oh!

nik9000 commented May 3, 2020

Uh oh!

nik9000 May 3, 2020

Uh oh!

nik9000 May 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

		@@ -158,19 +158,27 @@ public final InternalAggregation reducePipelines(

		@Override
		public InternalAggregation copyWithRewritenBuckets(Function<InternalAggregations, InternalAggregations> rewriter) {

Conversation

nik9000 commented Apr 28, 2020

Uh oh!

Uh oh!

nik9000 commented Apr 28, 2020

Uh oh!

nik9000 commented Apr 29, 2020

Uh oh!

nik9000 Apr 30, 2020

Choose a reason for hiding this comment

Uh oh!

nik9000 commented May 3, 2020

Uh oh!

nik9000 May 3, 2020

Choose a reason for hiding this comment

Uh oh!

nik9000 May 3, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants