Ref count search response bytes #103763

original-brownbear · 2023-12-29T19:55:44Z

Final step in #102030 ... actually makes SearchHit read a releasable bytes reference.
Does still fallback to copying to unrolled buffers here and there which can be removed in follow-ups where it's worth the effort (aggs being the most important one probably).

Hard to create very reliable benchmarks for this because all our macro-benchmarks are quite noisy. Running http logs and PMC though, there's a statistically significant reduction in GC and reduced tail latencies in most benchmarks.

The overhead for ref-counting these bytes isn't visible in profiling as far as I can tell and for large source values, no corresponding large byte[] are created any longer outside of the few remaining spots where we copy to pooled buffers.

closes #102657

…ponse-itself

…' into ref-count-search-response-itself

…ponse-itself

…' into ref-count-search-response-itself

…ponse-itself

It's in the title. Fix all the tests like we have everywhere else. Fix the production use of SearchResponse in BlobStoreCacheMaintenanceService by properly ref-counting the response it holds on to.

…ponse-itself

…-snapshots

…ponse-itself

…esponse-itself

elasticsearchmachine · 2024-01-10T20:33:06Z

Pinging @elastic/es-search (Team:Search)

elasticsearchmachine · 2024-01-10T20:33:06Z

Hi @original-brownbear, I've created a changelog YAML for you.

…ponse-bytes

original-brownbear · 2024-01-11T13:59:34Z

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

-    private final RefCounted refCounted = LeakTracker.wrap(AbstractRefCounted.of(this::doClose));
+    private final AtomicBoolean closed = new AtomicBoolean(false);
+
+    private final Releasable releasable = LeakTracker.wrap(() -> {


Changed plans on this one. Ref-counting was not appropriate here. We want to have this tracked by the context to be release as a last-resort but we also clearly know when this can be released earlier. So I just made it closable with the double-close noop so we can close it wherever.

...k/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/persistence/JobResultsProvider.java

…ponse-bytes

… into ref-count-search-response-bytes

elasticsearchmachine · 2024-01-12T00:22:22Z

Hi @original-brownbear, I've updated the changelog YAML for you.

…ponse-bytes

… into ref-count-search-response-bytes

benwtrent

Whew! what a read. This is awesome.

I eagerly await the performance numbers to see if there is a significant improvement.

It is frustrating to see how many non-test things require unPooled :(. But hopefully overtime those things can be updated to ensure we don't copy around bytes needlessly.

benwtrent · 2024-01-12T17:49:42Z

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

+            sortFields,
+            collapseField,
+            collapseValues,
+            hits.length == 0 ? ALWAYS_REFERENCED : LeakTracker.wrap(new AbstractRefCounted() {


I don't think this should be ref counted if none of the SearchHit[] values are not. It seems weird to ref count for no reason.

True, my reasoning for not adding that check would be that in production usage of this should effectively always work with ref-counted hirs and that condition would force another redundant iteration over the array on us.

modules/rank-eval/src/main/java/org/elasticsearch/index/rankeval/RatedSearchHit.java

benwtrent · 2024-01-12T17:56:30Z

...c/internalClusterTest/java/org/elasticsearch/search/functionscore/RandomScoreFunctionIT.java

@@ -112,7 +112,7 @@ public void testConsistentHitsWithSameSeed() throws Exception {
                            CoreMatchers.equalTo(0)
                        );
                        final int hitCount = response.getHits().getHits().length;
-                        final SearchHit[] currentHits = response.getHits().getHits();
+                        final SearchHit[] currentHits = response.getHits().asUnpooled().getHits();


It doesn't seem like we need to asUnpooled() these hits unless we drop into the if (finalI == 0) block below.

True but it's more code/change to make that conditional with how things work today. I'd deal with this in a follow-up where I was planning to restrict the explicit use of the array backing a SearchHits as much as possible.

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

benwtrent · 2024-01-12T18:11:08Z

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

-            resp.decRef();
-        }
+        context.addReleasable(resp::decRef);
+        fetchResults.close();


This is weird to me, we add fetchResults to context.addReleasable in the FetchSearchPhase ctor, but then we call close on it here. So, are we just closing it early here and relying on the context for releasing in a failure scenario?

Yes exactly that was the plan.

benwtrent · 2024-01-12T18:34:06Z

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

@@ -448,6 +497,7 @@ public String getSourceAsString() {
     * The source of the document as a map (can be {@code null}).
     */
    public Map<String, Object> getSourceAsMap() {
+        assert hasReferences();


I think we only need references of sourceAsMap == null

Technically yes, but I went for being strict here. We really shouldn't have code touching released instances ever. Much easier to trust tests if we verify that fact in more rather than fewer spots IMO

benwtrent · 2024-01-12T18:36:06Z

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

@@ -463,6 +513,7 @@ public Map<String, Object> getSourceAsMap() {
     * The hit field matching the given field name.
     */
    public DocumentField field(String fieldName) {
+        assert hasReferences();


Do we really need references here? DocumentField maps don't seem referenced.

#103763 (comment) same reasoning here. I'd love to do a follow-up that expands ref counting to the fields in some cases as well.

benwtrent · 2024-01-12T18:36:59Z

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

@@ -653,13 +704,72 @@ public Map<String, Float> getMatchedQueriesAndScores() {
     * @return Inner hits or <code>null</code> if there are none
     */
    public Map<String, SearchHits> getInnerHits() {
+        assert hasReferences();


Should we also assert that the inner hits objects have references?

Maybe not right? If we do that here, then we'd have to do it for the source itself before returning it and spots like that also? Seems like consistently asserting ref-counts on fields of ref-counted stuff is going to be a lot of code and doesn't add that much since whoever gets this map is probably going to do something with its contents anyway?

benwtrent · 2024-01-12T18:39:50Z

server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalTopHits.java

@@ -52,7 +52,7 @@ public InternalTopHits(
        this.from = from;
        this.size = size;
        this.topDocs = topDocs;
-        this.searchHits = searchHits;
+        this.searchHits = searchHits.asUnpooled();


One day this shall be pooled as well! :D

benwtrent · 2024-01-12T18:42:00Z

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

+            for (SearchHit searchHit : searchHits) {
+                if (searchHit != null) {
+                    searchHit.decRef();
+                }
+            }


benwtrent · 2024-01-12T19:04:16Z

@original-brownbear could you post some numbers for a before/after of the benchmarks for this change?

…ponse-bytes

benwtrent

Lots to do yet. But this is awesome. I am mostly concerned about multi-node search testing, and CI should handle most of that. We just need to be vigilant.

… into ref-count-search-response-bytes

…sponse-bytes # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

original-brownbear · 2024-01-17T15:16:21Z

Wee thanks so much Ben! Merging with Luca's blessing! I'll be on the lookout for leaks :)!

astefan

~~LGTM. Thank you, Armin.~~ Wrong PR review.

original-brownbear and others added 30 commits November 30, 2023 17:05

Fix more leaked SearchResponse instances in tests

6825cb2

save

5474223

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

277584f

…ponse-itself

clippyh

b2a9124

fixes

55d1f40

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

02319b3

…ponse-itself

Merge remote-tracking branch 'origin/ref-count-search-response-itself…

c301177

…' into ref-count-search-response-itself

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

8b42a9c

…ponse-itself

fix ml

cf9d196

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

a827020

…ponse-itself

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

8230777

…ponse-itself

fix eql

19efcac

Merge remote-tracking branch 'origin/ref-count-search-response-itself…

00e73bd

…' into ref-count-search-response-itself

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

b4da0b6

…ponse-itself

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

f234e75

…ponse-itself

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

d78047a

…ponse-itself

fixe

2f54229

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

a78f555

…ponse-itself

fix

a064f6d

ix

a250db6

ix

a64e6a6

retain ref async

2506dc1

Fix SearchResponse leaks in searchable snapshot test and production code

66e110e

It's in the title. Fix all the tests like we have everywhere else. Fix the production use of SearchResponse in BlobStoreCacheMaintenanceService by properly ref-counting the response it holds on to.

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

92417b2

…ponse-itself

Merge branch 'main' into fix-leaks-searchable-snapshots

1f531e2

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

7320701

…ponse-itself

Merge remote-tracking branch 'elastic/main' into fix-leaks-searchable…

24a3614

…-snapshots

Merge remote-tracking branch 'elastic/main' into fix-leaks-searchable…

dab8843

…-snapshots

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

cb5eff3

…ponse-itself

Merge branch 'fix-leaks-searchable-snapshots' into ref-count-search-r…

a53459e

…esponse-itself

original-brownbear added >enhancement and removed WIP labels Jan 10, 2024

elasticsearchmachine added the Team:Search Meta label for search team label Jan 10, 2024

original-brownbear and others added 2 commits January 10, 2024 21:33

Update docs/changelog/103763.yaml

306b232

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

c6aae01

…ponse-bytes

original-brownbear commented Jan 11, 2024

View reviewed changes

...k/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/job/persistence/JobResultsProvider.java Outdated Show resolved Hide resolved

original-brownbear added 3 commits January 11, 2024 15:10

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

f7e9fd9

…ponse-bytes

shorter

b49102b

Merge remote-tracking branch 'origin/ref-count-search-response-bytes'…

2ec731a

… into ref-count-search-response-bytes

original-brownbear and others added 3 commits January 12, 2024 01:22

Update docs/changelog/103763.yaml

0c737fc

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

e99d091

…ponse-bytes

Merge remote-tracking branch 'origin/ref-count-search-response-bytes'…

b66ebc2

… into ref-count-search-response-bytes

benwtrent reviewed Jan 12, 2024

View reviewed changes

original-brownbear added 4 commits January 12, 2024 20:10

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

bfb2b68

…ponse-bytes

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

2b135ac

…ponse-bytes

CR: comments

7c4744c

Merge remote-tracking branch 'elastic/main' into ref-count-search-res…

42caa0e

…ponse-bytes

benwtrent approved these changes Jan 17, 2024

View reviewed changes

original-brownbear added 2 commits January 17, 2024 13:38

Merge remote-tracking branch 'origin/ref-count-search-response-bytes'…

b12a673

… into ref-count-search-response-bytes

original-brownbear merged commit 73a6840 into elastic:main Jan 17, 2024
14 of 15 checks passed

original-brownbear deleted the ref-count-search-response-bytes branch January 17, 2024 15:16

DaveCTurner mentioned this pull request Jan 18, 2024

[LEAK: resource was not cleaned up before it was garbage-collected #104522

Closed

astefan reviewed Feb 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ref count search response bytes #103763

Ref count search response bytes #103763

original-brownbear commented Dec 29, 2023 •

edited

Loading

elasticsearchmachine commented Jan 10, 2024

elasticsearchmachine commented Jan 10, 2024

original-brownbear Jan 11, 2024

elasticsearchmachine commented Jan 12, 2024

benwtrent left a comment

benwtrent Jan 12, 2024

original-brownbear Jan 16, 2024

benwtrent Jan 12, 2024

original-brownbear Jan 16, 2024

benwtrent Jan 12, 2024

original-brownbear Jan 16, 2024

benwtrent Jan 12, 2024

original-brownbear Jan 16, 2024

benwtrent Jan 12, 2024

original-brownbear Jan 16, 2024

benwtrent Jan 12, 2024

original-brownbear Jan 16, 2024

benwtrent Jan 12, 2024

original-brownbear Jan 16, 2024

benwtrent Jan 12, 2024

benwtrent commented Jan 12, 2024

benwtrent left a comment

original-brownbear commented Jan 17, 2024

astefan left a comment •

edited

Loading

Ref count search response bytes #103763

Ref count search response bytes #103763

Conversation

original-brownbear commented Dec 29, 2023 • edited Loading

elasticsearchmachine commented Jan 10, 2024

elasticsearchmachine commented Jan 10, 2024

Choose a reason for hiding this comment

elasticsearchmachine commented Jan 12, 2024

benwtrent left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benwtrent commented Jan 12, 2024

benwtrent left a comment

Choose a reason for hiding this comment

original-brownbear commented Jan 17, 2024

astefan left a comment • edited Loading

Choose a reason for hiding this comment

original-brownbear commented Dec 29, 2023 •

edited

Loading

astefan left a comment •

edited

Loading