Skip to content

Commit cdc377e

Browse files
matrivmatriv
authored
Reduce performance impact of ExitableDirectoryReader (#53978)
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
1 parent 2a99f4e commit cdc377e

File tree

13 files changed

+60
-66
lines changed

13 files changed

+60
-66
lines changed

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

+4-7
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

+7-9
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

+9-12
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

+1-1
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 << 13) - 1; // 8191
249249

250250
private final PointValues.IntersectVisitor in;
251251
private final QueryCancellation queryCancellation;

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

+4-4
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

+4-4
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

+7-7
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)