Remove shortcutTotalHitCount optimization #89047
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Our TopDocsCollectorContext has an optimization to try and avoid counting total hit count for queries like match all docs, term query and field exists query, relying on the statistics from each segment instead. This optimization has been recently streamlined in lucene through the introduction of Weight#count and now leveraged directly by TotalHitCountCollector in lucene with https://issues.apache.org/jira/browse/LUCENE-10620 , later complemented by elastic#88396 within Elasticsearch. With this, we can remove this internal optimization and instead leverage the default lucene behaviour which covers more queries and will be possibly expanded in the future as well. Closes elastic#81034
javanna
added
:Search/Search
|
Pinging @elastic/es-search (Team:Search) |
dnhatn
approved these changes
Aug 2, 2022
jpountz
reviewed
Aug 3, 2022
| totalHitsSupplier = () -> topDocsSupplier.get().totalHits; | ||
| } else { | ||
| // don't compute hit counts via the collector | ||
| topDocsCollector = createCollector(sortAndFormats, numHits, searchAfter, 1); |
There was a problem hiding this comment.
I like this change, this is the only place where I think we may be seeing regressions. Before your change, we would tell the top docs collector that it doesn't have to count hits at all (the 1 here) since we computed it up-front. With your change, we would always count trackTotalHitsUpTo documents, which delays a bit skipping.
Maybe we could run benchmarks with the geonames track to quantify the impact. I'll be especially interested on the impact on the default and term queries.
|
I revived this PR and ran the geonames benchmarks. Nothing from the benchmarks results caught my eye, can you double check too @jpountz ? Would you like me to run other benchmarks? Baseline (current main without my change): Results from my branch which includes the fix: |
|
Sorry for the lag @javanna and thanks for running benchmarks. If the slowdown to |
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Feb 27, 2023
This reverts commit 283f8ac.
javanna
added a commit
that referenced
this pull request
Feb 27, 2023
This reverts commit 283f8ac in the 8.7 branch (#89047). We have found a performance regression around executing search requests with size greater than zero that hold queries that can shortcut their total hit count, like term and match_all. The previous shortcut total hit count optimization done in ES was able to shortcut those while the top score docs collector in Lucene does not support that. This can be improved further on main but for 8.7 we are going the safe path of reverting and leaving things how they were.
javanna
added a commit
to javanna/elasticsearch
that referenced
this pull request
Mar 29, 2023
We have removed shortcut total hit count with elastic#89047 and later noticed a couple of benchmark regressions. While we have moved to skip counting when possible when not collecting hits (e.g. size=0), which is the case where Elasticsearch uses TotalHitCountCollector and the shortcutting is supported natively in Lucene. For the case where hits are collected, the total hit count is counted as part of the collection in TopScoreDocCollector and TopFieldCollector, where Lucene does not support skipping the counting as it is hard to determine whether more competitive hits need to be collected or not. The previous change caused a regression specifically when collecting hits because we ended up removing our manual shortcut in favour of counting which causes overhead. With this change we reintroduce the shortcut total hit count method, and only use it when strictly necessary. When size is 0, we rely entirely on Lucene to shortcut the total hit counting, while when hits are collected we do it our way, for now. While at it, a few more tests are added to cover for situations that were not covered before.
javanna
added a commit
that referenced
this pull request
Mar 29, 2023
javanna
added a commit
that referenced
this pull request
Mar 30, 2023
We have removed shortcut total hit count with #89047 and later noticed a couple of benchmark regressions, which made us restore our shortcut total hit count mechanism. When not collecting hits (e.g. size=0) we can leverage Lucene skipping mechanism instead of our handmade shortcut total hit count, as Elasticsearch uses TotalHitCountCollector which calls Weight#count. The advantage of this is that it supports shortcutting for many more queries than the only 3 which our manual mechanism supports (match_all, term and field exists). While at it, a few more tests are added to cover for situations that were not covered before.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Our TopDocsCollectorContext has an optimization to try and avoid counting total hit count for queries like match all docs, term query and field exists query, relying on the statistics from each segment instead. This optimization has been recently streamlined in lucene through the introduction of
Weight#countand then leveraged directly byTotalHitCountCollectorin lucene with https://issues.apache.org/jira/browse/LUCENE-10620 , later complemented by #88396 within Elasticsearch.With this, we can remove the internal optimization and instead leverage the default lucene behaviour which covers more queries and will be possibly expanded in the future as well.
Closes #81034