Reduce performance impact of ExitableDirectoryReader #53978
Merged
Conversation
This file contains hidden or 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
matriv
added
:Search/Search
|
Pinging @elastic/es-search (:Search/Search) |
jimczi
reviewed
Mar 23, 2020
| @@ -276,7 +276,7 @@ public void visit(int docID, byte[] packedValue) throws IOException { | |||
|
|
|||
| @Override | |||
| public PointValues.Relation compare(byte[] minPackedValue, byte[] maxPackedValue) { | |||
| queryCancellation.checkCancelled(); | |||
| checkAndThrowWithSampling(); | |||
There was a problem hiding this comment.
I don't think we should throttle this. We don't have enough evidence that it's slowing down the request so I'd keep it as is for now.
| this(reader, similarity, queryCache, queryCachingPolicy, new MutableQueryTimeout()); | ||
| QueryCache queryCache, QueryCachingPolicy queryCachingPolicy, | ||
| boolean wrapWithExitableDirReader) throws IOException { | ||
| this(reader, similarity, queryCache, queryCachingPolicy, wrapWithExitableDirReader ? new MutableQueryTimeout() : null); |
There was a problem hiding this comment.
We use the cancellable even if the low level cancellation is set to false (query timeout option and bulk scorer) so we should never set the cancellable to null ?
There was a problem hiding this comment.
Yep, sure, I just pushed it to check some other compilation stuff. it's not ready yet, but thx!
Benchmarking showed that the effect of the ExitableDirectoryReader is reduced considerably when checking every 4095 docs. Moreover, set the cancellable task before calling QueryPhase.preProcess() and make sure we don't wrap with an ExitableDirectoryReader at all when lowLevelCancellation() is set to false to avoid completely any performance impact. Follows: elastic#52822 Follows: elastic#53166 Follows: elastic#53496
matriv
force-pushed
the
fix-perf-exitable
branch
from
701af61 to
8f23bc7
Compare
|
Benchmark results: Every 511 docs against before the ExitableDirectoryReader: Every 1023 docs against before the ExitableDirectoryReader: Every 2047 docs against before the ExitableDirectoryReader: Every 4095 docs against before the ExitableDirectoryReader: Every 8191 docs against before the ExitableDirectoryReader: |
|
I'm leaning towards 8191 instead of 4095 (current status in the PR). |
jimczi
approved these changes
Mar 23, 2020
matriv
added a commit
that referenced
this pull request
Mar 23, 2020
Benchmarking showed that the effect of the ExitableDirectoryReader is reduced considerably when checking every 8191 docs. Moreover, set the cancellable task before calling QueryPhase#preProcess() and make sure we don't wrap with an ExitableDirectoryReader at all when lowLevelCancellation is set to false to avoid completely any performance impact. Follows: #52822 Follows: #53166 Follows: #53496 (cherry picked from commit cdc377e)
38 tasks
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.
Benchmarking showed that the effect of the
ExitableDirectoryReaderis reduced considerably when checking every 8191 docs. Moreover,
set the cancellable task before calling
QueryPhase#preProcess()and make sure we don't wrap with an
ExitableDirectoryReaderat allwhen
lowLevelCancellationis set to false to avoid completely anyperformance impact.
Follows: #52822
Follows: #53166
Follows: #53496