Skip to content

Commit 595ba6b

Browse files
original-brownbearoriginal-brownbear
authored
Remove some redundant ref-counting from SearchHits (#124948)
Remove ref-counting that is obviously redundant because of clear ownership transfers from `SearchHits`.
1 parent 5f871c5 commit 595ba6b

File tree

7 files changed

+118
-73
lines changed

7 files changed

+118
-73
lines changed

‎server/src/main/java/org/elasticsearch/action/search/SearchPhaseController.java‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -277,26 +277,28 @@ public static SearchResponseSections merge(
277277
if (reducedQueryPhase.isEmptyResult) {
278278
return SearchResponseSections.EMPTY_WITH_TOTAL_HITS;
279279
}
280-
ScoreDoc[] sortedDocs = reducedQueryPhase.sortedTopDocs.scoreDocs;
281280
var fetchResults = fetchResultsArray.asList();
282-
final SearchHits hits = getHits(reducedQueryPhase, ignoreFrom, fetchResultsArray);
281+
SearchHits hits = getHits(reducedQueryPhase, ignoreFrom, fetchResultsArray);
283282
try {
284283
if (reducedQueryPhase.suggest != null && fetchResults.isEmpty() == false) {
285-
mergeSuggest(reducedQueryPhase, fetchResultsArray, hits, sortedDocs);
284+
mergeSuggest(reducedQueryPhase, fetchResultsArray, hits.getHits().length, reducedQueryPhase.sortedTopDocs.scoreDocs);
286285
}
287-
return reducedQueryPhase.buildResponse(hits, fetchResults);
286+
var res = reducedQueryPhase.buildResponse(hits, fetchResults);
287+
hits = null;
288+
return res;
288289
} finally {
289-
hits.decRef();
290+
if (hits != null) {
291+
hits.decRef();
292+
}
290293
}
291294
}
292295

293296
private static void mergeSuggest(
294297
ReducedQueryPhase reducedQueryPhase,
295298
AtomicArray<? extends SearchPhaseResult> fetchResultsArray,
296-
SearchHits hits,
299+
int currentOffset,
297300
ScoreDoc[] sortedDocs
298301
) {
299-
int currentOffset = hits.getHits().length;
300302
for (CompletionSuggestion suggestion : reducedQueryPhase.suggest.filter(CompletionSuggestion.class)) {
301303
final List<CompletionSuggestion.Entry.Option> suggestionOptions = suggestion.getOptions();
302304
for (int scoreDocIndex = currentOffset; scoreDocIndex < currentOffset + suggestionOptions.size(); scoreDocIndex++) {

‎server/src/main/java/org/elasticsearch/action/search/SearchResponseSections.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,6 @@ public SearchResponseSections(
6161
int numReducePhases
6262
) {
6363
this.hits = hits;
64-
hits.incRef();
6564
this.aggregations = aggregations;
6665
this.suggest = suggest;
6766
this.profileResults = profileResults;

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,7 @@ public boolean decRef() {
252252
}
253253

254254
private void deallocate() {
255+
var hits = this.hits;
255256
for (int i = 0; i < hits.length; i++) {
256257
assert hits[i] != null;
257258
hits[i].decRef();

‎server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -84,12 +84,18 @@ public void execute(SearchContext context, int[] docIdsToLoad, RankDocShardInfo
8484
try {
8585
hits = buildSearchHits(context, docIdsToLoad, profiler, rankDocs);
8686
} finally {
87-
// Always finish profiling
88-
ProfileResult profileResult = profiler.finish();
89-
// Only set the shardResults if building search hits was successful
90-
if (hits != null) {
91-
context.fetchResult().shardResult(hits, profileResult);
92-
hits.decRef();
87+
try {
88+
// Always finish profiling
89+
ProfileResult profileResult = profiler.finish();
90+
// Only set the shardResults if building search hits was successful
91+
if (hits != null) {
92+
context.fetchResult().shardResult(hits, profileResult);
93+
hits = null;
94+
}
95+
} finally {
96+
if (hits != null) {
97+
hits.decRef();
98+
}
9399
}
94100
}
95101
}

‎server/src/main/java/org/elasticsearch/search/fetch/FetchSearchResult.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,6 @@ public void shardResult(SearchHits hits, ProfileResult profileResult) {
6767
existing.decRef();
6868
}
6969
this.hits = hits;
70-
hits.mustIncRef();
7170
assert this.profileResult == null;
7271
this.profileResult = profileResult;
7372
}

‎server/src/test/java/org/elasticsearch/action/search/ExpandSearchPhaseTests.java‎

Lines changed: 74 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -117,31 +117,34 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL
117117

118118
SearchHit hit = new SearchHit(1, "ID");
119119
hit.setDocumentField("someField", new DocumentField("someField", Collections.singletonList(collapseValue)));
120-
SearchHits hits = new SearchHits(new SearchHit[] { hit }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F);
121-
try {
122-
ExpandSearchPhase phase = newExpandSearchPhase(
123-
mockSearchPhaseContext,
124-
new SearchResponseSections(hits, null, null, false, null, null, 1),
125-
null
126-
);
127-
128-
phase.run();
129-
mockSearchPhaseContext.assertNoFailure();
130-
SearchResponse theResponse = mockSearchPhaseContext.searchResponse.get();
131-
assertNotNull(theResponse);
132-
assertEquals(numInnerHits, theResponse.getHits().getHits()[0].getInnerHits().size());
120+
ExpandSearchPhase phase = newExpandSearchPhase(
121+
mockSearchPhaseContext,
122+
new SearchResponseSections(
123+
new SearchHits(new SearchHit[] { hit }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F),
124+
null,
125+
null,
126+
false,
127+
null,
128+
null,
129+
1
130+
),
131+
null
132+
);
133133

134-
for (int innerHitNum = 0; innerHitNum < numInnerHits; innerHitNum++) {
135-
assertSame(
136-
theResponse.getHits().getHits()[0].getInnerHits().get("innerHit" + innerHitNum),
137-
collapsedHits.get(innerHitNum)
138-
);
139-
}
134+
phase.run();
135+
mockSearchPhaseContext.assertNoFailure();
136+
SearchResponse theResponse = mockSearchPhaseContext.searchResponse.get();
137+
assertNotNull(theResponse);
138+
assertEquals(numInnerHits, theResponse.getHits().getHits()[0].getInnerHits().size());
140139

141-
assertTrue(executedMultiSearch.get());
142-
} finally {
143-
hits.decRef();
140+
for (int innerHitNum = 0; innerHitNum < numInnerHits; innerHitNum++) {
141+
assertSame(
142+
theResponse.getHits().getHits()[0].getInnerHits().get("innerHit" + innerHitNum),
143+
collapsedHits.get(innerHitNum)
144+
);
144145
}
146+
147+
assertTrue(executedMultiSearch.get());
145148
} finally {
146149
var resp = mockSearchPhaseContext.searchResponse.get();
147150
if (resp != null) {
@@ -188,8 +191,17 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL
188191
hit1.setDocumentField("someField", new DocumentField("someField", Collections.singletonList(collapseValue)));
189192
SearchHit hit2 = new SearchHit(2, "ID2");
190193
hit2.setDocumentField("someField", new DocumentField("someField", Collections.singletonList(collapseValue)));
191-
SearchHits hits = new SearchHits(new SearchHit[] { hit1, hit2 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F);
192-
try (SearchResponseSections searchResponseSections = new SearchResponseSections(hits, null, null, false, null, null, 1)) {
194+
try (
195+
SearchResponseSections searchResponseSections = new SearchResponseSections(
196+
new SearchHits(new SearchHit[] { hit1, hit2 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F),
197+
null,
198+
null,
199+
false,
200+
null,
201+
null,
202+
1
203+
)
204+
) {
193205
ExpandSearchPhase phase = newExpandSearchPhase(mockSearchPhaseContext, searchResponseSections, null);
194206
phase.run();
195207
assertThat(mockSearchPhaseContext.phaseFailure.get(), Matchers.instanceOf(RuntimeException.class));
@@ -198,7 +210,6 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL
198210
assertNull(mockSearchPhaseContext.searchResponse.get());
199211
} finally {
200212
mockSearchPhaseContext.results.close();
201-
hits.decRef();
202213
collapsedHits.decRef();
203214
}
204215
}
@@ -217,19 +228,22 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL
217228
hit1.setDocumentField("someField", new DocumentField("someField", Collections.singletonList(null)));
218229
SearchHit hit2 = new SearchHit(2, "ID2");
219230
hit2.setDocumentField("someField", new DocumentField("someField", Collections.singletonList(null)));
220-
SearchHits hits = new SearchHits(new SearchHit[] { hit1, hit2 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F);
221-
try {
222-
ExpandSearchPhase phase = newExpandSearchPhase(
223-
mockSearchPhaseContext,
224-
new SearchResponseSections(hits, null, null, false, null, null, 1),
225-
null
226-
);
227-
phase.run();
228-
mockSearchPhaseContext.assertNoFailure();
229-
assertNotNull(mockSearchPhaseContext.searchResponse.get());
230-
} finally {
231-
hits.decRef();
232-
}
231+
ExpandSearchPhase phase = newExpandSearchPhase(
232+
mockSearchPhaseContext,
233+
new SearchResponseSections(
234+
new SearchHits(new SearchHit[] { hit1, hit2 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F),
235+
null,
236+
null,
237+
false,
238+
null,
239+
null,
240+
1
241+
),
242+
null
243+
);
244+
phase.run();
245+
mockSearchPhaseContext.assertNoFailure();
246+
assertNotNull(mockSearchPhaseContext.searchResponse.get());
233247
} finally {
234248
mockSearchPhaseContext.results.close();
235249
var resp = mockSearchPhaseContext.searchResponse.get();
@@ -300,13 +314,20 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL
300314

301315
SearchHit hit = new SearchHit(1, "ID");
302316
hit.setDocumentField("someField", new DocumentField("someField", Collections.singletonList("foo")));
303-
SearchHits hits = new SearchHits(new SearchHit[] { hit }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F);
304-
try (SearchResponseSections searchResponseSections = new SearchResponseSections(hits, null, null, false, null, null, 1)) {
317+
try (
318+
SearchResponseSections searchResponseSections = new SearchResponseSections(
319+
new SearchHits(new SearchHit[] { hit }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F),
320+
null,
321+
null,
322+
false,
323+
null,
324+
null,
325+
1
326+
)
327+
) {
305328
ExpandSearchPhase phase = newExpandSearchPhase(mockSearchPhaseContext, searchResponseSections, null);
306329
phase.run();
307330
mockSearchPhaseContext.assertNoFailure();
308-
} finally {
309-
hits.decRef();
310331
}
311332
} finally {
312333
mockSearchPhaseContext.results.close();
@@ -364,13 +385,20 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL
364385

365386
SearchHit hit = new SearchHit(1, "ID");
366387
hit.setDocumentField("someField", new DocumentField("someField", Collections.singletonList("foo")));
367-
SearchHits hits = new SearchHits(new SearchHit[] { hit }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F);
368-
try (SearchResponseSections searchResponseSections = new SearchResponseSections(hits, null, null, false, null, null, 1)) {
388+
try (
389+
SearchResponseSections searchResponseSections = new SearchResponseSections(
390+
new SearchHits(new SearchHit[] { hit }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F),
391+
null,
392+
null,
393+
false,
394+
null,
395+
null,
396+
1
397+
)
398+
) {
369399
ExpandSearchPhase phase = newExpandSearchPhase(mockSearchPhaseContext, searchResponseSections, new AtomicArray<>(0));
370400
phase.run();
371401
mockSearchPhaseContext.assertNoFailure();
372-
} finally {
373-
hits.decRef();
374402
}
375403
} finally {
376404
mockSearchPhaseContext.results.close();

‎server/src/test/java/org/elasticsearch/action/search/FetchLookupFieldsPhaseTests.java‎

Lines changed: 22 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -46,12 +46,19 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL
4646
for (int i = 0; i < searchHits.length; i++) {
4747
searchHits[i] = SearchHitTests.createTestItem(randomBoolean(), randomBoolean());
4848
}
49-
SearchHits hits = new SearchHits(searchHits, new TotalHits(numHits, TotalHits.Relation.EQUAL_TO), 1.0f);
50-
try (var sections = new SearchResponseSections(hits, null, null, false, null, null, 1)) {
49+
try (
50+
var sections = new SearchResponseSections(
51+
new SearchHits(searchHits, new TotalHits(numHits, TotalHits.Relation.EQUAL_TO), 1.0f),
52+
null,
53+
null,
54+
false,
55+
null,
56+
null,
57+
1
58+
)
59+
) {
5160
FetchLookupFieldsPhase phase = new FetchLookupFieldsPhase(searchPhaseContext, sections, null);
5261
phase.run();
53-
} finally {
54-
hits.decRef();
5562
}
5663
searchPhaseContext.assertNoFailure();
5764
assertNotNull(searchPhaseContext.searchResponse.get());
@@ -182,16 +189,19 @@ void sendExecuteMultiSearch(
182189
)
183190
);
184191
}
185-
SearchHits searchHits = new SearchHits(
186-
new SearchHit[] { leftHit0, leftHit1 },
187-
new TotalHits(2, TotalHits.Relation.EQUAL_TO),
188-
1.0f
189-
);
190-
try (var sections = new SearchResponseSections(searchHits, null, null, false, null, null, 1)) {
192+
try (
193+
var sections = new SearchResponseSections(
194+
new SearchHits(new SearchHit[] { leftHit0, leftHit1 }, new TotalHits(2, TotalHits.Relation.EQUAL_TO), 1.0f),
195+
null,
196+
null,
197+
false,
198+
null,
199+
null,
200+
1
201+
)
202+
) {
191203
FetchLookupFieldsPhase phase = new FetchLookupFieldsPhase(searchPhaseContext, sections, null);
192204
phase.run();
193-
} finally {
194-
searchHits.decRef();
195205
}
196206
assertTrue(requestSent.get());
197207
searchPhaseContext.assertNoFailure();

0 commit comments

Comments
 (0)