Skip to content

Commit 8f23bc7

Browse files
matrivmatriv
committed
Reduce performance impact of ExitableDirectoryReader
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: #52822 Follows: #53166 Follows: #53496
1 parent 8264bdd commit 8f23bc7

File tree

13 files changed

+61
-67
lines changed

13 files changed

+61
-67
lines changed

‎server/src/main/java/org/elasticsearch/search/DefaultSearchContext.java‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ final class DefaultSearchContext extends SearchContext {
9898
private final QuerySearchResult queryResult;
9999
private final FetchSearchResult fetchResult;
100100
private final float queryBoost;
101+
private final boolean lowLevelCancellation;
101102
private TimeValue timeout;
102103
// terminate after count
103104
private int terminateAfter = DEFAULT_TERMINATE_AFTER;
@@ -118,7 +119,6 @@ final class DefaultSearchContext extends SearchContext {
118119
private int trackTotalHitsUpTo = SearchContext.DEFAULT_TRACK_TOTAL_HITS_UP_TO;
119120
private FieldDoc searchAfter;
120121
private CollapseContext collapse;
121-
private boolean lowLevelCancellation;
122122
// filter for sliced scroll
123123
private SliceBuilder sliceBuilder;
124124
private SearchShardTask task;
@@ -156,7 +156,7 @@ final class DefaultSearchContext extends SearchContext {
156156
DefaultSearchContext(SearchContextId id, ShardSearchRequest request, SearchShardTarget shardTarget,
157157
Engine.Searcher engineSearcher, ClusterService clusterService, IndexService indexService,
158158
IndexShard indexShard, BigArrays bigArrays, LongSupplier relativeTimeSupplier, TimeValue timeout,
159-
FetchPhase fetchPhase) throws IOException {
159+
FetchPhase fetchPhase, boolean lowLevelCancellation) throws IOException {
160160
this.id = id;
161161
this.request = request;
162162
this.fetchPhase = fetchPhase;
@@ -172,12 +172,13 @@ final class DefaultSearchContext extends SearchContext {
172172
this.indexService = indexService;
173173
this.clusterService = clusterService;
174174
this.searcher = new ContextIndexSearcher(engineSearcher.getIndexReader(), engineSearcher.getSimilarity(),
175-
engineSearcher.getQueryCache(), engineSearcher.getQueryCachingPolicy());
175+
engineSearcher.getQueryCache(), engineSearcher.getQueryCachingPolicy(), lowLevelCancellation);
176176
this.relativeTimeSupplier = relativeTimeSupplier;
177177
this.timeout = timeout;
178178
queryShardContext = indexService.newQueryShardContext(request.shardId().id(), searcher,
179179
request::nowInMillis, shardTarget.getClusterAlias());
180180
queryBoost = request.indexBoost();
181+
this.lowLevelCancellation = lowLevelCancellation;
181182
}
182183

183184
@Override
@@ -563,10 +564,6 @@ public boolean lowLevelCancellation() {
563564
return lowLevelCancellation;
564565
}
565566

566-
public void lowLevelCancellation(boolean lowLevelCancellation) {
567-
this.lowLevelCancellation = lowLevelCancellation;
568-
}
569-
570567
@Override
571568
public FieldDoc searchAfter() {
572569
return searchAfter;

‎server/src/main/java/org/elasticsearch/search/SearchService.java‎

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -339,10 +339,9 @@ public void executeDfsPhase(ShardSearchRequest request, SearchShardTask task, Ac
339339
}
340340

341341
private DfsSearchResult executeDfsPhase(SearchRewriteContext rewriteContext, SearchShardTask task) throws IOException {
342-
final SearchContext context = createAndPutContext(rewriteContext);
342+
final SearchContext context = createAndPutContext(rewriteContext, task);
343343
context.incRef();
344344
try {
345-
context.setTask(task);
346345
contextProcessing(context);
347346
dfsPhase.execute(context);
348347
contextProcessedSuccessfully(context);
@@ -422,11 +421,10 @@ private <T> void runAsync(SearchContextId contextId, Supplier<T> executable, Act
422421
}
423422

424423
private SearchPhaseResult executeQueryPhase(SearchRewriteContext rewriteContext, SearchShardTask task) throws Exception {
425-
final SearchContext context = createAndPutContext(rewriteContext);
424+
final SearchContext context = createAndPutContext(rewriteContext, task);
426425
final ShardSearchRequest request = rewriteContext.request;
427426
context.incRef();
428427
try {
429-
context.setTask(task);
430428
final long afterQueryTime;
431429
try (SearchOperationListenerExecutor executor = new SearchOperationListenerExecutor(context)) {
432430
contextProcessing(context);
@@ -626,8 +624,8 @@ private SearchContext findContext(SearchContextId contextId, TransportRequest re
626624
}
627625
}
628626

629-
final SearchContext createAndPutContext(SearchRewriteContext rewriteContext) throws IOException {
630-
SearchContext context = createContext(rewriteContext);
627+
final SearchContext createAndPutContext(SearchRewriteContext rewriteContext, SearchShardTask task) throws IOException {
628+
SearchContext context = createContext(rewriteContext, task);
631629
onNewContext(context);
632630
boolean success = false;
633631
try {
@@ -660,7 +658,7 @@ private void onNewContext(SearchContext context) {
660658
}
661659
}
662660

663-
final SearchContext createContext(SearchRewriteContext rewriteContext) throws IOException {
661+
final SearchContext createContext(SearchRewriteContext rewriteContext, SearchShardTask searchTask) throws IOException {
664662
final DefaultSearchContext context = createSearchContext(rewriteContext, defaultSearchTimeout);
665663
try {
666664
final ShardSearchRequest request = rewriteContext.request;
@@ -684,6 +682,7 @@ final SearchContext createContext(SearchRewriteContext rewriteContext) throws IO
684682
if (context.size() == -1) {
685683
context.size(DEFAULT_SIZE);
686684
}
685+
context.setTask(searchTask);
687686

688687
// pre process
689688
dfsPhase.preProcess(context);
@@ -696,7 +695,6 @@ final SearchContext createContext(SearchRewriteContext rewriteContext) throws IO
696695
keepAlive = request.scroll().keepAlive().millis();
697696
}
698697
contextScrollKeepAlive(context, keepAlive);
699-
context.lowLevelCancellation(lowLevelCancellation);
700698
} catch (Exception e) {
701699
context.close();
702700
throw e;
@@ -731,7 +729,7 @@ private DefaultSearchContext createSearchContext(SearchRewriteContext rewriteCon
731729
DefaultSearchContext searchContext = new DefaultSearchContext(
732730
new SearchContextId(readerId, idGenerator.incrementAndGet()),
733731
request, shardTarget, searcher, clusterService, indexService, indexShard, bigArrays,
734-
threadPool::relativeTimeInMillis, timeout, fetchPhase);
732+
threadPool::relativeTimeInMillis, timeout, fetchPhase, lowLevelCancellation);
735733
success = true;
736734
return searchContext;
737735
} finally {

‎server/src/main/java/org/elasticsearch/search/internal/ContextIndexSearcher.java‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -82,23 +82,20 @@ public class ContextIndexSearcher extends IndexSearcher {
8282
private MutableQueryTimeout cancellable;
8383

8484
public ContextIndexSearcher(IndexReader reader, Similarity similarity,
85-
QueryCache queryCache, QueryCachingPolicy queryCachingPolicy) throws IOException {
86-
this(reader, similarity, queryCache, queryCachingPolicy, new MutableQueryTimeout());
85+
QueryCache queryCache, QueryCachingPolicy queryCachingPolicy,
86+
boolean wrapWithExitableDirectoryReader) throws IOException {
87+
this(reader, similarity, queryCache, queryCachingPolicy, new MutableQueryTimeout(), wrapWithExitableDirectoryReader);
8788
}
8889

89-
// TODO: Make the 2nd constructor private so that the IndexReader is always wrapped.
90-
// Some issues must be fixed:
91-
// - regarding tests deriving from AggregatorTestCase and more specifically the use of searchAndReduce and
92-
// the ShardSearcher sub-searchers.
93-
// - tests that use a MultiReader
94-
public ContextIndexSearcher(IndexReader reader, Similarity similarity,
95-
QueryCache queryCache, QueryCachingPolicy queryCachingPolicy,
96-
MutableQueryTimeout cancellable) throws IOException {
97-
super(cancellable != null ? new ExitableDirectoryReader((DirectoryReader) reader, cancellable) : reader);
90+
private ContextIndexSearcher(IndexReader reader, Similarity similarity,
91+
QueryCache queryCache, QueryCachingPolicy queryCachingPolicy,
92+
MutableQueryTimeout cancellable,
93+
boolean wrapWithExitableDirectoryReader) throws IOException {
94+
super(wrapWithExitableDirectoryReader ? new ExitableDirectoryReader((DirectoryReader) reader, cancellable) : reader);
9895
setSimilarity(similarity);
9996
setQueryCache(queryCache);
10097
setQueryCachingPolicy(queryCachingPolicy);
101-
this.cancellable = cancellable != null ? cancellable : new MutableQueryTimeout();
98+
this.cancellable = cancellable;
10299
}
103100

104101
public void setProfiler(QueryProfiler profiler) {

‎server/src/main/java/org/elasticsearch/search/internal/ExitableDirectoryReader.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -245,7 +245,7 @@ public int getDocCount() {
245245

246246
private static class ExitableIntersectVisitor implements PointValues.IntersectVisitor {
247247

248-
private static final int MAX_CALLS_BEFORE_QUERY_TIMEOUT_CHECK = (1 << 10) - 1; // 1023
248+
private static final int MAX_CALLS_BEFORE_QUERY_TIMEOUT_CHECK = (1 << 12) - 1; // 4095
249249

250250
private final PointValues.IntersectVisitor in;
251251
private final QueryCancellation queryCancellation;
@@ -276,7 +276,7 @@ public void visit(int docID, byte[] packedValue) throws IOException {
276276

277277
@Override
278278
public PointValues.Relation compare(byte[] minPackedValue, byte[] maxPackedValue) {
279-
queryCancellation.checkCancelled();
279+
checkAndThrowWithSampling();
280280
return in.compare(minPackedValue, maxPackedValue);
281281
}
282282

‎server/src/main/java/org/elasticsearch/search/query/QueryPhase.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -112,9 +112,9 @@ public QueryPhase() {
112112
public void preProcess(SearchContext context) {
113113
final Runnable cancellation;
114114
if (context.lowLevelCancellation()) {
115-
SearchShardTask task = context.getTask();
116115
cancellation = context.searcher().addQueryCancellation(() -> {
117-
if (task.isCancelled()) {
116+
SearchShardTask task = context.getTask();
117+
if (task != null && task.isCancelled()) {
118118
throw new TaskCancelledException("cancelled");
119119
}
120120
});
@@ -282,9 +282,9 @@ static boolean executeInternal(SearchContext searchContext) throws QueryPhaseExe
282282
}
283283

284284
if (searchContext.lowLevelCancellation()) {
285-
SearchShardTask task = searchContext.getTask();
286285
searcher.addQueryCancellation(() -> {
287-
if (task.isCancelled()) {
286+
SearchShardTask task = searchContext.getTask();
287+
if (task != null && task.isCancelled()) {
288288
throw new TaskCancelledException("cancelled");
289289
}
290290
});

‎server/src/test/java/org/elasticsearch/search/DefaultSearchContextTests.java‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ public void testPreProcess() throws Exception {
121121
SearchShardTarget target = new SearchShardTarget("node", shardId, null, OriginalIndices.NONE);
122122

123123
DefaultSearchContext context1 = new DefaultSearchContext(new SearchContextId(UUIDs.randomBase64UUID(), 1L),
124-
shardSearchRequest, target, searcher, null, indexService, indexShard, bigArrays, null, timeout, null);
124+
shardSearchRequest, target, searcher, null, indexService, indexShard, bigArrays, null, timeout, null, false);
125125
context1.from(300);
126126

127127
// resultWindow greater than maxResultWindow and scrollContext is null
@@ -162,7 +162,7 @@ public void testPreProcess() throws Exception {
162162

163163
// rescore is null but sliceBuilder is not null
164164
DefaultSearchContext context2 = new DefaultSearchContext(new SearchContextId(UUIDs.randomBase64UUID(), 2L),
165-
shardSearchRequest, target, searcher, null, indexService, indexShard, bigArrays, null, timeout, null);
165+
shardSearchRequest, target, searcher, null, indexService, indexShard, bigArrays, null, timeout, null, false);
166166

167167
SliceBuilder sliceBuilder = mock(SliceBuilder.class);
168168
int numSlices = maxSlicesPerScroll + randomIntBetween(1, 100);
@@ -179,7 +179,7 @@ public void testPreProcess() throws Exception {
179179
when(shardSearchRequest.indexBoost()).thenReturn(AbstractQueryBuilder.DEFAULT_BOOST);
180180

181181
DefaultSearchContext context3 = new DefaultSearchContext(new SearchContextId(UUIDs.randomBase64UUID(), 3L),
182-
shardSearchRequest, target, searcher, null, indexService, indexShard, bigArrays, null, timeout, null);
182+
shardSearchRequest, target, searcher, null, indexService, indexShard, bigArrays, null, timeout, null, false);
183183
ParsedQuery parsedQuery = ParsedQuery.parsedMatchAllQuery();
184184
context3.sliceBuilder(null).parsedQuery(parsedQuery).preProcess(false);
185185
assertEquals(context3.query(), context3.buildFilteredQuery(parsedQuery.query()));
@@ -189,7 +189,7 @@ public void testPreProcess() throws Exception {
189189
when(shardSearchRequest.indexRoutings()).thenReturn(new String[0]);
190190

191191
DefaultSearchContext context4 = new DefaultSearchContext(new SearchContextId(UUIDs.randomBase64UUID(), 4L),
192-
shardSearchRequest, target, searcher, null, indexService, indexShard, bigArrays, null, timeout, null);
192+
shardSearchRequest, target, searcher, null, indexService, indexShard, bigArrays, null, timeout, null, false);
193193
context4.sliceBuilder(new SliceBuilder(1,2)).parsedQuery(parsedQuery).preProcess(false);
194194
Query query1 = context4.query();
195195
context4.sliceBuilder(new SliceBuilder(0,2)).parsedQuery(parsedQuery).preProcess(false);

‎server/src/test/java/org/elasticsearch/search/SearchCancellationTests.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,8 @@ public static void cleanup() throws IOException {
8686
}
8787

8888
public void testAddingCancellationActions() throws IOException {
89-
ContextIndexSearcher searcher = new ContextIndexSearcher(reader,
90-
IndexSearcher.getDefaultSimilarity(), IndexSearcher.getDefaultQueryCache(), IndexSearcher.getDefaultQueryCachingPolicy());
89+
ContextIndexSearcher searcher = new ContextIndexSearcher(reader, IndexSearcher.getDefaultSimilarity(),
90+
IndexSearcher.getDefaultQueryCache(), IndexSearcher.getDefaultQueryCachingPolicy(), true);
9191
NullPointerException npe = expectThrows(NullPointerException.class, () -> searcher.addQueryCancellation(null));
9292
assertEquals("cancellation runnable should not be null", npe.getMessage());
9393

@@ -100,8 +100,8 @@ public void testAddingCancellationActions() throws IOException {
100100
public void testCancellableCollector() throws IOException {
101101
TotalHitCountCollector collector1 = new TotalHitCountCollector();
102102
Runnable cancellation = () -> { throw new TaskCancelledException("cancelled"); };
103-
ContextIndexSearcher searcher = new ContextIndexSearcher(reader,
104-
IndexSearcher.getDefaultSimilarity(), IndexSearcher.getDefaultQueryCache(), IndexSearcher.getDefaultQueryCachingPolicy());
103+
ContextIndexSearcher searcher = new ContextIndexSearcher(reader, IndexSearcher.getDefaultSimilarity(),
104+
IndexSearcher.getDefaultQueryCache(), IndexSearcher.getDefaultQueryCachingPolicy(), true);
105105

106106
searcher.search(new MatchAllDocsQuery(), collector1);
107107
assertThat(collector1.getTotalHits(), equalTo(reader.numDocs()));
@@ -116,14 +116,14 @@ public void testCancellableCollector() throws IOException {
116116
assertThat(collector2.getTotalHits(), equalTo(reader.numDocs()));
117117
}
118118

119-
public void testCancellableDirectoryReader() throws IOException {
119+
public void testExitableDirectoryReader() throws IOException {
120120
AtomicBoolean cancelled = new AtomicBoolean(true);
121121
Runnable cancellation = () -> {
122122
if (cancelled.get()) {
123123
throw new TaskCancelledException("cancelled");
124124
}};
125-
ContextIndexSearcher searcher = new ContextIndexSearcher(reader,
126-
IndexSearcher.getDefaultSimilarity(), IndexSearcher.getDefaultQueryCache(), IndexSearcher.getDefaultQueryCachingPolicy());
125+
ContextIndexSearcher searcher = new ContextIndexSearcher(reader, IndexSearcher.getDefaultSimilarity(),
126+
IndexSearcher.getDefaultQueryCache(), IndexSearcher.getDefaultQueryCachingPolicy(), true);
127127
searcher.addQueryCancellation(cancellation);
128128
CompiledAutomaton automaton = new CompiledAutomaton(new RegExp("a.*").toAutomaton());
129129

0 commit comments

Comments
 (0)