From 25a0addb17d8d5f20cf951218b10c05151ce694a Mon Sep 17 00:00:00 2001 From: Alan Woodward Date: Tue, 31 Mar 2020 10:49:26 +0100 Subject: [PATCH] Don't double-wrap values (#54432) After commit #53661 converted the lang-expressions module to using DoubleValuesSource, we've seen a performance regression for expressions that use geopoints. Some investigation suggests that this may be due to GeoLatitudeValueSource and GeoLongitudeValueSource wrapping their per-document values in a DoubleValues.withDefault() class. Values exposed via expressions already have a '0' default value, so this extra wrapping is unnecessary, and is directly on the hot path. This commit removes the extra wrapping. --- .../script/expression/DateObjectValueSource.java | 4 ++-- .../script/expression/ExpressionScriptEngine.java | 3 +-- .../elasticsearch/script/expression/GeoEmptyValueSource.java | 4 ++-- .../script/expression/GeoLatitudeValueSource.java | 4 ++-- .../script/expression/GeoLongitudeValueSource.java | 4 ++-- 5 files changed, 9 insertions(+), 10 deletions(-) diff --git a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/DateObjectValueSource.java b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/DateObjectValueSource.java index d49dd6c0a19..2c027a060b9 100644 --- a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/DateObjectValueSource.java +++ b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/DateObjectValueSource.java @@ -54,7 +54,7 @@ class DateObjectValueSource extends FieldDataValueSource { LeafNumericFieldData leafData = (LeafNumericFieldData) fieldData.load(leaf); MutableDateTime joda = new MutableDateTime(0, DateTimeZone.UTC); NumericDoubleValues docValues = multiValueMode.select(leafData.getDoubleValues()); - return DoubleValues.withDefault(new DoubleValues() { + return new DoubleValues() { @Override public double doubleValue() throws IOException { joda.setMillis((long)docValues.doubleValue()); @@ -65,7 +65,7 @@ class DateObjectValueSource extends FieldDataValueSource { public boolean advanceExact(int doc) throws IOException { return docValues.advanceExact(doc); } - }, 0); + }; } @Override diff --git a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java index 53a5b8aa368..19db3880c9c 100644 --- a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java +++ b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/ExpressionScriptEngine.java @@ -23,7 +23,6 @@ import org.apache.lucene.expressions.Expression; import org.apache.lucene.expressions.SimpleBindings; import org.apache.lucene.expressions.js.JavascriptCompiler; import org.apache.lucene.expressions.js.VariableContext; -import org.apache.lucene.queries.function.valuesource.DoubleConstValueSource; import org.apache.lucene.search.DoubleValuesSource; import org.apache.lucene.search.SortField; import org.elasticsearch.SpecialPermission; @@ -512,7 +511,7 @@ public class ExpressionScriptEngine implements ScriptEngine { // but if we were to reverse it, we could provide a way to supply dynamic defaults for documents missing the field? Object value = params.get(variable); if (value instanceof Number) { - bindings.add(variable, new DoubleConstValueSource(((Number) value).doubleValue()).asDoubleValuesSource()); + bindings.add(variable, DoubleValuesSource.constant(((Number)value).doubleValue())); } else { throw new ParseException("Parameter [" + variable + "] must be a numeric type", 0); } diff --git a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/GeoEmptyValueSource.java b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/GeoEmptyValueSource.java index 5e3a20f532e..8960fe1f82c 100644 --- a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/GeoEmptyValueSource.java +++ b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/GeoEmptyValueSource.java @@ -40,7 +40,7 @@ final class GeoEmptyValueSource extends FieldDataBasedDoubleValuesSource { public DoubleValues getValues(LeafReaderContext leaf, DoubleValues scores) { LeafGeoPointFieldData leafData = (LeafGeoPointFieldData) fieldData.load(leaf); final MultiGeoPointValues values = leafData.getGeoPointValues(); - return DoubleValues.withDefault(new DoubleValues() { + return new DoubleValues() { @Override public double doubleValue() { return 1; @@ -50,7 +50,7 @@ final class GeoEmptyValueSource extends FieldDataBasedDoubleValuesSource { public boolean advanceExact(int doc) throws IOException { return values.advanceExact(doc); } - }, 0); + }; } @Override diff --git a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/GeoLatitudeValueSource.java b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/GeoLatitudeValueSource.java index 8b9a3282ec7..e3daf32427f 100644 --- a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/GeoLatitudeValueSource.java +++ b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/GeoLatitudeValueSource.java @@ -40,7 +40,7 @@ final class GeoLatitudeValueSource extends FieldDataBasedDoubleValuesSource { public DoubleValues getValues(LeafReaderContext leaf, DoubleValues scores) { LeafGeoPointFieldData leafData = (LeafGeoPointFieldData) fieldData.load(leaf); final MultiGeoPointValues values = leafData.getGeoPointValues(); - return DoubleValues.withDefault(new DoubleValues() { + return new DoubleValues() { @Override public double doubleValue() throws IOException { return values.nextValue().getLat(); @@ -50,7 +50,7 @@ final class GeoLatitudeValueSource extends FieldDataBasedDoubleValuesSource { public boolean advanceExact(int doc) throws IOException { return values.advanceExact(doc); } - }, 0); + }; } @Override diff --git a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/GeoLongitudeValueSource.java b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/GeoLongitudeValueSource.java index 9bd285f8da8..85bb6d4a311 100644 --- a/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/GeoLongitudeValueSource.java +++ b/modules/lang-expression/src/main/java/org/elasticsearch/script/expression/GeoLongitudeValueSource.java @@ -40,7 +40,7 @@ final class GeoLongitudeValueSource extends FieldDataBasedDoubleValuesSource { public DoubleValues getValues(LeafReaderContext leaf, DoubleValues scores) { LeafGeoPointFieldData leafData = (LeafGeoPointFieldData) fieldData.load(leaf); final MultiGeoPointValues values = leafData.getGeoPointValues(); - return DoubleValues.withDefault(new DoubleValues() { + return new DoubleValues() { @Override public double doubleValue() throws IOException { return values.nextValue().getLon(); @@ -50,7 +50,7 @@ final class GeoLongitudeValueSource extends FieldDataBasedDoubleValuesSource { public boolean advanceExact(int doc) throws IOException { return values.advanceExact(doc); } - }, 0.0); + }; } @Override