Skip to content

[8.x] Remove some redundant ref-counting from SearchHits (#124948) #126517

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 9, 2025
Merged

[8.x] Remove some redundant ref-counting from SearchHits (#124948) #126517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions server/src/main/java/org/elasticsearch/action/search/SearchPhaseController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,26 +273,28 @@ public static SearchResponseSections merge(
if (reducedQueryPhase.isEmptyResult) {
return SearchResponseSections.EMPTY_WITH_TOTAL_HITS;
}
ScoreDoc[] sortedDocs = reducedQueryPhase.sortedTopDocs.scoreDocs;
var fetchResults = fetchResultsArray.asList();
final SearchHits hits = getHits(reducedQueryPhase, ignoreFrom, fetchResultsArray);
SearchHits hits = getHits(reducedQueryPhase, ignoreFrom, fetchResultsArray);
try {
if (reducedQueryPhase.suggest != null && fetchResults.isEmpty() == false) {
mergeSuggest(reducedQueryPhase, fetchResultsArray, hits, sortedDocs);
mergeSuggest(reducedQueryPhase, fetchResultsArray, hits.getHits().length, reducedQueryPhase.sortedTopDocs.scoreDocs);
}
return reducedQueryPhase.buildResponse(hits, fetchResults);
var res = reducedQueryPhase.buildResponse(hits, fetchResults);
hits = null;
return res;
} finally {
hits.decRef();
if (hits != null) {
hits.decRef();
}
}
}

private static void mergeSuggest(
ReducedQueryPhase reducedQueryPhase,
AtomicArray<? extends SearchPhaseResult> fetchResultsArray,
SearchHits hits,
int currentOffset,
ScoreDoc[] sortedDocs
) {
int currentOffset = hits.getHits().length;
for (CompletionSuggestion suggestion : reducedQueryPhase.suggest.filter(CompletionSuggestion.class)) {
final List<CompletionSuggestion.Entry.Option> suggestionOptions = suggestion.getOptions();
for (int scoreDocIndex = currentOffset; scoreDocIndex < currentOffset + suggestionOptions.size(); scoreDocIndex++) {
Expand Down
1 change: 0 additions & 1 deletion server/src/main/java/org/elasticsearch/action/search/SearchResponseSections.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ public SearchResponseSections(
int numReducePhases
) {
this.hits = hits;
hits.incRef();
this.aggregations = aggregations;
this.suggest = suggest;
this.profileResults = profileResults;
Expand Down
1 change: 1 addition & 0 deletions server/src/main/java/org/elasticsearch/search/SearchHits.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ public boolean decRef() {
}

private void deallocate() {
var hits = this.hits;
for (int i = 0; i < hits.length; i++) {
assert hits[i] != null;
hits[i].decRef();
Expand Down
18 changes: 12 additions & 6 deletions server/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,18 @@ public void execute(SearchContext context, int[] docIdsToLoad, RankDocShardInfo
try {
hits = buildSearchHits(context, docIdsToLoad, profiler, rankDocs);
} finally {
// Always finish profiling
ProfileResult profileResult = profiler.finish();
// Only set the shardResults if building search hits was successful
if (hits != null) {
context.fetchResult().shardResult(hits, profileResult);
hits.decRef();
try {
// Always finish profiling
ProfileResult profileResult = profiler.finish();
// Only set the shardResults if building search hits was successful
if (hits != null) {
context.fetchResult().shardResult(hits, profileResult);
hits = null;
}
} finally {
if (hits != null) {
hits.decRef();
}
}
}
}
Expand Down
1 change: 0 additions & 1 deletion server/src/main/java/org/elasticsearch/search/fetch/FetchSearchResult.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,6 @@ public void shardResult(SearchHits hits, ProfileResult profileResult) {
existing.decRef();
}
this.hits = hits;
hits.mustIncRef();
assert this.profileResult == null;
this.profileResult = profileResult;
}
Expand Down
120 changes: 74 additions & 46 deletions server/src/test/java/org/elasticsearch/action/search/ExpandSearchPhaseTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,31 +116,34 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL

SearchHit hit = new SearchHit(1, "ID");
hit.setDocumentField("someField", new DocumentField("someField", Collections.singletonList(collapseValue)));
SearchHits hits = new SearchHits(new SearchHit[] { hit }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F);
try {
ExpandSearchPhase phase = newExpandSearchPhase(
mockSearchPhaseContext,
new SearchResponseSections(hits, null, null, false, null, null, 1),
null
);

phase.run();
mockSearchPhaseContext.assertNoFailure();
SearchResponse theResponse = mockSearchPhaseContext.searchResponse.get();
assertNotNull(theResponse);
assertEquals(numInnerHits, theResponse.getHits().getHits()[0].getInnerHits().size());
ExpandSearchPhase phase = newExpandSearchPhase(
mockSearchPhaseContext,
new SearchResponseSections(
new SearchHits(new SearchHit[] { hit }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F),
null,
null,
false,
null,
null,
1
),
null
);

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

assertTrue(executedMultiSearch.get());
} finally {
hits.decRef();
for (int innerHitNum = 0; innerHitNum < numInnerHits; innerHitNum++) {
assertSame(
theResponse.getHits().getHits()[0].getInnerHits().get("innerHit" + innerHitNum),
collapsedHits.get(innerHitNum)
);
}

assertTrue(executedMultiSearch.get());
} finally {
var resp = mockSearchPhaseContext.searchResponse.get();
if (resp != null) {
Expand Down Expand Up @@ -202,8 +205,17 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL
hit1.setDocumentField("someField", new DocumentField("someField", Collections.singletonList(collapseValue)));
SearchHit hit2 = new SearchHit(2, "ID2");
hit2.setDocumentField("someField", new DocumentField("someField", Collections.singletonList(collapseValue)));
SearchHits hits = new SearchHits(new SearchHit[] { hit1, hit2 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F);
try (SearchResponseSections searchResponseSections = new SearchResponseSections(hits, null, null, false, null, null, 1)) {
try (
SearchResponseSections searchResponseSections = new SearchResponseSections(
new SearchHits(new SearchHit[] { hit1, hit2 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F),
null,
null,
false,
null,
null,
1
)
) {
ExpandSearchPhase phase = newExpandSearchPhase(mockSearchPhaseContext, searchResponseSections, null);
phase.run();
assertThat(mockSearchPhaseContext.phaseFailure.get(), Matchers.instanceOf(RuntimeException.class));
Expand All @@ -212,7 +224,6 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL
assertNull(mockSearchPhaseContext.searchResponse.get());
} finally {
mockSearchPhaseContext.results.close();
hits.decRef();
collapsedHits.decRef();
}
}
Expand All @@ -231,19 +242,22 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL
hit1.setDocumentField("someField", new DocumentField("someField", Collections.singletonList(null)));
SearchHit hit2 = new SearchHit(2, "ID2");
hit2.setDocumentField("someField", new DocumentField("someField", Collections.singletonList(null)));
SearchHits hits = new SearchHits(new SearchHit[] { hit1, hit2 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F);
try {
ExpandSearchPhase phase = newExpandSearchPhase(
mockSearchPhaseContext,
new SearchResponseSections(hits, null, null, false, null, null, 1),
null
);
phase.run();
mockSearchPhaseContext.assertNoFailure();
assertNotNull(mockSearchPhaseContext.searchResponse.get());
} finally {
hits.decRef();
}
ExpandSearchPhase phase = newExpandSearchPhase(
mockSearchPhaseContext,
new SearchResponseSections(
new SearchHits(new SearchHit[] { hit1, hit2 }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F),
null,
null,
false,
null,
null,
1
),
null
);
phase.run();
mockSearchPhaseContext.assertNoFailure();
assertNotNull(mockSearchPhaseContext.searchResponse.get());
} finally {
mockSearchPhaseContext.results.close();
var resp = mockSearchPhaseContext.searchResponse.get();
Expand Down Expand Up @@ -314,13 +328,20 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL

SearchHit hit = new SearchHit(1, "ID");
hit.setDocumentField("someField", new DocumentField("someField", Collections.singletonList("foo")));
SearchHits hits = new SearchHits(new SearchHit[] { hit }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F);
try (SearchResponseSections searchResponseSections = new SearchResponseSections(hits, null, null, false, null, null, 1)) {
try (
SearchResponseSections searchResponseSections = new SearchResponseSections(
new SearchHits(new SearchHit[] { hit }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F),
null,
null,
false,
null,
null,
1
)
) {
ExpandSearchPhase phase = newExpandSearchPhase(mockSearchPhaseContext, searchResponseSections, null);
phase.run();
mockSearchPhaseContext.assertNoFailure();
} finally {
hits.decRef();
}
} finally {
mockSearchPhaseContext.results.close();
Expand Down Expand Up @@ -378,13 +399,20 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL

SearchHit hit = new SearchHit(1, "ID");
hit.setDocumentField("someField", new DocumentField("someField", Collections.singletonList("foo")));
SearchHits hits = new SearchHits(new SearchHit[] { hit }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F);
try (SearchResponseSections searchResponseSections = new SearchResponseSections(hits, null, null, false, null, null, 1)) {
try (
SearchResponseSections searchResponseSections = new SearchResponseSections(
new SearchHits(new SearchHit[] { hit }, new TotalHits(1, TotalHits.Relation.EQUAL_TO), 1.0F),
null,
null,
false,
null,
null,
1
)
) {
ExpandSearchPhase phase = newExpandSearchPhase(mockSearchPhaseContext, searchResponseSections, new AtomicArray<>(0));
phase.run();
mockSearchPhaseContext.assertNoFailure();
} finally {
hits.decRef();
}
} finally {
mockSearchPhaseContext.results.close();
Expand Down
34 changes: 22 additions & 12 deletions server/src/test/java/org/elasticsearch/action/search/FetchLookupFieldsPhaseTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,19 @@ void sendExecuteMultiSearch(MultiSearchRequest request, SearchTask task, ActionL
for (int i = 0; i < searchHits.length; i++) {
searchHits[i] = SearchHitTests.createTestItem(randomBoolean(), randomBoolean());
}
SearchHits hits = new SearchHits(searchHits, new TotalHits(numHits, TotalHits.Relation.EQUAL_TO), 1.0f);
try (var sections = new SearchResponseSections(hits, null, null, false, null, null, 1)) {
try (
var sections = new SearchResponseSections(
new SearchHits(searchHits, new TotalHits(numHits, TotalHits.Relation.EQUAL_TO), 1.0f),
null,
null,
false,
null,
null,
1
)
) {
FetchLookupFieldsPhase phase = new FetchLookupFieldsPhase(searchPhaseContext, sections, null);
phase.run();
} finally {
hits.decRef();
}
searchPhaseContext.assertNoFailure();
assertNotNull(searchPhaseContext.searchResponse.get());
Expand Down Expand Up @@ -182,16 +189,19 @@ void sendExecuteMultiSearch(
)
);
}
SearchHits searchHits = new SearchHits(
new SearchHit[] { leftHit0, leftHit1 },
new TotalHits(2, TotalHits.Relation.EQUAL_TO),
1.0f
);
try (var sections = new SearchResponseSections(searchHits, null, null, false, null, null, 1)) {
try (
var sections = new SearchResponseSections(
new SearchHits(new SearchHit[] { leftHit0, leftHit1 }, new TotalHits(2, TotalHits.Relation.EQUAL_TO), 1.0f),
null,
null,
false,
null,
null,
1
)
) {
FetchLookupFieldsPhase phase = new FetchLookupFieldsPhase(searchPhaseContext, sections, null);
phase.run();
} finally {
searchHits.decRef();
}
assertTrue(requestSent.get());
searchPhaseContext.assertNoFailure();
Expand Down