Skip to content

value_count Aggregation optimization #54854

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 10, 2020
Merged

value_count Aggregation optimization #54854

merged 2 commits into from
Apr 10, 2020

Conversation

xjtushilei
Copy link
Contributor

We found some problems during the test.

Data: 200Million docs, 1 shard,0 replica。

Hit avg sum value_count
20k 38ms 33ms 63ms
200k 127ms 125ms 334ms
2Million 789ms 729ms 3.176s
20Million 4.2s 3.239s 22.787s
200Million(100%) 21s 22s 154.917s

The performance of avg, sum and other is very close when performing statistics, but the performance of value_count has always been poor, even not on an order of magnitude. Based on some common-sense knowledge, we think that value_count and sum are similar operations, and the time consumed should be the same. Therefore, we have discussed the agg of value_count.

The principle of counting in es is to traverse the field of each document. If the field is an ordinary value, the count value is increased by 1. If it is an array type, the count value is increased by n. However, the problem lies in traversing each document and taking out the field, which changes from disk to an object in the Java language. We summarize its current problems with Elasticsearch as:

  • Number cast to string overhead, and GC problems caused by a large number of strings
  • After the number type is converted to string, sorting and other unnecessary operations are performed

Here is the proof of type conversion overhead.

// Java long to string source code, getChars is very time-consuming.
public static String toString(long i) {
        int size = stringSize(i);
        if (COMPACT_STRINGS) {
            byte[] buf = new byte[size];
            getChars(i, size, buf);
            return new String(buf, LATIN1);
        } else {
            byte[] buf = new byte[size * 2];
            StringUTF16.getChars(i, size, buf);
            return new String(buf, UTF16);
        }
}   
test type average min max sum
double->long 32.2ns 28ns 0.024ms 3.22s
long->double 31.9ns 28ns 0.036ms 3.19s
long->String 163.8ns 93ns 1921ms 16.3s

#36752 The program heat map shows that the toString time is particularly serious.

optimization

Our optimization code is actually very simple. It is to manage different types separately, instead of uniformly converting to string unified processing. We added type identification in ValueCountAggregator, and made special treatment for number and geopoint types to cancel their type conversion. Because the string type is reduced and the string constant is reduced, the improvement effect is very obvious.

result

Hit avg sum value_countdouble before value_countdouble after value_countkeyword before value_countkeyword after value_countgeo_point before value_countgeo_point after
20k 38ms 33ms 63ms 26ms 30ms 30ms 38ms 15ms
200k 127ms 125ms 334ms 78ms 116ms 99s 278ms 31ms
2Million 789ms 729ms 3.176s 439ms 348ms 386ms 3.365s 178ms
20Million 4.2s 3.239s 22.787s 2.7s 2.5s 2.6s 25.192s 1.278s
200Million(100%) 21s 22s 154.917s 18.99s 19s 20s 168.971s 9.093s
  • The results are more in line with common sense. valuecount is about the same as avg, sum, etc., or even lower than these. Previously, valuecount was much larger than avg and sum, and it was not even an order of magnitude when the amount of data was large.
  • When calculating numeric types such as double and long, the performance is improved by about 8 to 9 times; when calculating the geo_point type, the performance is improved by 18 to 20 times.

@cla-checker-service
Copy link

cla-checker-service bot commented Apr 7, 2020

💚 CLA has been signed

@xjtushilei xjtushilei changed the title count Aggregation optimization value_count Aggregation optimization Apr 7, 2020
@elasticmachine
Copy link
Collaborator

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

@nik9000
Copy link
Member

nik9000 commented Apr 7, 2020

Nice!

@elasticmachine, ok to test.

@xjtushilei, could you sign the CLA so we can get this in and work on it?

@@ -66,6 +66,34 @@ public LeafBucketCollector getLeafCollector(LeafReaderContext ctx,
return LeafBucketCollector.NO_OP_COLLECTOR;
}
final BigArrays bigArrays = context.bigArrays();

if (valuesSource instanceof ValuesSource.Numeric) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be nice to have a "value counting" member in ValuesSource but I'd be happy to merge this as is and open up a follow up myself to do that. Or you can, if you want @xjtushilei.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As much as it is annoying to put something just for one agg in ValuesSource, I think it'd be nice to have a something in there just so it is more obvious that we do these sorts of things to the values.

Another option is to use the values source refactor to plug in different count implementations. That'd probably be more in line with the direction we're going now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way, I'm happy to take this as is and do the twisting around myself in a follow up change and ping you for review.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be nice to have a "value counting" member in ValuesSource but I'd be happy to merge this as is and open up a follow up myself to do that. Or you can, if you want @xjtushilei.

yes, you can do it.

@nik9000
Copy link
Member

nik9000 commented Apr 7, 2020

@xjtushilei, looks like Jenkins hit a compile error. Are you ok to fix it?

@xjtushilei
Copy link
Contributor Author

xjtushilei commented Apr 8, 2020

Nice!

@elasticmachine, ok to test.

@xjtushilei, could you sign the CLA so we can get this in and work on it?

i have signed the CLA with Email "[email protected]",the email is Email of GIT submitted records.
maybe it is not my github account email?

Today, I have added this email to my GitHub account email.

@xjtushilei
Copy link
Contributor Author

@xjtushilei, looks like Jenkins hit a compile error. Are you ok to fix it?

I tested it in 6.x branch with no compilation errors , and the latest master branch has not been compiled.

When I tried in the master branch, ". / gradlew idea" was too slow. Could you fix it ?

@nik9000
Copy link
Member

nik9000 commented Apr 8, 2020

When I tried in the master branch, ". / gradlew idea" was too slow. Could you fix it ?

I think that ./gradlew idea has been turned off in master. I'm probably the not the right person to help because I use Eclipse, but CONTRIBUTING.md says to import it as a gradle project. If you don't want to go through with all that I'd be happy to finish the PR up for you, but if you want to keep contributing it is probably worth it to get gradle resolved.

Today, I have added this email to my GitHub account email.

One of those things did the trick. I never remember all of the rules for the CLA system. Thanks for making those changes! It is all good on my side now.

@nik9000
Copy link
Member

nik9000 commented Apr 9, 2020

@xjtushilei, I've pushed a fix for the failing test. That test actually shows that this is a breaking change but in a super esoteric way: if you run with a script the input will change from a String to the appropriate type. In the case of the issue in comes back as some kind of number. I'll still backport it to 7.8.0 and add a breaking change note. I don't believe it is super likely to break folks so I'm ok making a breaking change in a minor for this.

@xjtushilei
Copy link
Contributor Author

@xjtushilei, I've pushed a fix for the failing test. That test actually shows that this is a breaking change but in a super esoteric way: if you run with a script the input will change from a String to the appropriate type. In the case of the issue in comes back as some kind of number. I'll still backport it to 7.8.0 and add a breaking change note. I don't believe it is super likely to break folks so I'm ok making a breaking change in a minor for this.

That sounds cool.

@nik9000 nik9000 merged commit 8e8ce96 into elastic:master Apr 10, 2020
@nik9000
Copy link
Member

nik9000 commented Apr 10, 2020

OK! Jenkins has approved this so I've merged and am backporting. I took some liberties turning your PR description into a commit message. You should see a backport PR sometime in the next hour or so.

nik9000 pushed a commit to nik9000/elasticsearch that referenced this pull request Apr 10, 2020
We found some problems during the test.

Data: 200Million docs, 1 shard, 0 replica

    hits    |   avg   |   sum   | value_count |
----------- | ------- | ------- | ----------- |
     20,000 |   .038s |   .033s |       .063s |
    200,000 |   .127s |   .125s |       .334s |
  2,000,000 |   .789s |   .729s |      3.176s |
 20,000,000 |  4.200s |  3.239s |     22.787s |
200,000,000 | 21.000s | 22.000s |    154.917s |

The performance of `avg`, `sum` and other is very close when performing
statistics, but the performance of `value_count` has always been poor,
even not on an order of magnitude. Based on some common-sense knowledge,
we think that `value_count` and sum are similar operations, and the time
consumed should be the same. Therefore, we have discussed the agg
of `value_count`.

The principle of counting in es is to traverse the field of each
document. If the field is an ordinary value, the count value is
increased by 1. If it is an array type, the count value is increased
by n. However, the problem lies in traversing each document and taking
out the field, which changes from disk to an object in the Java
language. We summarize its current problems with Elasticsearch as:

- Number cast to string overhead, and GC problems caused by a large
  number of strings
- After the number type is converted to string, sorting and other
  unnecessary operations are performed

Here is the proof of type conversion overhead.

```
// Java long to string source code, getChars is very time-consuming.
public static String toString(long i) {
        int size = stringSize(i);
        if (COMPACT_STRINGS) {
            byte[] buf = new byte[size];
            getChars(i, size, buf);
            return new String(buf, LATIN1);
        } else {
            byte[] buf = new byte[size * 2];
            StringUTF16.getChars(i, size, buf);
            return new String(buf, UTF16);
        }
}
```

  test type  | average |  min |     max     |   sum
------------ | ------- | ---- | ----------- | -------
double->long |  32.2ns | 28ns |     0.024ms |  3.22s
long->double |  31.9ns | 28ns |     0.036ms |  3.19s
long->String | 163.8ns | 93ns |  1921    ms | 16.3s

particularly serious.

Our optimization code is actually very simple. It is to manage different
types separately, instead of uniformly converting to string unified
processing. We added type identification in ValueCountAggregator, and
made special treatment for number and geopoint types to cancel their
type conversion. Because the string type is reduced and the string
constant is reduced, the improvement effect is very obvious.

    hits    |   avg   |   sum   | value_count | value_count | value_count | value_count | value_count | value_count |
            |         |         |    double   |    double   |   keyword   |   keyword   |  geo_point  |  geo_point  |
            |         |         |   before    |    after    |   before    |    after    |   before    |    after    |
----------- | ------- | ------- | ----------- | ----------- | ----------- | ----------- | ----------- | ----------- |
     20,000 |     38s |   .033s |       .063s |       .026s |       .030s |       .030s |       .038s |       .015s |
    200,000 |    127s |   .125s |       .334s |       .078s |       .116s |       .099s |       .278s |       .031s |
  2,000,000 |    789s |   .729s |      3.176s |       .439s |       .348s |       .386s |      3.365s |       .178s |
 20,000,000 |  4.200s |  3.239s |     22.787s |      2.700s |      2.500s |      2.600s |     25.192s |      1.278s |
200,000,000 | 21.000s | 22.000s |    154.917s |     18.990s |     19.000s |     20.000s |    168.971s |      9.093s |

- The results are more in line with common sense. `value_count` is about
  the same as `avg`, `sum`, etc., or even lower than these. Previously,
  `value_count` was much larger than avg and sum, and it was not even an
  order of magnitude when the amount of data was large.
- When calculating numeric types such as `double` and `long`, the
  performance is improved by about 8 to 9 times; when calculating the
  `geo_point` type, the performance is improved by 18 to 20 times.
nik9000 added a commit that referenced this pull request Apr 10, 2020
We found some problems during the test.

Data: 200Million docs, 1 shard, 0 replica

    hits    |   avg   |   sum   | value_count |
----------- | ------- | ------- | ----------- |
     20,000 |   .038s |   .033s |       .063s |
    200,000 |   .127s |   .125s |       .334s |
  2,000,000 |   .789s |   .729s |      3.176s |
 20,000,000 |  4.200s |  3.239s |     22.787s |
200,000,000 | 21.000s | 22.000s |    154.917s |

The performance of `avg`, `sum` and other is very close when performing
statistics, but the performance of `value_count` has always been poor,
even not on an order of magnitude. Based on some common-sense knowledge,
we think that `value_count` and sum are similar operations, and the time
consumed should be the same. Therefore, we have discussed the agg
of `value_count`.

The principle of counting in es is to traverse the field of each
document. If the field is an ordinary value, the count value is
increased by 1. If it is an array type, the count value is increased
by n. However, the problem lies in traversing each document and taking
out the field, which changes from disk to an object in the Java
language. We summarize its current problems with Elasticsearch as:

- Number cast to string overhead, and GC problems caused by a large
  number of strings
- After the number type is converted to string, sorting and other
  unnecessary operations are performed

Here is the proof of type conversion overhead.

```
// Java long to string source code, getChars is very time-consuming.
public static String toString(long i) {
        int size = stringSize(i);
        if (COMPACT_STRINGS) {
            byte[] buf = new byte[size];
            getChars(i, size, buf);
            return new String(buf, LATIN1);
        } else {
            byte[] buf = new byte[size * 2];
            StringUTF16.getChars(i, size, buf);
            return new String(buf, UTF16);
        }
}
```

  test type  | average |  min |     max     |   sum
------------ | ------- | ---- | ----------- | -------
double->long |  32.2ns | 28ns |     0.024ms |  3.22s
long->double |  31.9ns | 28ns |     0.036ms |  3.19s
long->String | 163.8ns | 93ns |  1921    ms | 16.3s

particularly serious.

Our optimization code is actually very simple. It is to manage different
types separately, instead of uniformly converting to string unified
processing. We added type identification in ValueCountAggregator, and
made special treatment for number and geopoint types to cancel their
type conversion. Because the string type is reduced and the string
constant is reduced, the improvement effect is very obvious.

    hits    |   avg   |   sum   | value_count | value_count | value_count | value_count | value_count | value_count |
            |         |         |    double   |    double   |   keyword   |   keyword   |  geo_point  |  geo_point  |
            |         |         |   before    |    after    |   before    |    after    |   before    |    after    |
----------- | ------- | ------- | ----------- | ----------- | ----------- | ----------- | ----------- | ----------- |
     20,000 |     38s |   .033s |       .063s |       .026s |       .030s |       .030s |       .038s |       .015s |
    200,000 |    127s |   .125s |       .334s |       .078s |       .116s |       .099s |       .278s |       .031s |
  2,000,000 |    789s |   .729s |      3.176s |       .439s |       .348s |       .386s |      3.365s |       .178s |
 20,000,000 |  4.200s |  3.239s |     22.787s |      2.700s |      2.500s |      2.600s |     25.192s |      1.278s |
200,000,000 | 21.000s | 22.000s |    154.917s |     18.990s |     19.000s |     20.000s |    168.971s |      9.093s |

- The results are more in line with common sense. `value_count` is about
  the same as `avg`, `sum`, etc., or even lower than these. Previously,
  `value_count` was much larger than avg and sum, and it was not even an
  order of magnitude when the amount of data was large.
- When calculating numeric types such as `double` and `long`, the
  performance is improved by about 8 to 9 times; when calculating the
  `geo_point` type, the performance is improved by 18 to 20 times.
@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants