Optimize Min and Max BKD optimizations #44315

michalperlak · 2019-07-14T12:20:19Z

Applied optimizations for BKD flow in Min/Max aggregators from #44290.

MinAggregator - skip BKD optimization when no result found after 1024 lookups.
MaxAggregator - skip unnecessary conversions.

References #44290.

elasticmachine · 2019-07-15T08:25:27Z

Pinging @elastic/es-analytics-geo

polyfractal

Thanks @michalperlak! Left a few comments/questions and tagged @iverase to have a look, since he's more familiar with the BKD stuff than I am.

polyfractal · 2019-07-15T15:25:13Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java

@@ -219,6 +222,9 @@ public void visit(int docID) {

                @Override
                public void visit(int docID, byte[] packedValue) {
+                    if (lookupCounter.incrementAndGet() > MAX_BKD_LOOKUPS) {


I think if we exit early, we need to set result back to null to signal that we didn't find a value?

In addition, I think it makes more sense to put this check at the bottom of the method. If we have traversed the tree to this leaf value it seems wasteful to exit before checking if the value is the one we need.

I may be wrong, but it looks that the only possibility where returned result[0] is not null is the case when the liveDocs.get(docID) is true, so when the intersection process is terminated by early exit the returned value is null.

Ah sorry, I was looking at the line where result = new Number[1]; and missed that the return value is result[0] and not just result. All good 👍

polyfractal · 2019-07-15T15:26:39Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java

@@ -210,6 +212,7 @@ static Number findLeafMinValue(LeafReader reader, String fieldName, Function<byt
            return converter.apply(pointValues.getMinPackedValue());
        }
        final Number[] result = new Number[1];
+        final AtomicInteger lookupCounter = new AtomicInteger(0);


I could be wrong, but I think we can move the counter inside the PointValues.IntersectVisitor() anonymous class and avoid having to use an atomic. I'll defer to @iverase though, in case intersect behaves differently than I'm thinking.

yes, the tree is traversed in one single thread so it make more sense to add the counter inside the visitor and not using an atomic +1.

Great, atomic replaced with short field in the visitor.

polyfractal · 2019-07-15T15:27:37Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java

@@ -186,7 +186,10 @@ public void visit(int docID, byte[] packedValue) {
                if (liveDocs.get(docID)) {
                    // we need to collect all values in this leaf (the sort is ascending) where
                    // the last live doc is guaranteed to contain the max value for the segment.
-                    result[0] = converter.apply(packedValue);
+                    if (result[0] == null) {
+                        result[0] = new byte[packedValue.length];


I hadn't thought about this before, @iverase do you know if the packedValue is guaranteed to have same length for all values in the tree? Or is it a variable packing scheme?

This is ok, it is guaranteed to have the same length.

iverase

The only thing here is to change the use of atomic for a counter inside the visitor class.

iverase · 2019-07-15T16:03:29Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java

@@ -219,6 +222,9 @@ public void visit(int docID) {

                @Override
                public void visit(int docID, byte[] packedValue) {
+                    if (lookupCounter.incrementAndGet() > MAX_BKD_LOOKUPS) {


In addition, I think it makes more sense to put this check at the bottom of the method. If we have traversed the tree to this leaf value it seems wasteful to exit before checking if the value is the one we need.

iverase · 2019-07-15T16:04:46Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/MinAggregator.java

@@ -210,6 +212,7 @@ static Number findLeafMinValue(LeafReader reader, String fieldName, Function<byt
            return converter.apply(pointValues.getMinPackedValue());
        }
        final Number[] result = new Number[1];
+        final AtomicInteger lookupCounter = new AtomicInteger(0);


yes, the tree is traversed in one single thread so it make more sense to add the counter inside the visitor and not using an atomic +1.

iverase · 2019-07-15T16:05:30Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/MaxAggregator.java

@@ -186,7 +186,10 @@ public void visit(int docID, byte[] packedValue) {
                if (liveDocs.get(docID)) {
                    // we need to collect all values in this leaf (the sort is ascending) where
                    // the last live doc is guaranteed to contain the max value for the segment.
-                    result[0] = converter.apply(packedValue);
+                    if (result[0] == null) {
+                        result[0] = new byte[packedValue.length];


This is ok, it is guaranteed to have the same length.

…t method

polyfractal · 2019-07-16T17:07:18Z

@elasticmachine test this please

polyfractal · 2019-07-17T16:24:42Z

@elasticmachine test this please

iverase · 2019-07-18T10:02:16Z

@elasticmachine test this please

polyfractal · 2019-07-25T19:30:58Z

Sorry for the delay @michalperlak, was out on holiday. I think those test failures are unrelated, I'm going to merge in master to see if that helps unstick CI.

polyfractal · 2019-07-25T19:31:51Z

@elasticmachine update branch

polyfractal · 2019-07-25T19:32:20Z

@elasticmachine test this please

polyfractal · 2019-07-26T19:40:40Z

Merged, thanks @michalperlak!

michalperlak · 2019-07-26T21:28:34Z

Great, thank you :)

MinAggregator - skip BKD optimization when no result found after 1024 lookups. MaxAggregator - skip unnecessary conversions.

minglinlu · 2019-08-11T09:29:06Z

A good example!

Min/Max aggregators - optimize BKB path

47fa306

javanna added the :Analytics/Aggregations Aggregations label Jul 15, 2019

javanna added the >enhancement label Jul 15, 2019

javanna requested a review from polyfractal July 15, 2019 08:25

$polyfractal$

polyfractal reviewed Jul 15, 2019

View reviewed changes

iverase reviewed Jul 16, 2019

View reviewed changes

michalperlak added 2 commits July 16, 2019 18:59

Replace atomic with plain counter, move exit check to the end of visi…

fd95f7a

…t method

Remove unused import

1443120

michalperlak force-pushed the min_max_bkd_optimization branch from 9eaccd8 to 1443120 Compare July 16, 2019 19:59

Merge branch 'master' into min_max_bkd_optimization

a70d8eb

$@polyfractal$ polyfractal merged commit 20284b8 into elastic:master Jul 26, 2019

$@polyfractal$ polyfractal added v7.4.0 v8.0.0 labels Jul 26, 2019

michalperlak deleted the min_max_bkd_optimization branch July 26, 2019 21:28

polyfractal pushed a commit that referenced this pull request Jul 29, 2019

$@polyfractal$

Optimize Min and Max BKD optimizations (#44315)

245c9b7

MinAggregator - skip BKD optimization when no result found after 1024 lookups. MaxAggregator - skip unnecessary conversions.

jkakavas pushed a commit that referenced this pull request Jul 31, 2019

Optimize Min and Max BKD optimizations (#44315)

b9a17b4

MinAggregator - skip BKD optimization when no result found after 1024 lookups. MaxAggregator - skip unnecessary conversions.

$@polyfractal$ polyfractal mentioned this pull request Aug 26, 2019

Optimize Min and Max BKD optimizations :) #44290

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

Optimize Min and Max BKD optimizations #44315

Optimize Min and Max BKD optimizations #44315

michalperlak commented Jul 14, 2019

elasticmachine commented Jul 15, 2019

$@polyfractal$ polyfractal left a comment

$@polyfractal$ polyfractal Jul 15, 2019

iverase Jul 15, 2019

michalperlak Jul 15, 2019

$@polyfractal$ polyfractal Jul 15, 2019

$@polyfractal$ polyfractal Jul 15, 2019

iverase Jul 15, 2019

michalperlak Jul 16, 2019

$@polyfractal$ polyfractal Jul 15, 2019

iverase Jul 15, 2019

$@polyfractal$ polyfractal Jul 16, 2019

iverase left a comment

iverase Jul 15, 2019

iverase Jul 15, 2019

iverase Jul 15, 2019

polyfractal commented Jul 16, 2019

polyfractal commented Jul 17, 2019

iverase commented Jul 18, 2019

polyfractal commented Jul 25, 2019 •

edited

Loading

polyfractal commented Jul 25, 2019

polyfractal commented Jul 25, 2019

polyfractal commented Jul 26, 2019

michalperlak commented Jul 26, 2019

minglinlu commented Aug 11, 2019

Optimize Min and Max BKD optimizations #44315

Optimize Min and Max BKD optimizations #44315

Conversation

michalperlak commented Jul 14, 2019

elasticmachine commented Jul 15, 2019

polyfractal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iverase left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

polyfractal commented Jul 16, 2019

polyfractal commented Jul 17, 2019

iverase commented Jul 18, 2019

polyfractal commented Jul 25, 2019 • edited Loading

polyfractal commented Jul 25, 2019

polyfractal commented Jul 25, 2019

polyfractal commented Jul 26, 2019

michalperlak commented Jul 26, 2019

minglinlu commented Aug 11, 2019

$@polyfractal$ polyfractal left a comment

polyfractal commented Jul 25, 2019 •

edited

Loading