Reduce merge map memory overhead in the Variable Width Histogram Aggregation #59366

jamesdorfman · 2020-07-10T22:26:18Z

When a document which is distant from existing buckets gets collected, the variable_width_histogram will create a new bucket and then insert it into the ordered list of buckets.

Currently, a new merge map array is created to move this bucket. This is very expensive as there might be thousands of buckets.

This PR creates mergeBuckets(UnaryOperator<Long> mergeMap) methods in BucketsAggregator and MergingBucketsDefferingCollector, and updates the variable_width_histogram to use them. This eliminates the need to create an entire merge map array for each new bucket and reduces the memory overhead of the algorithm.

jamesdorfman · 2020-07-10T22:29:30Z

@nik9000 I recall you left a comment on #59094 mentioning a solution like this. Let me know if you had something different in mind!

elasticmachine · 2020-07-12T19:25:04Z

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

nik9000

It is what I was thinking of!

I left a few comments, the one that is really important:

Could you replace UnaryOperator<Long> with LongUnaryOperator? That'll prevent auto-boxing of the parameter.

Another comment: would you be up for writing a test for these two new methods? We don't have any tests for any of the methods around them which is a sad thing. The changes that you made look right, but I'm worried that one day we'll accidentally break things in sneaky ways if we don't have a test. I mostly write tests because I don't trust future-@nik9000 to remember things now-@nik9000 was thinking.

nik9000 · 2020-07-12T19:28:25Z

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

+            }
+        };
+
+        mergeBuckets(mergeMapOperator, newNumBuckets);


I think you could write this:

mergeBuckets(newNumbBuckets, buck -> mergeMap[Math.toIntExact(bucket)]);

nik9000 · 2020-07-12T19:30:50Z

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

+     * This only tidies up doc counts. Call {@link MergingBucketsDeferringCollector#mergeBuckets(UnaryOperator)} to
+     * merge the actual ordinals and doc ID deltas.
+     */
+    public final void mergeBuckets(UnaryOperator<Long> mergeMap, long newNumBuckets){


Two things, only one is important though:

Could you replace UnaryOperator<Long> with LongUnaryOperator? That'll prevent auto-boxing of the parameter.

Could you switch the order of the arguments? I think the call sites are a little prettier when the "function" argument is last, if possible.

nik9000 · 2020-07-12T19:31:02Z

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

        try (IntArray oldDocCounts = docCounts) {
            docCounts = bigArrays.newIntArray(newNumBuckets, true);
            docCounts.fill(0, newNumBuckets, 0);
            for (int i = 0; i < oldDocCounts.size(); i++) {


Could you swap to long i?

nik9000 · 2020-07-12T19:32:25Z

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

+    }
+
+    /**
+     * This only tidies up doc counts. Call {@link MergingBucketsDeferringCollector#mergeBuckets(UnaryOperator)} to


I think it is worth saying what the unary operator does here, that -1 means throw away and otherwise it is the destination index.

nik9000 · 2020-07-12T19:36:13Z

...org/elasticsearch/search/aggregations/bucket/histogram/VariableWidthHistogramAggregator.java

+                           return i + 1;
+                       }
+                    }
+                };


I'd have tried to write this:

mergeBuckets(numClusters, bucket -> { if (i < index) { // The clusters in range {0 ... idx - 1} don't move return 1; } if (i == numClusters - 1) { // The new cluster moves to index return i; } // The clusters in range {index ... numClusters - 1} shift forward return i = 1; });

I like the "inline function declaration" form of this because it makes it super obvious that it doesn't escape.

I also like early return instead of else if, but that is totally up to you. Its a matter of style and we don't have a standard.

I'm not sure if this would work, since this same function is used in the calls to bothBucketsAggregator::mergeBuckets and MergingBucketsDeferringCollector::mergeBuckets.

Ah! Well, what you have is just fine too.

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

…xes) in the mergeBuckets methods

jamesdorfman · 2020-07-16T16:56:55Z

Thanks for the feedback @nik9000 :)

I made all the changes you requested - mainly I updated the UnaryOperator to a LongUnaryOperator and I marked the methods that take a merge map as an array as deprecated.

I also added some test for both of the mergeBuckets methods. Sorry for the delay in addressing your comments. It took me a long time to figure out how to test MergingBucketsDeferringCollector::mergeBuckets. I'm not sure if I did this in the proper way, please let me know!

nik9000

Makes sense to me! I'll have a closer look at the test tomorrow.

Elasticmachine, ok to test.

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

nik9000 · 2020-07-17T00:03:09Z

...org/elasticsearch/search/aggregations/bucket/histogram/VariableWidthHistogramAggregator.java

+                           return i + 1;
+                       }
+                    }
+                };


Ah! Well, what you have is just fine too.

nik9000

I left a comment about one of the tests, but I think everything else is great! That test is just using Query in a rather strange way, and I think it'd be clearer to wrap the collector.

nik9000 · 2020-07-17T16:31:15Z

...java/org/elasticsearch/search/aggregations/bucket/MergingBucketsDeferringCollectorTests.java

+     * Usually all documents get collected into ordinal 0 unless they are part of a sub aggregation
+     * @return a query that collects the i'th document into bucket ordinal i
+     */
+    private Query getQueryToCollectIntoDifferentOrdinals() {


I think it'd be a little cleaner to do this by wrapping the collector that you pass to indexSearcher.search and just us MatchAllDocsQuery instead.

Fixed! I overrode one of the methods in MergingBucketsDeferringCollector and now it is a lot cleaner. Is this what you had in mind?

I'm not sure if wrapping the bucket collector directly would work, since the deferring collector stores the bucket ordinal before it calls the bucket collector's collect method.

That isn't quite what I was thinking. I think part of the problem is that I'm sort of stuck in a "do it like aggs do" mindset. In that mindset there are two collectors and the MergingBucketsDeferringCollector. One collector emulates the outer aggregator and calls to the MergingBucketsDeferringCollector's collect method, calling merge in some funny shape. And the other bucket emulates the inner aggregation and is just called by the aggregator.

I've got half of a patch on my laptop that does this but I'm probably going to stop for the day. I'll try and post it this weekend.

I was thinking something like this. There is a collector that just counts and a collector the distributes bucket ords and merges. And the MergingBucketsDeferringCollector sits between them.

Two neat things:

I think I found a bug! I don't think it actually comes up in production because you have to throw away buckets using the merge method while collecting buckets and I think we only do that in the rare_terms aggregators and they only merge after all the collections are done.

My test only really covers the variable-width histogram style of merging, not the rare_terms style. But I think that is pretty ok. Its is the harder style.

Sorry for my delayed response, I had a very hectic start of week!

Very cool, that is definitely a lot cleaner, thanks for the patch! I've added it and filed a corresponding bug.
That's a strange bug by the way...

Sorry for my delayed response, I had a very hectic start of week!

Its cool! Thanks for getting back to me!

That's a strange bug by the way...

It's sneaky! I'm hoping we really don't actually hit it. But it is the kind of thing that happens without unit tests, I think.

…ement a custom query, in MergingBucketsDeferringCollectorTests

…ests much cleaner

nik9000

Looks good to me!

nik9000 · 2020-07-23T12:47:41Z

@elasticmachine, ok to test

nik9000 · 2020-07-23T13:19:26Z

run elasticsearch-ci/bwc

nik9000 · 2020-07-24T13:00:38Z

@elasticmachine update branch

nik9000 · 2020-07-24T13:00:49Z

Let's see if that gets it unwedged.

nik9000 · 2020-07-24T14:08:44Z

That did it! I've merged and will backport to 7.x.

…egation (elastic#59366) When a document which is distant from existing buckets gets collected, the `variable_width_histogram` will create a new bucket and then insert it into the ordered list of buckets. Currently, a new merge map array is created to move this bucket. This is very expensive as there might be thousands of buckets. This PR creates `mergeBuckets(UnaryOperator<Long> mergeMap)` methods in `BucketsAggregator` and `MergingBucketsDefferingCollector`, and updates the `variable_width_histogram` to use them. This eliminates the need to create an entire merge map array for each new bucket and reduces the memory overhead of the algorithm.

jamesdorfman · 2020-07-24T17:28:26Z

Great, sounds good :) Thanks for reviewing!!!

…egation (#59366) (#60171) When a document which is distant from existing buckets gets collected, the `variable_width_histogram` will create a new bucket and then insert it into the ordered list of buckets. Currently, a new merge map array is created to move this bucket. This is very expensive as there might be thousands of buckets. This PR creates `mergeBuckets(UnaryOperator<Long> mergeMap)` methods in `BucketsAggregator` and `MergingBucketsDefferingCollector`, and updates the `variable_width_histogram` to use them. This eliminates the need to create an entire merge map array for each new bucket and reduces the memory overhead of the algorithm. Co-authored-by: James Dorfman <[email protected]>

Convert merge map to a UnaryOperator in VWH

4c70683

nik9000 self-requested a review July 12, 2020 19:24

nik9000 added :Analytics/Aggregations Aggregations v7.9.0 v8.0.0 labels Jul 12, 2020

elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Jul 12, 2020

nik9000 added the >refactoring label Jul 12, 2020

nik9000 requested changes Jul 12, 2020

View reviewed changes

nik9000 reviewed Jul 13, 2020

View reviewed changes

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

jamesdorfman added 3 commits July 15, 2020 02:30

Change UnaryOperator to LongUnaryOperator (and other similar style fi…

5e90bd4

…xes) in the mergeBuckets methods

Add tests for BucketsAggregator::mergeBuckets

77ebb63

Add tests for MergingBucketsDeferringCollector::mergeBuckets

3b32baf

jamesdorfman added 2 commits July 16, 2020 13:06

Fix formatting

4e8597f

Remove unused imports

71b1797

nik9000 reviewed Jul 17, 2020

View reviewed changes

jamesdorfman added 4 commits July 17, 2020 14:38

Wrap the MergingBucketsDeferringCollector and remove the need to impl…

8df9748

…ement a custom query, in MergingBucketsDeferringCollectorTests

Merge with master and resolve merge conflicts

db73958

Resolve merge conflict

e286f0a

Add patch from @nik9000 to make the MergingBucketsDeferringCollectorT…

822dfd6

…ests much cleaner

nik9000 approved these changes Jul 23, 2020

View reviewed changes

Merge branch 'master' into vwh_efficient_merge_map

c675f68

nik9000 merged commit 8b7c556 into elastic:master Jul 24, 2020

nik9000 added the backport pending label Jul 24, 2020

nik9000 removed the backport pending label Jul 27, 2020

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

Reduce merge map memory overhead in the Variable Width Histogram Aggregation #59366

Reduce merge map memory overhead in the Variable Width Histogram Aggregation #59366

Uh oh!

Conversation

jamesdorfman commented Jul 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesdorfman commented Jul 10, 2020

Uh oh!

elasticmachine commented Jul 12, 2020

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesdorfman commented Jul 16, 2020

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nik9000 left a comment

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Jul 23, 2020

Uh oh!

nik9000 commented Jul 23, 2020

Uh oh!

nik9000 commented Jul 24, 2020

Uh oh!

nik9000 commented Jul 24, 2020

Uh oh!

nik9000 commented Jul 24, 2020

Uh oh!

jamesdorfman commented Jul 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jamesdorfman commented Jul 10, 2020 •

edited

Loading