Remove some redundant ref-counting from SearchHits (#124948)

Remove ref-counting that is obviously redundant because of clear
ownership transfers from `SearchHits`.

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

@@ -277,26 +277,28 @@ public final class SearchPhaseController {
         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++) {

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

@@ -61,7 +61,6 @@ public class SearchResponseSections implements Releasable {
         int numReducePhases
     ) {
         this.hits = hits;
-        hits.incRef();
         this.aggregations = aggregations;
         this.suggest = suggest;
         this.profileResults = profileResults;

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

@@ -252,6 +252,7 @@ public final class SearchHits implements Writeable, ChunkedToXContent, RefCounte
     }
 
     private void deallocate() {
+        var hits = this.hits;
         for (int i = 0; i < hits.length; i++) {
             assert hits[i] != null;
             hits[i].decRef();

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

@@ -84,12 +84,18 @@ public final class FetchPhase {
         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();
+                }
             }
         }
     }

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

@@ -67,7 +67,6 @@ public final class FetchSearchResult extends SearchPhaseResult {
             existing.decRef();
         }
         this.hits = hits;
-        hits.mustIncRef();
         assert this.profileResult == null;
         this.profileResult = profileResult;
     }

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

@@ -117,31 +117,34 @@ public class ExpandSearchPhaseTests extends ESTestCase {
 
                 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) {
@@ -188,8 +191,17 @@ public class ExpandSearchPhaseTests extends ESTestCase {
         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));
@@ -198,7 +210,6 @@ public class ExpandSearchPhaseTests extends ESTestCase {
             assertNull(mockSearchPhaseContext.searchResponse.get());
         } finally {
             mockSearchPhaseContext.results.close();
-            hits.decRef();
             collapsedHits.decRef();
         }
     }
@@ -217,19 +228,22 @@ public class ExpandSearchPhaseTests extends ESTestCase {
             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();
@@ -300,13 +314,20 @@ public class ExpandSearchPhaseTests extends ESTestCase {
 
             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();
@@ -364,13 +385,20 @@ public class ExpandSearchPhaseTests extends ESTestCase {
 
             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();

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

@@ -46,12 +46,19 @@ public class FetchLookupFieldsPhaseTests extends ESTestCase {
             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());
@@ -182,16 +189,19 @@ public class FetchLookupFieldsPhaseTests extends ESTestCase {
                     )
                 );
             }
-            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();