Expand the lifecycle of the AggregationContext by not-napoleon · Pull Request #94023 · elastic/elasticsearch

not-napoleon · 2023-02-22T15:35:38Z

This PR replaces and supersedes #93594

Relates to #89437

One big lesson we learned from the first prototype of a dense memory aggregations format, in order for aggregation BigArrays to live into the serialization phase on the data nodes, we needed to not release the circuit breaker at the end of collection. The AggregationContext currently manages all of aggregations memory. Rather than change that, this PR extends the life cycle of the AggregationContext so it isn't closed until QuerySearchResult is closed, at which point we have serialized the aggregation information back to the coordinating node.

Importantly, this PR enables ref counting on QuerySearchResult, in what I think is a straightforward way. My earlier attempt, linked above, delegated that ref counting to a wrapped releasable, but ran into problems because the QuerySearchResult gets created before the things it needs to release. This iteration delegates directly to a ref counter, and maintains a list of items to release, which currently is just the aggregation context. That list can be appended to after creation, thus resolving the timing issue.

elasticsearchmachine · 2023-02-22T15:36:05Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

elasticsearchmachine · 2023-02-22T15:36:06Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

This reverts commit 4a331a3.

This reverts commit 0e55ac5.

This reverts commit 931af39.

not-napoleon · 2023-03-24T14:00:28Z

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

                readerContext.setRescoreDocIds(rescoreDocIds);
+                // Since we are returning the raw QuerySearchResult here, there's no opportunity for the object that will be holding that
+                // reference to incRef it before our try block closes and decRef's it when it closes the search context
+                context.queryResult().incRef();


I'm a little uncomfortable with this, as it makes a lot of assumptions about our caller. But I don't see a way around that, since we'll decRef when the try-with-resources closes, and if that drops us to 0 references (which it should), we'll release the agg context.

It's common, and totally reasonable to assume (require?) that the caller will take ownership of the returned value, and release it when no longer needed. I think it's not clear from the code that this is required, so it might be worth documenting it in this method's JavaDocs. I would recommend against the inline comments here however, stuff like this seems useful when you write it but tends to just become clutter over time.

I think the caller does behave as expected in this case, right? It looks like we pass it (pretty) directly to a ChannelActionListener which does decRef() once it no longer needs it. (aside: we really should document this)

There's definitely a risk of leaks with this kind of API tho, so it's important that a leak in this area would be caught by tests. Is that the case here? For instance, if it's using potentially-recycled pages then we should be using a CountingPageCacheRecycler (preferred) or MockPageCacheRecycler (relies on GC).

Yes, the caller does the right thing and the tests do catch it if we don't. Honestly, the testing around this has been very good so far.

And you're right, I should just write javadoc for these methods. I hadn't because I've really only been focusing on this one aspect, which is not the main thrust of what's going on here at all, but they currently have none at all, so I'm sure I can do better than that.

Even thought this method is private, it makes sense to document the responsibility of the caller at the method level.

not-napoleon · 2023-03-24T14:02:21Z

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

                readerContext.setRescoreDocIds(rescoreDocIds);
+                // Since we are returning the raw QuerySearchResult here, there's no opportunity for the object that will be holding that
+                // reference to incRef it before our try block closes and decRef's it when it closes the search context
+                searchContext.queryResult().incRef();


As noted above, this is uncomfortable but I don't have a better solution.

not-napoleon · 2023-03-24T14:09:37Z

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

ScrollQueryFetchSearcResult is a thin wrapper over QueryFetchSearchResult, so it seems reasonable to delegate ref counting to the wrapped reference.

not-napoleon · 2023-03-24T15:25:32Z

There's a lot of places where things reach through SearchContext to modify QuerySearchResult, but don't actually acquire a reference to it. I'd like to refactor some of that to be more encapsulated, maybe in a follow up PR.

There's also the rather terrifying QuerySearchResult#readFromWithId method which essentially overwrites the QuerySearchResult in place. I'm not sure what to do about that.

Finally, QuerySearchResult has about five different constructors, including a null case, and exists in somewhat different states on the data nodes and the coordinating node. This work really only focuses on the data node side (because the AggregationContext that I'm trying to manage is data node only). I'm somewhat purposefully ignoring the coordinating node side of this, because I don't really know what changes I will need to make there yet.

not-napoleon · 2023-03-27T13:56:34Z

@elasticmachine run elasticsearch-ci/docs

DaveCTurner · 2023-03-27T18:36:15Z

There's also the rather terrifying QuerySearchResult#readFromWithId method which essentially overwrites the QuerySearchResult in place. I'm not sure what to do about that.

I think just before overwriting each releasable/refcounted field it should close/decRef the current value. Unless there's some (non-obvious-to-me) invariant which means that we're never meaningfully overwriting anything. But in that case we should be able to add assertions to that effect at least.

martijnvg

I left a few small questions/comments. In general this looks good to me.

martijnvg · 2023-04-12T07:18:05Z

server/src/main/java/org/elasticsearch/search/query/QuerySearchResult.java


+    private final RefCounted refCounted;
+
+    private List<Releasable> toRelease;


I think toRelease can be final?

martijnvg · 2023-04-12T07:19:41Z

server/src/main/java/org/elasticsearch/search/query/QuerySearchResult.java

+    }
+
+    public void addReleasable(Releasable releasable) {
+        toRelease.add(releasable);


Are we sure toRelease is always assigned when this method is invoked?

Currently? I'm pretty sure, but it's tricky. Right now, this class has basically three "modes" - data node, coordinating node, and null. The data node constructor path correctly initializes toRelease, but neither of the other to do. Coordinating mode could initialize it, and the associated ref counting, but we don't do anything with it there yet. The null case probably shouldn't? I'm not entirely clear on why we need that path, but it's definitely used. Seemed safer to throw (NPE) if we tried to add a releasable in a path we weren't expecting it, but I'm open to discussion if you'd rather do something else.

martijnvg · 2023-04-12T07:32:39Z

server/src/main/java/org/elasticsearch/index/SearchSlowLog.java

            messageFields.put("elasticsearch.slowlog.took_millis", TimeUnit.NANOSECONDS.toMillis(tookInNanos));
-            if (context.queryResult().getTotalHits() != null) {
-                messageFields.put("elasticsearch.slowlog.total_hits", context.queryResult().getTotalHits());
+            if (context.getTotalHits() != null) {


Just double checking, but the change to this file doesn't seem to be related with the core of this PR?

martijnvg · 2023-04-12T07:52:55Z

server/src/main/java/org/elasticsearch/search/query/QuerySearchResult.java

        }
    }

+    public void releaseAggregationContext() {


I think this method can have a private visibility? Since it is only called from line 104?

martijnvg · 2023-04-12T07:54:06Z

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

                readerContext.setRescoreDocIds(rescoreDocIds);
+                // Since we are returning the raw QuerySearchResult here, there's no opportunity for the object that will be holding that
+                // reference to incRef it before our try block closes and decRef's it when it closes the search context
+                context.queryResult().incRef();


Even thought this method is private, it makes sense to document the responsibility of the caller at the method level.

not-napoleon · 2023-04-17T20:20:20Z

@elasticmachine update branch

not-napoleon · 2023-04-19T17:03:26Z

That test failure reproduced in main. I've opened #95386 to investigate, but I don't think it needs to block this PR. I'm going to re-run it to get a different random seed.

not-napoleon · 2023-04-19T17:03:37Z

@elasticmachine run elasticsearch-ci/part-1

…95860) Refactorings done in #94023. improved memory management but also made it possible to run into a race condition in DelegatingCircuitBreaker. This change makes DelegatingCircuitBreaker thread-safe. This change is a quick-fix with low risk. Eventually DelegatingCircuitBreaker should be removed. Work on that is underway: #89437. Under that circumstances the change is reasonable to me. The added locking overhead shouldn't be a problem, re-allocations happen rarely and there is just the disconnect case where 2 threads potentially access the structure at the same time. fixes #95681

…lastic#95860) Refactorings done in elastic#94023. improved memory management but also made it possible to run into a race condition in DelegatingCircuitBreaker. This change makes DelegatingCircuitBreaker thread-safe. This change is a quick-fix with low risk. Eventually DelegatingCircuitBreaker should be removed. Work on that is underway: elastic#89437. Under that circumstances the change is reasonable to me. The added locking overhead shouldn't be a problem, re-allocations happen rarely and there is just the disconnect case where 2 threads potentially access the structure at the same time. fixes elastic#95681

…95860) (#95879) Refactorings done in #94023. improved memory management but also made it possible to run into a race condition in DelegatingCircuitBreaker. This change makes DelegatingCircuitBreaker thread-safe. This change is a quick-fix with low risk. Eventually DelegatingCircuitBreaker should be removed. Work on that is underway: #89437. Under that circumstances the change is reasonable to me. The added locking overhead shouldn't be a problem, re-allocations happen rarely and there is just the disconnect case where 2 threads potentially access the structure at the same time. fixes #95681

not-napoleon added 2 commits February 22, 2023 09:16

let QuerySearchResult manage some releasables

77b9c3d

Give QuerySearchResult responsibility for releasing the agg context

344e36e

not-napoleon added :Analytics/Aggregations Aggregations >refactoring v8.8.0 labels Feb 22, 2023

elasticsearchmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label Feb 22, 2023

not-napoleon added 6 commits March 2, 2023 15:12

ref counting behavior

931af39

a few more places that probably need to incRef

0e55ac5

apparently spotless doesn't like twospace after a period

4a331a3

Revert "apparently spotless doesn't like twospace after a period"

93d2105

This reverts commit 4a331a3.

Revert "a few more places that probably need to incRef"

bc6a652

This reverts commit 0e55ac5.

Revert "ref counting behavior"

8dd19c0

This reverts commit 931af39.

not-napoleon mentioned this pull request Mar 8, 2023

Limit the resutls objects SearchContext creates #94405

Merged

not-napoleon added 9 commits March 15, 2023 14:28

QueryFetch result should delegate ref counting

e727706

better ref counting for QueryFetch...

d68a82d

Merge branch 'main' into preallocated-breaker-lifetime-v2

4580ff0

Add results objects before creating aggregations.

b50bf5b

DFS phase probably doesn't need aggregations?

125f86a

delegate refcounting for ScrollQueryFetchResult

20152a1

spotless apply

41405cb

incRef when we keep a copy of QSR

35f0b4d

Tests looking at aggs should create the right results type

cdc902e

not-napoleon mentioned this pull request Mar 24, 2023

Expand the life cycle of the preallocated circuit breaker #93594

Closed

not-napoleon commented Mar 24, 2023

View reviewed changes

not-napoleon requested a review from jdconrad March 24, 2023 15:28

not-napoleon requested a review from nik9000 March 24, 2023 15:28

henningandersen self-requested a review March 24, 2023 16:10

not-napoleon requested a review from martijnvg April 5, 2023 15:40

not-napoleon mentioned this pull request Apr 6, 2023

Enable Circuit Breaker tracking in more parts of the aggregations framework #89437

Open

34 tasks

martijnvg approved these changes Apr 12, 2023

View reviewed changes

not-napoleon added 2 commits April 17, 2023 10:01

javadoc

df3eed5

PR Feedback

7b2efda

Merge branch 'main' into preallocated-breaker-lifetime-v2

be66285

not-napoleon merged commit cb04885 into elastic:main Apr 20, 2023

not-napoleon deleted the preallocated-breaker-lifetime-v2 branch April 20, 2023 13:23

This was referenced May 4, 2023

[CI] MlWithSecurityIT test {yaml=ml/frequent_item_sets_agg/Test frequent item sets filter} failing #95681

Closed

[ML] fix possible race condition in frequent item sets aggregation #95860

Merged


		private final RefCounted refCounted;

		private List<Releasable> toRelease;

Conversation

not-napoleon commented Feb 22, 2023

Uh oh!

elasticsearchmachine commented Feb 22, 2023

Uh oh!

elasticsearchmachine commented Feb 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

not-napoleon commented Mar 24, 2023

Uh oh!

not-napoleon commented Mar 27, 2023

Uh oh!

DaveCTurner commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martijnvg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

not-napoleon commented Apr 17, 2023

Uh oh!

not-napoleon commented Apr 19, 2023

Uh oh!

not-napoleon commented Apr 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

DaveCTurner commented Mar 27, 2023 •

edited

Loading