Remove some redundant ref-counting from SearchHits #124948
Conversation
Remove ref-counting that is obviously redundant because of clear ownership transfers from `SearchHits`.
original-brownbear
added
>non-issue
:Search Foundations/Search
|
Pinging @elastic/es-search-foundations (Team:Search Foundations) |
elasticsearchmachine
added
the
Team:Search Foundations
| SearchResponse theResponse = mockSearchPhaseContext.searchResponse.get(); | ||
| assertNotNull(theResponse); | ||
| assertEquals(numInnerHits, theResponse.getHits().getHits()[0].getInnerHits().size()); | ||
| ExpandSearchPhase phase = newExpandSearchPhase( |
There was a problem hiding this comment.
This stuff is what blows up the LoC changed in this PR but I felt it best to inline these values still since their lifecycle is now bound to the SearchResponseSections.
| @@ -252,6 +252,7 @@ public boolean decRef() { | |||
| } | |||
|
|
|||
| private void deallocate() { | |||
| var hits = this.hits; | |||
There was a problem hiding this comment.
Just saving the memory reference, turns out this was what made this method show up in profiling.
| ProfileResult profileResult = profiler.finish(); | ||
| // Only set the shardResults if building search hits was successful | ||
| if (hits != null) { | ||
| context.fetchResult().shardResult(hits, profileResult); |
There was a problem hiding this comment.
For ease of reviewing: this is the only production callsite where we pass a pooled instance into shardResult so changing shardResult(..) and this logic is relatively obviously safe.
| } | ||
| } | ||
|
|
||
| private static void mergeSuggest( | ||
| ReducedQueryPhase reducedQueryPhase, | ||
| AtomicArray<? extends SearchPhaseResult> fetchResultsArray, | ||
| SearchHits hits, | ||
| int currentOffset, |
There was a problem hiding this comment.
No need to escape the hits from the caller just to get their length, adjusting this to make things more obviously safe in the only callsite.
Remove ref-counting that is obviously redundant because of clear
ownership transfers from
SearchHits.This makes the compiler's life a little easier (i.e. saves cache and instructions) now and makes it easier to release
SearchHitsearlier than it is released today during response serialization.