Speed up terms agg when not force merged by nik9000 · Pull Request #71241 · elastic/elasticsearch

nik9000 · 2021-04-02T15:55:22Z

This speeds up the terms aggregation when it can't take the fancy
filters path, there is more than one segment, and any of those
segments have only a single value for the field. These three things are
super common.

Here are the performance change numbers:

|        50th percentile latency | date-histo-string-terms-via-global-ords | 3414.02 | 2632.01 | -782.015 | ms |
|        90th percentile latency | date-histo-string-terms-via-global-ords | 3470.91 | 2756.88 | -714.031 | ms |
|       100th percentile latency | date-histo-string-terms-via-global-ords | 3620.89 | 2875.79 | -745.102 | ms |
|   50th percentile service time | date-histo-string-terms-via-global-ords | 3410.15 | 2628.87 | -781.275 | ms |
|   90th percentile service time | date-histo-string-terms-via-global-ords | 3467.36 | 2752.43 | -714.933 | ms |   20%!!!!
|  100th percentile service time | date-histo-string-terms-via-global-ords | 3617.71 | 2871.63 | -746.083 | ms |

This works by hooking global ordinals into DocValues.unwrapSingleton.
Without this you could unwrap singletons if the segment's ordinals
aligned exactly with the global ordinals. If they didn't we'd return an
doc values iterator that you can't unwrap. Even if the segment ordinals
were singletons.

That speeds up the terms aggregator because we have a fast path we can
take if we have singletons. It was previously only working if we had a
single segment. Or if the segment's ordinals lined up exactly. Which,
for low cardinality fields is fairly common. So they might not benefit
from this quite as much as high cardinality fields.

Closes #71086

This speeds up the `terms` aggregation when it can't take the fancy `filters` path, there is more than one segment, and any of those segments have only a single value for the field. These three things are super common. Here are the performance change numbers: ``` | 50th percentile latency | date-histo-string-terms-via-global-ords | 3414.02 | 2632.01 | -782.015 | ms | | 90th percentile latency | date-histo-string-terms-via-global-ords | 3470.91 | 2756.88 | -714.031 | ms | | 100th percentile latency | date-histo-string-terms-via-global-ords | 3620.89 | 2875.79 | -745.102 | ms | | 50th percentile service time | date-histo-string-terms-via-global-ords | 3410.15 | 2628.87 | -781.275 | ms | | 90th percentile service time | date-histo-string-terms-via-global-ords | 3467.36 | 2752.43 | -714.933 | ms | 20%!!!! | 100th percentile service time | date-histo-string-terms-via-global-ords | 3617.71 | 2871.63 | -746.083 | ms | ``` This works by hooking global ordinals into `DocValues.unwrapSingleton`. Without this you could unwrap singletons *if* the segment's ordinals aligned exactly with the global ordinals. If they didn't we'd return an doc values iterator that you can't unwrap. Even if the segment ordinals were singletons. That speeds up the terms aggregator because we have a fast path we can take if we have singletons. It was previously only working if we had a single segment. Or if the segment's ordinals lined up exactly. Which, for low cardinality fields is fairly common. So they might not benefit from this quite as much as high cardinality fields. Closes elastic#71086

elasticmachine · 2021-04-02T15:55:25Z

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

imotov

Looks reasonable to me in general, but I think @not-napoleon should review it as well since he is much more familiar with this code.

imotov · 2021-04-05T15:03:14Z

...r/src/main/java/org/elasticsearch/index/fielddata/ordinals/GlobalOrdinalsIndexFieldData.java

+                                new SingletonGlobalOrdinalMapping(ordinalMap, singleton, atomicLookups, context.ord)
+                            );
+                        }
+                    }


This feels a bit brittle. Basically, SingletonGlobalOrdinalMapping seems to be a very specialized class that can only function if its constructor parameters satisfy these very specific criteria that are neither documented nor enforced in the class itself. I wonder if we could make this less brittle if we made SingletonGlobalOrdinalMapping constructor private and moved the logic and comment above into a factory method that would return null in case the conditions are not satisfied.

Yeah. The second test - that you can unwrap - is encoded in the ctor's signature. But the first bit - the value count - that totally should be guarded like you say.

not-napoleon

LGTM overall. Would like that constructor guard before merging.

not-napoleon · 2021-04-08T13:15:19Z

.../src/main/java/org/elasticsearch/index/fielddata/ordinals/SingletonGlobalOrdinalMapping.java

+    SingletonGlobalOrdinalMapping(OrdinalMap ordinalMap, SortedDocValues values, TermsEnum[] lookups, int segmentIndex) {
+        this.values = values;
+        this.lookups = lookups;
+        this.ordinalMap = ordinalMap;


I agree with Igor. At a minimum we should throw here if ordinalMap.getValueCount() >= MAX_INT. I think it's okay if we require callers to pre-check it rather than building a factory method (although a factory method would also be fine), but we shouldn't allow the construction of invalid objects.

not-napoleon · 2021-04-08T13:21:01Z

server/src/test/java/org/elasticsearch/index/fielddata/AbstractStringFieldDataTestCase.java

+        writer.addDocument(d);
+
+        d = new Document();
+        addField(d, "_id", "1");


Should the _id value here be 2? or are you intentionally duplicating IDs?

It should be 2, yeah.

not-napoleon · 2021-04-08T13:28:51Z

...r/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java

+                random(),
+                directory,
+                // Attempt to disable merging.
+                LuceneTestCase.newIndexWriterConfig(random(), new StandardAnalyzer()).setMergePolicy(NoMergePolicy.INSTANCE)


This is handy. I wouldn't mind a method in AggregatorTestCase to get a non-merging(ish) index writer.

not-napoleon · 2021-04-08T13:40:36Z

...r/src/test/java/org/elasticsearch/search/aggregations/bucket/terms/TermsAggregatorTests.java

        }
    }

+    public void testManySegmentsStillSingleton() throws IOException {


I'm a little unhappy that this test doesn't run through AggregatorTestCase.testCase, or at least use AggregatorTestCase.searchAndReduce. I think that's because you want to assert on the debug info which we don't currently make easy. If that's correct, I don't think we need to address it here, but I would like to leave a todo and maybe a ticket.

I've added something for this in #71503 and will update the PR when that one lands.

nik9000 · 2021-04-14T14:57:02Z

@imotov do you want to have another look at this one?

imotov

LGTM. Thanks!

nik9000 · 2021-04-15T12:27:39Z

Thanks for all the reviews!

This speeds up the `terms` aggregation when it can't take the fancy `filters` path, there is more than one segment, and any of those segments have only a single value for the field. These three things are super common. Here are the performance change numbers: ``` | 50th percentile latency | date-histo-string-terms-via-global-ords | 3414.02 | 2632.01 | -782.015 | ms | | 90th percentile latency | date-histo-string-terms-via-global-ords | 3470.91 | 2756.88 | -714.031 | ms | | 100th percentile latency | date-histo-string-terms-via-global-ords | 3620.89 | 2875.79 | -745.102 | ms | | 50th percentile service time | date-histo-string-terms-via-global-ords | 3410.15 | 2628.87 | -781.275 | ms | | 90th percentile service time | date-histo-string-terms-via-global-ords | 3467.36 | 2752.43 | -714.933 | ms | 20%!!!! | 100th percentile service time | date-histo-string-terms-via-global-ords | 3617.71 | 2871.63 | -746.083 | ms | ``` This works by hooking global ordinals into `DocValues.unwrapSingleton`. Without this you could unwrap singletons *if* the segment's ordinals aligned exactly with the global ordinals. If they didn't we'd return an doc values iterator that you can't unwrap. Even if the segment ordinals were singletons. That speeds up the terms aggregator because we have a fast path we can take if we have singletons. It was previously only working if we had a single segment. Or if the segment's ordinals lined up exactly. Which, for low cardinality fields is fairly common. So they might not benefit from this quite as much as high cardinality fields. Closes elastic#71086

This speeds up the `terms` aggregation when it can't take the fancy `filters` path, there is more than one segment, and any of those segments have only a single value for the field. These three things are super common. Here are the performance change numbers: ``` | 50th percentile latency | date-histo-string-terms-via-global-ords | 3414.02 | 2632.01 | -782.015 | ms | | 90th percentile latency | date-histo-string-terms-via-global-ords | 3470.91 | 2756.88 | -714.031 | ms | | 100th percentile latency | date-histo-string-terms-via-global-ords | 3620.89 | 2875.79 | -745.102 | ms | | 50th percentile service time | date-histo-string-terms-via-global-ords | 3410.15 | 2628.87 | -781.275 | ms | | 90th percentile service time | date-histo-string-terms-via-global-ords | 3467.36 | 2752.43 | -714.933 | ms | 20%!!!! | 100th percentile service time | date-histo-string-terms-via-global-ords | 3617.71 | 2871.63 | -746.083 | ms | ``` This works by hooking global ordinals into `DocValues.unwrapSingleton`. Without this you could unwrap singletons *if* the segment's ordinals aligned exactly with the global ordinals. If they didn't we'd return an doc values iterator that you can't unwrap. Even if the segment ordinals were singletons. That speeds up the terms aggregator because we have a fast path we can take if we have singletons. It was previously only working if we had a single segment. Or if the segment's ordinals lined up exactly. Which, for low cardinality fields is fairly common. So they might not benefit from this quite as much as high cardinality fields. Closes #71086

nik9000 added 2 commits April 2, 2021 11:30

Finish words

993304d

nik9000 added >enhancement :Analytics/Aggregations Aggregations v8.0.0 v7.13.0 labels Apr 2, 2021

nik9000 requested review from imotov and not-napoleon April 2, 2021 15:55

elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Apr 2, 2021

imotov reviewed Apr 5, 2021

View reviewed changes

not-napoleon approved these changes Apr 8, 2021

View reviewed changes

nik9000 added 5 commits April 12, 2021 12:55

Merge branch 'master' into investigate_71086

8ff369f

fixup

d6af4de

Merge branch 'master' into investigate_71086

6f6b974

Merge branch 'master' into investigate_71086

ff228e8

Do no need me

63c9c15

imotov approved these changes Apr 15, 2021

View reviewed changes

nik9000 merged commit 1d69985 into elastic:master Apr 15, 2021

nik9000 mentioned this pull request May 4, 2021

More debugging info for significant_text #72727

Merged

nik9000 mentioned this pull request May 24, 2021

[CI] Failure in SmokeTestMultiNodeClientYamlTestSuiteIT search.aggregation/20_terms/string profiler #60881

Closed

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

Conversation

nik9000 commented Apr 2, 2021

Uh oh!

elasticmachine commented Apr 2, 2021

Uh oh!

imotov 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!

not-napoleon 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!

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Apr 14, 2021

Uh oh!

imotov left a comment

Choose a reason for hiding this comment

Uh oh!

nik9000 commented Apr 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants