Try to save memory on aggregations #53793

nik9000 · 2020-03-19T13:18:56Z

This delays deserializing the aggregation response try until right
before we merge the objects. It isn't super clear how much space this
saves, but:

It turns some object from fairly long lives to very short lived
which is almost certainly a good thing from a JVM perspective.
It gives us a convenient place to check how much space the
aggregation tree uses.
It probably does save a fair bit of memory because of the overhead
the JVM has for many small objects, which the aggregation tree is
mostly made up of.

This delays deserializing the aggregation response try until *right* before we merge the objects.

nik9000 · 2020-03-19T13:19:17Z

I'm making this a draft PR so the robots can chew on it.

nik9000 · 2020-03-19T14:06:49Z

server/src/main/java/org/elasticsearch/common/io/stream/DelayableWriteable.java

+ * A holder for {@link Writeable}s that can delays reading the underlying
+ * {@linkplain Writeable} when it is read from a remote node.
+ */
+public abstract class DelayableWriteable<T extends Writeable> implements Supplier<T>, Writeable {


We're only using this for InternalAggregations, but it is a heck of a lot simpler to test if it is generic.

nik9000 · 2020-03-19T14:29:48Z

Ok, robots, get to work!

nik9000 · 2020-03-19T15:39:24Z

Ok, robots, get to work!

OK! The tests are passing. I think it is time to un-draft this!

elasticmachine · 2020-03-19T15:39:51Z

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

jimczi

I left some questions but I like this change a lot. It's simple but it could also improve the memory profile of coordinating node significantly.

jimczi · 2020-03-19T20:41:03Z

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

                        Arrays.fill(aggsBuffer, null);
-                        aggsBuffer[0] = reducedAggs;
+                        aggsBuffer[0] = () -> reducedAggs;


should we nullify the rest of the array to make the reduced aggs eligible for gc ?

Actually the line right above does that.

Right but we keep the serialized + deserialized form until after the partial reduce. We can try to release the serialized form early with:

List<InternalAggregations> toReduce = Arrays.stream(aggsBuffer).map(Supplier::get).collect(toList()); Arrays.fill(aggsBuffer, null); InternalAggregaions reducedAggs = InternalAggregations.topLevelReduce(toReduce, aggReduceContextBuilder.forPartialReduction()); aggsBuffer[0] = () -> reducedAggs;

Or we can nullify the serialized form when the supplier is called like discussed below.

Right! I noticed that right after I sent this. I'm playing with nulling the cell in the array as soon as I call get. That feels a little safer than nulling the bytes.

jimczi · 2020-03-19T20:42:40Z

server/src/main/java/org/elasticsearch/common/io/stream/DelayableWriteable.java

+                try (StreamInput in = registry == null ?
+                        serialized.streamInput() : new NamedWriteableAwareStreamInput(serialized.streamInput(), registry)) {
+                    in.setVersion(remoteVersion);
+                    return reader.read(in);


Should we nullify the bytes ref before returning the deserialized aggs ? We could also protect against multiple calls by keeping the deserialized aggs internally on the first call ?

I'm worried about race conditions with that. The way it is it is fairly simple the look at and say "there are no race conditions." I think nulifying the other references would be good enough from a GC perspective. Do you?

yep nullifying the reference should be enough but it would be better if we can nullify after each deserialization. Otherwise you'd need to keep the deserialized aggs and their bytes representation during the entire partial reduce which defeats the purpose of saving memories here ?

jimczi · 2020-03-19T20:45:30Z

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

@@ -366,7 +371,11 @@ public void writeToNoId(StreamOutput out) throws IOException {
            out.writeBoolean(false);
        } else {
            out.writeBoolean(true);
-            aggregations.writeTo(out);
+            if (out.getVersion().before(Version.V_8_0_0)) {
+                aggregations.get().writeTo(out);


we can maybe get the aggs once if the remote node is in a version before v8 (instead of calling get here and below to get the pipeline aggs) ?

@nik9000 ? should we avoid the double deserialization if we need the pipeline aggs below ?

Darn it. I twisted the other side around but missed this comment. Of course!

Mostly these are going to be the "referencing" ones anyway. But I'll turn it around.

jimczi · 2020-03-19T20:47:39Z

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

@@ -54,7 +55,7 @@
    private TotalHits totalHits;
    private float maxScore = Float.NaN;
    private DocValueFormat[] sortValueFormats;
-    private InternalAggregations aggregations;


Can you add a comment here to explain why we use a delayable writable ?

nik9000 · 2020-03-23T13:13:32Z

@jimczi I think this is ready for another round.

jimczi

I left one comment but the change looks good to me.
One simple follow up could be to add the memory consumed by these aggregations in the request circuit breaker and to release it when we perform a partial/final reduce ?

jimczi · 2020-03-23T13:20:56Z

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

@@ -366,7 +371,11 @@ public void writeToNoId(StreamOutput out) throws IOException {
            out.writeBoolean(false);
        } else {
            out.writeBoolean(true);
-            aggregations.writeTo(out);
+            if (out.getVersion().before(Version.V_8_0_0)) {
+                aggregations.get().writeTo(out);


@nik9000 ? should we avoid the double deserialization if we need the pipeline aggs below ?

nik9000 · 2020-03-23T13:35:54Z

One simple follow up could be to add the memory consumed by these aggregations in the request circuit breaker and to release it when we perform a partial/final reduce ?

Yeah! I'd also love to add it somewhere where I can see it. But that does seem like a great thing.

This delays deserializing the aggregation response try until *right* before we merge the objects.

Update version in wire protocol and disable BWC.

This delays deserializing the aggregation response try until *right* before we merge the objects.

I created this bug today in elastic#53793. When a `DelayableWriteable` that references an existing object serializes itself it wasn't taking the version of the node on the other side of the wire into account. This fixes that.

I created this bug today in #53793. When a `DelayableWriteable` that references an existing object serializes itself it wasn't taking the version of the node on the other side of the wire into account. This fixes that.

* Reenable BWC tests after backport of #53793

Try to save memory on aggregations

39d604c

This delays deserializing the aggregation response try until *right* before we merge the objects.

nik9000 commented Mar 19, 2020

View reviewed changes

nik9000 added 2 commits March 19, 2020 10:27

Fix request cache

8cddcab

Merge branch 'master' into agg_await_deserialization

04dba53

nik9000 marked this pull request as ready for review March 19, 2020 15:39

nik9000 requested a review from jimczi March 19, 2020 15:39

nik9000 added :Analytics/Aggregations Aggregations >enhancement v7.7.0 v8.0.0 labels Mar 19, 2020

jimczi reviewed Mar 19, 2020

View reviewed changes

nik9000 added 4 commits March 19, 2020 18:18

Explain

6921c8c

Rework read

76b077d

clear asap

eb37343

Checkstyle

80e31ad

jimczi approved these changes Mar 23, 2020

View reviewed changes

Twist

c6162ff

nik9000 merged commit 1ca52fc into elastic:master Mar 23, 2020

nik9000 added the backport pending label Mar 23, 2020

nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Mar 23, 2020

Try to save memory on aggregations (backport of elastic#53793)

f35c346

This delays deserializing the aggregation response try until *right* before we merge the objects.

nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Mar 23, 2020

Prepare to backport elastic#53793

33ef69e

Update version in wire protocol and disable BWC.

nik9000 added a commit that referenced this pull request Mar 23, 2020

Prepare to backport #53793 (#54010)

1016e20

Update version in wire protocol and disable BWC.

nik9000 added a commit that referenced this pull request Mar 23, 2020

Try to save memory on aggregations (backport of #53793) (#53996)

181bc80

This delays deserializing the aggregation response try until *right* before we merge the objects.

nik9000 added a commit to nik9000/elasticsearch that referenced this pull request Mar 23, 2020

Reenable BWC tests after backport of elastic#53793

17a5bfe

nik9000 mentioned this pull request Mar 23, 2020

Fix serialization bug for aggs #54029

Merged

nik9000 added a commit that referenced this pull request Mar 23, 2020

Reenable BWC tests after backport of #53793 (#54018)

0bce5d2

* Reenable BWC tests after backport of #53793

bpintea removed the backport pending label Mar 25, 2020

jimczi mentioned this pull request Mar 27, 2020

Change the default batched_reduce_size of search requests #51857

Open

codebrain mentioned this pull request Apr 1, 2020

7.7.0 meta ticket (Part 2) elastic/elasticsearch-net#4533

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to save memory on aggregations #53793

Try to save memory on aggregations #53793

nik9000 commented Mar 19, 2020 •

edited

Loading

nik9000 commented Mar 19, 2020

nik9000 Mar 19, 2020

nik9000 commented Mar 19, 2020

nik9000 commented Mar 19, 2020

elasticmachine commented Mar 19, 2020

jimczi left a comment

jimczi Mar 19, 2020

nik9000 Mar 19, 2020

nik9000 Mar 20, 2020

jimczi Mar 20, 2020

nik9000 Mar 20, 2020

jimczi Mar 19, 2020

nik9000 Mar 19, 2020

jimczi Mar 19, 2020

jimczi Mar 19, 2020

jimczi Mar 23, 2020

nik9000 Mar 23, 2020

nik9000 Mar 23, 2020

jimczi Mar 19, 2020

nik9000 Mar 19, 2020

nik9000 commented Mar 23, 2020

jimczi left a comment

jimczi Mar 23, 2020

nik9000 commented Mar 23, 2020

Try to save memory on aggregations #53793

Try to save memory on aggregations #53793

Conversation

nik9000 commented Mar 19, 2020 • edited Loading

nik9000 commented Mar 19, 2020

Choose a reason for hiding this comment

nik9000 commented Mar 19, 2020

nik9000 commented Mar 19, 2020

elasticmachine commented Mar 19, 2020

jimczi 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

nik9000 commented Mar 23, 2020

jimczi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nik9000 commented Mar 23, 2020

nik9000 commented Mar 19, 2020 •

edited

Loading