Skip to content

Commit 4a8b1c4

Browse files
original-brownbearoriginal-brownbear
authored
Remove some redundant ref-counting from SearchHits (elastic#124948) (elastic#126517)
Remove ref-counting that is obviously redundant because of clear ownership transfers from `SearchHits`.
1 parent 19a6883 commit 4a8b1c4

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
@@ -273,26 +273,28 @@ public static SearchResponseSections merge(
273273
if (reducedQueryPhase.isEmptyResult) {
274274
return SearchResponseSections.EMPTY_WITH_TOTAL_HITS;
275275
}
276-
ScoreDoc[] sortedDocs = reducedQueryPhase.sortedTopDocs.scoreDocs;
277276
var fetchResults = fetchResultsArray.asList();
278-
final SearchHits hits = getHits(reducedQueryPhase, ignoreFrom, fetchResultsArray);
277+
SearchHits hits = getHits(reducedQueryPhase, ignoreFrom, fetchResultsArray);
279278
try {
280279
if (reducedQueryPhase.suggest != null && fetchResults.isEmpty() == false) {
281-
mergeSuggest(reducedQueryPhase, fetchResultsArray, hits, sortedDocs);
280+
mergeSuggest(reducedQueryPhase, fetchResultsArray, hits.getHits().length, reducedQueryPhase.sortedTopDocs.scoreDocs);
282281
}
283-
return reducedQueryPhase.buildResponse(hits, fetchResults);
282+
var res = reducedQueryPhase.buildResponse(hits, fetchResults);
283+
hits = null;
284+
return res;
284285
} finally {
285-
hits.decRef();
286+
if (hits != null) {
287+
hits.decRef();
288+
}
286289
}
287290
}
288291

289292
private static void mergeSuggest(
290293
ReducedQueryPhase reducedQueryPhase,
291294
AtomicArray<? extends SearchPhaseResult> fetchResultsArray,
292-
SearchHits hits,
295+
int currentOffset,
293296
ScoreDoc[] sortedDocs
294297
) {
295-
int currentOffset = hits.getHits().length;
296298
for (CompletionSuggestion suggestion : reducedQueryPhase.suggest.filter(CompletionSuggestion.class)) {
297299
final List<CompletionSuggestion.Entry.Option> suggestionOptions = suggestion.getOptions();
298300
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
@@ -251,6 +251,7 @@ public boolean decRef() {
251251
}
252252

253253
private void deallocate() {
254+
var hits = this.hits;
254255
for (int i = 0; i < hits.length; i++) {
255256
assert hits[i] != null;
256257
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
@@ -83,12 +83,18 @@ public void execute(SearchContext context, int[] docIdsToLoad, RankDocShardInfo
8383
try {
8484
hits = buildSearchHits(context, docIdsToLoad, profiler, rankDocs);
8585
} finally {
86-
// Always finish profiling
87-
ProfileResult profileResult = profiler.finish();
88-
// Only set the shardResults if building search hits was successful
89-
if (hits != null) {
90-
context.fetchResult().shardResult(hits, profileResult);
91-
hits.decRef();
86+
try {
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 = null;
93+
}
94+
} finally {
95+
if (hits != null) {
96+
hits.decRef();
97+
}
9298
}
9399
}
94100
}

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

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

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

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

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

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

140-
assertTrue(executedMultiSearch.get());
141-
} finally {
142-
hits.decRef();
139+
for (int innerHitNum = 0; innerHitNum < numInnerHits; innerHitNum++) {
140+
assertSame(
141+
theResponse.getHits().getHits()[0].getInnerHits().get("innerHit" + innerHitNum),
142+
collapsedHits.get(innerHitNum)
143+
);
143144
}
145+
146+
assertTrue(executedMultiSearch.get());
144147
} finally {
145148
var resp = mockSearchPhaseContext.searchResponse.get();
146149
if (resp != null) {
@@ -202,8 +205,17 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL
202205
hit1.setDocumentField("someField", new DocumentField("someField", Collections.singletonList(collapseValue)));
203206
SearchHit hit2 = new SearchHit(2, "ID2");
204207
hit2.setDocumentField("someField", new DocumentField("someField", Collections.singletonList(collapseValue)));
205-
SearchHits hits = new SearchHits(new SearchHit[] { hit1, hit2 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F);
206-
try (SearchResponseSections searchResponseSections = new SearchResponseSections(hits, null, null, false, null, null, 1)) {
208+
try (
209+
SearchResponseSections searchResponseSections = new SearchResponseSections(
210+
new SearchHits(new SearchHit[] { hit1, hit2 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F),
211+
null,
212+
null,
213+
false,
214+
null,
215+
null,
216+
1
217+
)
218+
) {
207219
ExpandSearchPhase phase = newExpandSearchPhase(mockSearchPhaseContext, searchResponseSections, null);
208220
phase.run();
209221
assertThat(mockSearchPhaseContext.phaseFailure.get(), Matchers.instanceOf(RuntimeException.class));
@@ -212,7 +224,6 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL
212224
assertNull(mockSearchPhaseContext.searchResponse.get());
213225
} finally {
214226
mockSearchPhaseContext.results.close();
215-
hits.decRef();
216227
collapsedHits.decRef();
217228
}
218229
}
@@ -231,19 +242,22 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL
231242
hit1.setDocumentField("someField", new DocumentField("someField", Collections.singletonList(null)));
232243
SearchHit hit2 = new SearchHit(2, "ID2");
233244
hit2.setDocumentField("someField", new DocumentField("someField", Collections.singletonList(null)));
234-
SearchHits hits = new SearchHits(new SearchHit[] { hit1, hit2 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F);
235-
try {
236-
ExpandSearchPhase phase = newExpandSearchPhase(
237-
mockSearchPhaseContext,
238-
new SearchResponseSections(hits, null, null, false, null, null, 1),
239-
null
240-
);
241-
phase.run();
242-
mockSearchPhaseContext.assertNoFailure();
243-
assertNotNull(mockSearchPhaseContext.searchResponse.get());
244-
} finally {
245-
hits.decRef();
246-
}
245+
ExpandSearchPhase phase = newExpandSearchPhase(
246+
mockSearchPhaseContext,
247+
new SearchResponseSections(
248+
new SearchHits(new SearchHit[] { hit1, hit2 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F),
249+
null,
250+
null,
251+
false,
252+
null,
253+
null,
254+
1
255+
),
256+
null
257+
);
258+
phase.run();
259+
mockSearchPhaseContext.assertNoFailure();
260+
assertNotNull(mockSearchPhaseContext.searchResponse.get());
247261
} finally {
248262
mockSearchPhaseContext.results.close();
249263
var resp = mockSearchPhaseContext.searchResponse.get();
@@ -314,13 +328,20 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL
314328

315329
SearchHit hit = new SearchHit(1, "ID");
316330
hit.setDocumentField("someField", new DocumentField("someField", Collections.singletonList("foo")));
317-
SearchHits hits = new SearchHits(new SearchHit[] { hit }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F);
318-
try (SearchResponseSections searchResponseSections = new SearchResponseSections(hits, null, null, false, null, null, 1)) {
331+
try (
332+
SearchResponseSections searchResponseSections = new SearchResponseSections(
333+
new SearchHits(new SearchHit[] { hit }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F),
334+
null,
335+
null,
336+
false,
337+
null,
338+
null,
339+
1
340+
)
341+
) {
319342
ExpandSearchPhase phase = newExpandSearchPhase(mockSearchPhaseContext, searchResponseSections, null);
320343
phase.run();
321344
mockSearchPhaseContext.assertNoFailure();
322-
} finally {
323-
hits.decRef();
324345
}
325346
} finally {
326347
mockSearchPhaseContext.results.close();
@@ -378,13 +399,20 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL
378399

379400
SearchHit hit = new SearchHit(1, "ID");
380401
hit.setDocumentField("someField", new DocumentField("someField", Collections.singletonList("foo")));
381-
SearchHits hits = new SearchHits(new SearchHit[] { hit }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F);
382-
try (SearchResponseSections searchResponseSections = new SearchResponseSections(hits, null, null, false, null, null, 1)) {
402+
try (
403+
SearchResponseSections searchResponseSections = new SearchResponseSections(
404+
new SearchHits(new SearchHit[] { hit }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F),
405+
null,
406+
null,
407+
false,
408+
null,
409+
null,
410+
1
411+
)
412+
) {
383413
ExpandSearchPhase phase = newExpandSearchPhase(mockSearchPhaseContext, searchResponseSections, new AtomicArray<>(0));
384414
phase.run();
385415
mockSearchPhaseContext.assertNoFailure();
386-
} finally {
387-
hits.decRef();
388416
}
389417
} finally {
390418
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)