Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Increase step between checks for cancellation #53712

Merged
merged 2 commits into from
Mar 18, 2020
Merged

Increase step between checks for cancellation #53712

merged 2 commits into from
Mar 18, 2020

Conversation

matriv
Copy link
Contributor

@matriv matriv commented Mar 18, 2020

The introduction of the ExitableDirectoryReader showed increase of
latencies for range queries using pointvalues.

Check for cancellation every 1024 docs instead of every 15 to lower
the impact of the check in query's performance.

Follows: #52822
Fixes: #53496

@matriv
Increase step between checks for cancellation
a1839d0 
Check for cancellation every 1024 docs instead of every 15 to lower
the impact of the check in query's performance.

Fixes: elastic#53496
@matriv matriv added :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.7.0 labels Mar 18, 2020
@matriv matriv requested a review from jimczi March 18, 2020 08:51
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

jimczi
jimczi approved these changes Mar 18, 2020
View reviewed changes
Copy link
Contributor

@jimczi jimczi left a comment

Choose a reason for hiding this comment

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

I left one comment that needs to be addressed

server/src/main/java/org/elasticsearch/search/internal/ExitableDirectoryReader.java Outdated
@@ -245,7 +245,7 @@ public int getDocCount() {

private static class ExitableIntersectVisitor implements PointValues.IntersectVisitor {

private static final int MAX_CALLS_BEFORE_QUERY_TIMEOUT_CHECK = (1 << 4) - 1; // 15
private static final int MAX_CALLS_BEFORE_QUERY_TIMEOUT_CHECK = 1 << 10; // 1024
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private static final int MAX_CALLS_BEFORE_QUERY_TIMEOUT_CHECK = 1 << 10; // 1024
private static final int MAX_CALLS_BEFORE_QUERY_TIMEOUT_CHECK = (1 << 10) - 1; // 1023

@matriv matriv merged commit 6b5fc35 into elastic:master Mar 18, 2020
@matriv matriv deleted the fix-53496 branch March 18, 2020 13:52
matriv added a commit that referenced this pull request Mar 18, 2020
@matriv
Increase step between checks for cancellation (#53712)
d56dee5 
The introduction of the ExitableDirectoryReader showed increase of
latencies for range queries using pointvalues.

Check for cancellation every 1024 docs instead of every 15 to lower
the impact of the check in query's performance.

Follows: #52822
Fixes: #53496
(cherry picked from commit 6b5fc35)
@matriv
Copy link
Contributor Author

matriv commented Mar 18, 2020 •
edited
Loading

Benchmark results showing the effect of the change:

      50th percentile latency | range | 799.665 | 630.320 | -169.344
      90th percentile latency | range | 819.129 | 635.710 | -183.419
      99th percentile latency | range | 824.655 | 667.478 | -157.177
     100th percentile latency | range | 824.683 | 668.240 | -156.443
 50th percentile service time | range | 799.465 | 629.931 | -169.534
 90th percentile service time | range | 818.915 | 635.316 | -183.599
 99th percentile service time | range | 824.430 | 667.118 | -157.312
100th percentile service time | range | 824.464 | 667.875 | -156.589

@matriv
Copy link
Contributor Author

matriv commented Mar 19, 2020

Here are the results between the status before the ExitableDirectoryReader and after but with the change to check every 1023 docs instead of every 15:

      50th percentile latency |  range |  641.796 |  642.785 |   0.98898
      90th percentile latency |  range |  643.440 |  649.266 |   5.82666
      99th percentile latency |  range |  650.121 |  692.947 |  42.82630
     100th percentile latency |  range |  650.332 |  712.341 |  62.00830
 50th percentile service time |  range |  641.418 |  642.400 |   0.98233
 90th percentile service time |  range |  643.063 |  648.894 |   5.83093
 99th percentile service time |  range |  649.741 |  692.553 |  42.81110
100th percentile service time |  range |  649.957 |  711.938 |  61.98130

FYI @jimczi

@codebrain codebrain mentioned this pull request Apr 1, 2020
Closed
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jimczi jimczi jimczi approved these changes

Assignees
No one assigned
Labels
:Search/Search Search-related issues that do not fall into other categories v7.7.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ExitableDirectoryReader slowdowns intensive points and terms query
4 participants
@matriv @elasticmachine @jimczi @jakelandis