From 2448eecea9e29e9d7daa838871dbf789f660e2a9 Mon Sep 17 00:00:00 2001 From: Ignacio Vera Date: Wed, 6 Nov 2024 16:22:38 +0100 Subject: [PATCH] Make InternalCentroid leaner (#116302) We are currently holding to fields to extract values, this commit makes them abstract methods so we don't use any heap. --- .../metrics/InternalCentroid.java | 50 +++++++------------ .../metrics/InternalGeoCentroid.java | 37 ++++++++------ .../metrics/InternalCartesianCentroid.java | 26 +++++++--- 3 files changed, 59 insertions(+), 54 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalCentroid.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalCentroid.java index 05dd82fd59c4f..eb789bcdd8a74 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalCentroid.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalCentroid.java @@ -23,7 +23,6 @@ import java.util.List; import java.util.Map; import java.util.Objects; -import java.util.function.Function; /** * Serialization and merge logic for {@link GeoCentroidAggregator}. @@ -31,24 +30,13 @@ public abstract class InternalCentroid extends InternalAggregation implements CentroidAggregation { protected final SpatialPoint centroid; protected final long count; - private final FieldExtractor firstField; - private final FieldExtractor secondField; - - public InternalCentroid( - String name, - SpatialPoint centroid, - long count, - Map metadata, - FieldExtractor firstField, - FieldExtractor secondField - ) { + + public InternalCentroid(String name, SpatialPoint centroid, long count, Map metadata) { super(name, metadata); assert (centroid == null) == (count == 0); this.centroid = centroid; assert count >= 0; this.count = count; - this.firstField = firstField; - this.secondField = secondField; } protected abstract SpatialPoint centroidFromStream(StreamInput in) throws IOException; @@ -59,7 +47,7 @@ public InternalCentroid( * Read from a stream. */ @SuppressWarnings("this-escape") - protected InternalCentroid(StreamInput in, FieldExtractor firstField, FieldExtractor secondField) throws IOException { + protected InternalCentroid(StreamInput in) throws IOException { super(in); count = in.readVLong(); if (in.readBoolean()) { @@ -67,8 +55,6 @@ protected InternalCentroid(StreamInput in, FieldExtractor firstField, FieldExtra } else { centroid = null; } - this.firstField = firstField; - this.secondField = secondField; } @Override @@ -110,11 +96,11 @@ public void accept(InternalAggregation aggregation) { if (centroidAgg.count > 0) { totalCount += centroidAgg.count; if (Double.isNaN(firstSum)) { - firstSum = centroidAgg.count * firstField.extractor.apply(centroidAgg.centroid); - secondSum = centroidAgg.count * secondField.extractor.apply(centroidAgg.centroid); + firstSum = centroidAgg.count * extractFirst(centroidAgg.centroid); + secondSum = centroidAgg.count * extractSecond(centroidAgg.centroid); } else { - firstSum += centroidAgg.count * firstField.extractor.apply(centroidAgg.centroid); - secondSum += centroidAgg.count * secondField.extractor.apply(centroidAgg.centroid); + firstSum += centroidAgg.count * extractFirst(centroidAgg.centroid); + secondSum += centroidAgg.count * extractSecond(centroidAgg.centroid); } } } @@ -126,6 +112,14 @@ public InternalAggregation get() { }; } + protected abstract String nameFirst(); + + protected abstract double extractFirst(SpatialPoint point); + + protected abstract String nameSecond(); + + protected abstract double extractSecond(SpatialPoint point); + @Override public InternalAggregation finalizeSampling(SamplingContext samplingContext) { return copyWith(centroid, samplingContext.scaleUp(count)); @@ -136,16 +130,6 @@ protected boolean mustReduceOnSingleInternalAgg() { return false; } - protected static class FieldExtractor { - private final String name; - private final Function extractor; - - public FieldExtractor(String name, Function extractor) { - this.name = name; - this.extractor = extractor; - } - } - protected abstract double extractDouble(String name); @Override @@ -174,8 +158,8 @@ public XContentBuilder doXContentBody(XContentBuilder builder, Params params) th if (centroid != null) { builder.startObject(Fields.CENTROID.getPreferredName()); { - builder.field(firstField.name, firstField.extractor.apply(centroid)); - builder.field(secondField.name, secondField.extractor.apply(centroid)); + builder.field(nameFirst(), extractFirst(centroid)); + builder.field(nameSecond(), extractSecond(centroid)); } builder.endObject(); } diff --git a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalGeoCentroid.java b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalGeoCentroid.java index 10e301608ec2f..1609046d59708 100644 --- a/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalGeoCentroid.java +++ b/server/src/main/java/org/elasticsearch/search/aggregations/metrics/InternalGeoCentroid.java @@ -15,7 +15,6 @@ import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.support.SamplingContext; -import org.elasticsearch.xcontent.ParseField; import java.io.IOException; import java.util.Map; @@ -26,21 +25,14 @@ public class InternalGeoCentroid extends InternalCentroid implements GeoCentroid { public InternalGeoCentroid(String name, SpatialPoint centroid, long count, Map metadata) { - super( - name, - centroid, - count, - metadata, - new FieldExtractor("lat", SpatialPoint::getY), - new FieldExtractor("lon", SpatialPoint::getX) - ); + super(name, centroid, count, metadata); } /** * Read from a stream. */ public InternalGeoCentroid(StreamInput in) throws IOException { - super(in, new FieldExtractor("lat", SpatialPoint::getY), new FieldExtractor("lon", SpatialPoint::getX)); + super(in); } public static InternalGeoCentroid empty(String name, Map metadata) { @@ -84,12 +76,27 @@ protected InternalGeoCentroid copyWith(double firstSum, double secondSum, long t } @Override - public InternalAggregation finalizeSampling(SamplingContext samplingContext) { - return new InternalGeoCentroid(name, centroid, samplingContext.scaleUp(count), getMetadata()); + protected String nameFirst() { + return "lat"; + } + + @Override + protected double extractFirst(SpatialPoint point) { + return point.getY(); + } + + @Override + protected String nameSecond() { + return "lon"; + } + + @Override + protected double extractSecond(SpatialPoint point) { + return point.getX(); } - static class Fields { - static final ParseField CENTROID_LAT = new ParseField("lat"); - static final ParseField CENTROID_LON = new ParseField("lon"); + @Override + public InternalAggregation finalizeSampling(SamplingContext samplingContext) { + return new InternalGeoCentroid(name, centroid, samplingContext.scaleUp(count), getMetadata()); } } diff --git a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/metrics/InternalCartesianCentroid.java b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/metrics/InternalCartesianCentroid.java index e009e07d35aa4..63f43458c79b5 100644 --- a/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/metrics/InternalCartesianCentroid.java +++ b/x-pack/plugin/spatial/src/main/java/org/elasticsearch/xpack/spatial/search/aggregations/metrics/InternalCartesianCentroid.java @@ -13,7 +13,6 @@ import org.elasticsearch.search.aggregations.InternalAggregation; import org.elasticsearch.search.aggregations.metrics.InternalCentroid; import org.elasticsearch.search.aggregations.support.SamplingContext; -import org.elasticsearch.xcontent.ParseField; import org.elasticsearch.xpack.spatial.common.CartesianPoint; import java.io.IOException; @@ -25,14 +24,14 @@ public class InternalCartesianCentroid extends InternalCentroid implements CartesianCentroid { public InternalCartesianCentroid(String name, SpatialPoint centroid, long count, Map metadata) { - super(name, centroid, count, metadata, new FieldExtractor("x", SpatialPoint::getX), new FieldExtractor("y", SpatialPoint::getY)); + super(name, centroid, count, metadata); } /** * Read from a stream. */ public InternalCartesianCentroid(StreamInput in) throws IOException { - super(in, new FieldExtractor("x", SpatialPoint::getX), new FieldExtractor("y", SpatialPoint::getY)); + super(in); } @Override @@ -80,8 +79,23 @@ public InternalAggregation finalizeSampling(SamplingContext samplingContext) { return new InternalCartesianCentroid(name, centroid, samplingContext.scaleUp(count), getMetadata()); } - static class Fields { - static final ParseField CENTROID_X = new ParseField("x"); - static final ParseField CENTROID_Y = new ParseField("y"); + @Override + protected String nameFirst() { + return "x"; + } + + @Override + protected double extractFirst(SpatialPoint point) { + return point.getX(); + } + + @Override + protected String nameSecond() { + return "y"; + } + + @Override + protected double extractSecond(SpatialPoint point) { + return point.getY(); } }