Added performance improvement for datetime field parsing (#9567)

* Added performance improvement for datetime field parsing This adds caching of formatters in case of no explicit format specified for the datetime field in mapping. This also adds `strict_date_time_no_millis` as additional formatter in default date time formats Signed-off-by: Prabhat Sharma <ptsharma@amazon.com> * Refactor DateTimeFormatter Access under featireflag Signed-off-by: Prabhat Sharma <ptsharma@amazon.com> --------- Signed-off-by: Prabhat Sharma <ptsharma@amazon.com> Co-authored-by: Prabhat Sharma <ptsharma@amazon.com>
opensearch-project · Oct 5, 2023 · 2965e69 · 2965e69
1 parent de8511b
commit 2965e69
Show file tree

Hide file tree

Showing 33 changed files with 265 additions and 70 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -54,6 +54,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 - Change http code on create index API with bad input raising NotXContentException from 500 to 400 ([#4773](https://github.com/opensearch-project/OpenSearch/pull/4773))
 - Improve summary error message for invalid setting updates ([#4792](https://github.com/opensearch-project/OpenSearch/pull/4792))
 - Return 409 Conflict HTTP status instead of 503 on failure to concurrently execute snapshots ([#8986](https://github.com/opensearch-project/OpenSearch/pull/5855))
+- Performance improvement for Datetime field caching ([#4558](https://github.com/opensearch-project/OpenSearch/issues/4558))
 
 ### Deprecated
 

diff --git a/distribution/src/config/opensearch.yml b/distribution/src/config/opensearch.yml
@@ -121,3 +121,9 @@ ${path.logs}
 # index searcher threadpool.
 #
 #opensearch.experimental.feature.concurrent_segment_search.enabled: false
+#
+#
+# Gates the optimization of datetime formatters caching along with change in default datetime formatter
+# Once there is no observed impact on performance, this feature flag can be removed.
+#
+#opensearch.experimental.optimization.datetime_formatter_caching.enabled: false
diff --git a/...c/internalClusterTest/java/org/opensearch/search/aggregations/bucket/DateHistogramIT.java b/...c/internalClusterTest/java/org/opensearch/search/aggregations/bucket/DateHistogramIT.java
@@ -124,7 +124,7 @@ protected Settings featureFlagSettings() {
     }
 
     private ZonedDateTime date(String date) {
-        return DateFormatters.from(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.parse(date));
+        return DateFormatters.from(DateFieldMapper.getDefaultDateTimeFormatter().parse(date));
     }
 
     private static String format(ZonedDateTime date, String pattern) {
@@ -1624,8 +1624,8 @@ public void testScriptCaching() throws Exception {
                 .setSettings(Settings.builder().put("requests.cache.enable", true).put("number_of_shards", 1).put("number_of_replicas", 1))
                 .get()
         );
-        String date = DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.format(date(1, 1));
-        String date2 = DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.format(date(2, 1));
+        String date = DateFieldMapper.getDefaultDateTimeFormatter().format(date(1, 1));
+        String date2 = DateFieldMapper.getDefaultDateTimeFormatter().format(date(2, 1));
         indexRandom(
             true,
             client().prepareIndex("cache_test_idx").setId("1").setSource("d", date),

diff --git a/...rnalClusterTest/java/org/opensearch/search/aggregations/bucket/DateHistogramOffsetIT.java b/...rnalClusterTest/java/org/opensearch/search/aggregations/bucket/DateHistogramOffsetIT.java
@@ -92,7 +92,7 @@ protected Settings featureFlagSettings() {
     }
 
     private ZonedDateTime date(String date) {
-        return DateFormatters.from(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.parse(date));
+        return DateFormatters.from(DateFieldMapper.getDefaultDateTimeFormatter().parse(date));
     }
 
     @Before

diff --git a/server/src/main/java/org/opensearch/common/joda/JodaDateFormatter.java b/server/src/main/java/org/opensearch/common/joda/JodaDateFormatter.java
@@ -125,6 +125,11 @@ public String pattern() {
         return pattern;
     }
 
+    @Override
+    public String printPattern() {
+        throw new UnsupportedOperationException("JodaDateFormatter does not have a print pattern");
+    }
+
     @Override
     public Locale locale() {
         return printer.getLocale();

diff --git a/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java b/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java
@@ -39,7 +39,8 @@ protected FeatureFlagSettings(
                 FeatureFlags.EXTENSIONS_SETTING,
                 FeatureFlags.IDENTITY_SETTING,
                 FeatureFlags.CONCURRENT_SEGMENT_SEARCH_SETTING,
-                FeatureFlags.TELEMETRY_SETTING
+                FeatureFlags.TELEMETRY_SETTING,
+                FeatureFlags.DATETIME_FORMATTER_CACHING_SETTING
             )
         )
     );

diff --git a/server/src/main/java/org/opensearch/common/time/DateFormatter.java b/server/src/main/java/org/opensearch/common/time/DateFormatter.java
@@ -126,6 +126,14 @@ default String formatJoda(DateTime dateTime) {
      */
     String pattern();
 
+    /**
+     * A name based format for this formatter. Can be one of the registered formatters like <code>epoch_millis</code> or
+     * a configured format like <code>HH:mm:ss</code>
+     *
+     * @return The name of this formatter
+     */
+    String printPattern();
+
     /**
      * Returns the configured locale of the date formatter
      *
@@ -147,7 +155,7 @@ default String formatJoda(DateTime dateTime) {
      */
     DateMathParser toDateMathParser();
 
-    static DateFormatter forPattern(String input) {
+    static DateFormatter forPattern(String input, String printPattern, Boolean canCacheFormatter) {
 
         if (Strings.hasLength(input) == false) {
             throw new IllegalArgumentException("No date pattern provided");
@@ -158,7 +166,28 @@ static DateFormatter forPattern(String input) {
         List<String> patterns = splitCombinedPatterns(format);
         List<DateFormatter> formatters = patterns.stream().map(DateFormatters::forPattern).collect(Collectors.toList());
 
-        return JavaDateFormatter.combined(input, formatters);
+        DateFormatter printFormatter = formatters.get(0);
+        if (Strings.hasLength(printPattern)) {
+            String printFormat = strip8Prefix(printPattern);
+            try {
+                printFormatter = DateFormatters.forPattern(printFormat);
+            } catch (IllegalArgumentException e) {
+                throw new IllegalArgumentException("Invalid print format: " + e.getMessage(), e);
+            }
+        }
+        return JavaDateFormatter.combined(input, formatters, printFormatter, canCacheFormatter);
+    }
+
+    static DateFormatter forPattern(String input) {
+        return forPattern(input, null, false);
+    }
+
+    static DateFormatter forPattern(String input, String printPattern) {
+        return forPattern(input, printPattern, false);
+    }
+
+    static DateFormatter forPattern(String input, Boolean canCacheFormatter) {
+        return forPattern(input, null, canCacheFormatter);
     }
 
     static String strip8Prefix(String input) {

diff --git a/server/src/main/java/org/opensearch/common/time/JavaDateFormatter.java b/server/src/main/java/org/opensearch/common/time/JavaDateFormatter.java
@@ -32,6 +32,7 @@
 
 package org.opensearch.common.time;
 
+import org.opensearch.common.util.FeatureFlags;
 import org.opensearch.core.common.Strings;
 
 import java.text.ParsePosition;
@@ -51,6 +52,7 @@
 import java.util.Locale;
 import java.util.Map;
 import java.util.Objects;
+import java.util.concurrent.CopyOnWriteArrayList;
 import java.util.function.BiConsumer;
 import java.util.stream.Collectors;
 
@@ -67,9 +69,11 @@ class JavaDateFormatter implements DateFormatter {
     }
 
     private final String format;
+    private final String printFormat;
     private final DateTimeFormatter printer;
     private final List<DateTimeFormatter> parsers;
     private final JavaDateFormatter roundupParser;
+    private final Boolean canCacheLastParsedFormatter;
 
     /**
      * A round up formatter
@@ -93,8 +97,18 @@ JavaDateFormatter getRoundupParser() {
     }
 
     // named formatters use default roundUpParser
+    JavaDateFormatter(
+        String format,
+        String printFormat,
+        DateTimeFormatter printer,
+        Boolean canCacheLastParsedFormatter,
+        DateTimeFormatter... parsers
+    ) {
+        this(format, printFormat, printer, ROUND_UP_BASE_FIELDS, canCacheLastParsedFormatter, parsers);
+    }
+
     JavaDateFormatter(String format, DateTimeFormatter printer, DateTimeFormatter... parsers) {
-        this(format, printer, ROUND_UP_BASE_FIELDS, parsers);
+        this(format, format, printer, false, parsers);
     }
 
     private static final BiConsumer<DateTimeFormatterBuilder, DateTimeFormatter> ROUND_UP_BASE_FIELDS = (builder, parser) -> {
@@ -111,8 +125,10 @@ JavaDateFormatter getRoundupParser() {
     // subclasses override roundUpParser
     JavaDateFormatter(
         String format,
+        String printFormat,
         DateTimeFormatter printer,
         BiConsumer<DateTimeFormatterBuilder, DateTimeFormatter> roundupParserConsumer,
+        Boolean canCacheLastParsedFormatter,
         DateTimeFormatter... parsers
     ) {
         if (printer == null) {
@@ -128,16 +144,27 @@ JavaDateFormatter getRoundupParser() {
         }
         this.printer = printer;
         this.format = format;
+        this.printFormat = printFormat;
+        this.canCacheLastParsedFormatter = canCacheLastParsedFormatter;
 
         if (parsers.length == 0) {
             this.parsers = Collections.singletonList(printer);
         } else {
-            this.parsers = Arrays.asList(parsers);
+            this.parsers = new CopyOnWriteArrayList<>(parsers);
         }
         List<DateTimeFormatter> roundUp = createRoundUpParser(format, roundupParserConsumer);
         this.roundupParser = new RoundUpFormatter(format, roundUp);
     }
 
+    JavaDateFormatter(
+        String format,
+        DateTimeFormatter printer,
+        BiConsumer<DateTimeFormatterBuilder, DateTimeFormatter> roundupParserConsumer,
+        DateTimeFormatter... parsers
+    ) {
+        this(format, format, printer, roundupParserConsumer, false, parsers);
+    }
+
     /**
      * This is when the RoundUp Formatters are created. In further merges (with ||) it will only append them to a list.
      * || is not expected to be provided as format when a RoundUp formatter is created. It will be splitted before in
@@ -164,36 +191,61 @@ private List<DateTimeFormatter> createRoundUpParser(
         return null;
     }
 
-    public static DateFormatter combined(String input, List<DateFormatter> formatters) {
+    public static DateFormatter combined(
+        String input,
+        List<DateFormatter> formatters,
+        DateFormatter printFormatter,
+        Boolean canCacheLastParsedFormatter
+    ) {
         assert formatters.size() > 0;
+        assert printFormatter != null;
 
         List<DateTimeFormatter> parsers = new ArrayList<>(formatters.size());
         List<DateTimeFormatter> roundUpParsers = new ArrayList<>(formatters.size());
 
-        DateTimeFormatter printer = null;
+        assert printFormatter instanceof JavaDateFormatter;
+        JavaDateFormatter javaPrintFormatter = (JavaDateFormatter) printFormatter;
+        DateTimeFormatter printer = javaPrintFormatter.getPrinter();
         for (DateFormatter formatter : formatters) {
             assert formatter instanceof JavaDateFormatter;
             JavaDateFormatter javaDateFormatter = (JavaDateFormatter) formatter;
-            if (printer == null) {
-                printer = javaDateFormatter.getPrinter();
-            }
             parsers.addAll(javaDateFormatter.getParsers());
             roundUpParsers.addAll(javaDateFormatter.getRoundupParser().getParsers());
         }
 
-        return new JavaDateFormatter(input, printer, roundUpParsers, parsers);
+        return new JavaDateFormatter(
+            input,
+            javaPrintFormatter.format,
+            printer,
+            roundUpParsers,
+            parsers,
+            canCacheLastParsedFormatter & FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING_SETTING)
+        ); // check if caching is enabled
     }
 
     private JavaDateFormatter(
         String format,
+        String printFormat,
         DateTimeFormatter printer,
         List<DateTimeFormatter> roundUpParsers,
-        List<DateTimeFormatter> parsers
+        List<DateTimeFormatter> parsers,
+        Boolean canCacheLastParsedFormatter
     ) {
         this.format = format;
+        this.printFormat = printFormat;
         this.printer = printer;
         this.roundupParser = roundUpParsers != null ? new RoundUpFormatter(format, roundUpParsers) : null;
-        this.parsers = parsers;
+        this.parsers = new CopyOnWriteArrayList<>(parsers);
+        this.canCacheLastParsedFormatter = canCacheLastParsedFormatter;
+    }
+
+    private JavaDateFormatter(
+        String format,
+        DateTimeFormatter printer,
+        List<DateTimeFormatter> roundUpParsers,
+        List<DateTimeFormatter> parsers
+    ) {
+        this(format, format, printer, roundUpParsers, parsers, false);
     }
 
     JavaDateFormatter getRoundupParser() {
@@ -233,12 +285,24 @@ public TemporalAccessor parse(String input) {
      */
     private TemporalAccessor doParse(String input) {
         if (parsers.size() > 1) {
+            Object object = null;
+            DateTimeFormatter lastParsedformatter = null;
             for (DateTimeFormatter formatter : parsers) {
                 ParsePosition pos = new ParsePosition(0);
-                Object object = formatter.toFormat().parseObject(input, pos);
+                object = formatter.toFormat().parseObject(input, pos);
                 if (parsingSucceeded(object, input, pos)) {
-                    return (TemporalAccessor) object;
+                    lastParsedformatter = formatter;
+                    break;
+                }
+            }
+            if (lastParsedformatter != null) {
+                if (canCacheLastParsedFormatter && lastParsedformatter != parsers.get(0)) {
+                    synchronized (parsers) {
+                        parsers.remove(lastParsedformatter);
+                        parsers.add(0, lastParsedformatter);
+                    }
                 }
+                return (TemporalAccessor) object;
             }
             throw new DateTimeParseException("Failed to parse with all enclosed parsers", input, 0);
         }
@@ -255,12 +319,14 @@ public DateFormatter withZone(ZoneId zoneId) {
         if (zoneId.equals(zone())) {
             return this;
         }
-        List<DateTimeFormatter> parsers = this.parsers.stream().map(p -> p.withZone(zoneId)).collect(Collectors.toList());
+        List<DateTimeFormatter> parsers = new CopyOnWriteArrayList<>(
+            this.parsers.stream().map(p -> p.withZone(zoneId)).collect(Collectors.toList())
+        );
         List<DateTimeFormatter> roundUpParsers = this.roundupParser.getParsers()
             .stream()
             .map(p -> p.withZone(zoneId))
             .collect(Collectors.toList());
-        return new JavaDateFormatter(format, printer.withZone(zoneId), roundUpParsers, parsers);
+        return new JavaDateFormatter(format, printFormat, printer.withZone(zoneId), roundUpParsers, parsers, canCacheLastParsedFormatter);
     }
 
     @Override
@@ -269,12 +335,14 @@ public DateFormatter withLocale(Locale locale) {
         if (locale.equals(locale())) {
             return this;
         }
-        List<DateTimeFormatter> parsers = this.parsers.stream().map(p -> p.withLocale(locale)).collect(Collectors.toList());
+        List<DateTimeFormatter> parsers = new CopyOnWriteArrayList<>(
+            this.parsers.stream().map(p -> p.withLocale(locale)).collect(Collectors.toList())
+        );
         List<DateTimeFormatter> roundUpParsers = this.roundupParser.getParsers()
             .stream()
             .map(p -> p.withLocale(locale))
             .collect(Collectors.toList());
-        return new JavaDateFormatter(format, printer.withLocale(locale), roundUpParsers, parsers);
+        return new JavaDateFormatter(format, printFormat, printer.withLocale(locale), roundUpParsers, parsers, canCacheLastParsedFormatter);
     }
 
     @Override
@@ -287,6 +355,11 @@ public String pattern() {
         return format;
     }
 
+    @Override
+    public String printPattern() {
+        return printFormat;
+    }
+
     @Override
     public Locale locale() {
         return this.printer.getLocale();

diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java
@@ -55,6 +55,11 @@ public class FeatureFlags {
      */
     public static final String TELEMETRY = "opensearch.experimental.feature.telemetry.enabled";
 
+    /**
+     * Gates the optimization of datetime formatters caching along with change in default datetime formatter.
+     */
+    public static final String DATETIME_FORMATTER_CACHING = "opensearch.experimental.optimization.datetime_formatter_caching.enabled";
+
     /**
      * Should store the settings from opensearch.yml.
      */
@@ -83,6 +88,17 @@ public static boolean isEnabled(String featureFlagName) {
         return settings != null && settings.getAsBoolean(featureFlagName, false);
     }
 
+    public static boolean isEnabled(Setting<Boolean> featureFlag) {
+        if ("true".equalsIgnoreCase(System.getProperty(featureFlag.getKey()))) {
+            // TODO: Remove the if condition once FeatureFlags are only supported via opensearch.yml
+            return true;
+        } else if (settings != null) {
+            return featureFlag.get(settings);
+        } else {
+            return featureFlag.getDefault(Settings.EMPTY);
+        }
+    }
+
     public static final Setting<Boolean> SEGMENT_REPLICATION_EXPERIMENTAL_SETTING = Setting.boolSetting(
         SEGMENT_REPLICATION_EXPERIMENTAL,
         false,
@@ -100,4 +116,10 @@ public static boolean isEnabled(String featureFlagName) {
         false,
         Property.NodeScope
     );
+
+    public static final Setting<Boolean> DATETIME_FORMATTER_CACHING_SETTING = Setting.boolSetting(
+        DATETIME_FORMATTER_CACHING,
+        true,
+        Property.NodeScope
+    );
 }