From b2901c77bc96a6c94d106be84fdb9ea39463e726 Mon Sep 17 00:00:00 2001 From: Prabhat Sharma Date: Mon, 28 Aug 2023 08:08:56 +0530 Subject: [PATCH] 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 --- CHANGELOG.md | 1 + distribution/src/config/opensearch.yml | 6 +++ .../common/settings/FeatureFlagSettings.java | 3 +- .../opensearch/common/time/DateFormatter.java | 8 +++- .../common/time/JavaDateFormatter.java | 45 ++++++++++++++----- .../opensearch/common/util/FeatureFlags.java | 11 +++++ .../index/mapper/DateFieldMapper.java | 7 ++- 7 files changed, 66 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 96af8741a0de9..1409abab83cce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -158,6 +158,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Removing the vec file extension from INDEX_STORE_HYBRID_NIO_EXTENSIONS, to ensure the no performance degradation for vector search via Lucene Engine.([#9528](https://github.com/opensearch-project/OpenSearch/pull/9528))) - Separate request-based and settings-based concurrent segment search controls and introduce AggregatorFactory method to determine concurrent search support ([#9469](https://github.com/opensearch-project/OpenSearch/pull/9469)) - [Remote Store] Rate limiter integration for remote store uploads and downloads([#9448](https://github.com/opensearch-project/OpenSearch/pull/9448/)) +- 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 index 3c4fe822005e0..77e6b13fd931a 100644 --- a/distribution/src/config/opensearch.yml +++ b/distribution/src/config/opensearch.yml @@ -129,3 +129,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/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java b/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java index e109d2a871cef..73aa30b5e942c 100644 --- a/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java +++ b/server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java @@ -40,7 +40,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 index d57fd441b9bf4..916012e963569 100644 --- a/server/src/main/java/org/opensearch/common/time/DateFormatter.java +++ b/server/src/main/java/org/opensearch/common/time/DateFormatter.java @@ -147,7 +147,7 @@ default String formatJoda(DateTime dateTime) { */ DateMathParser toDateMathParser(); - static DateFormatter forPattern(String input) { + static DateFormatter forPattern(String input, Boolean canCacheFormatter) { if (Strings.hasLength(input) == false) { throw new IllegalArgumentException("No date pattern provided"); @@ -158,7 +158,11 @@ static DateFormatter forPattern(String input) { List patterns = splitCombinedPatterns(format); List formatters = patterns.stream().map(DateFormatters::forPattern).collect(Collectors.toList()); - return JavaDateFormatter.combined(input, formatters); + return JavaDateFormatter.combined(input, formatters, canCacheFormatter); + } + + static DateFormatter forPattern(String input) { + return forPattern(input, false); } 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 index 07013a3dc75f2..cb8b3e924b985 100644 --- 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; @@ -70,6 +71,7 @@ class JavaDateFormatter implements DateFormatter { private final DateTimeFormatter printer; private final List parsers; private final JavaDateFormatter roundupParser; + private final Boolean canCacheLastParsedFormatter; /** * A round up formatter @@ -78,8 +80,8 @@ class JavaDateFormatter implements DateFormatter { */ static class RoundUpFormatter extends JavaDateFormatter { - RoundUpFormatter(String format, List roundUpParsers) { - super(format, firstFrom(roundUpParsers), null, roundUpParsers); + RoundUpFormatter(String format, List roundUpParsers, Boolean canCacheLastParsedFormatter) { + super(format, firstFrom(roundUpParsers), null, roundUpParsers, canCacheLastParsedFormatter); } private static DateTimeFormatter firstFrom(List roundUpParsers) { @@ -93,8 +95,12 @@ JavaDateFormatter getRoundupParser() { } // named formatters use default roundUpParser + JavaDateFormatter(String format, DateTimeFormatter printer, Boolean canCacheLastParsedFormatter, DateTimeFormatter... parsers) { + this(format, printer, ROUND_UP_BASE_FIELDS, canCacheLastParsedFormatter, parsers); + } + JavaDateFormatter(String format, DateTimeFormatter printer, DateTimeFormatter... parsers) { - this(format, printer, ROUND_UP_BASE_FIELDS, parsers); + this(format, printer, false, parsers); } private static final BiConsumer ROUND_UP_BASE_FIELDS = (builder, parser) -> { @@ -113,6 +119,7 @@ JavaDateFormatter getRoundupParser() { String format, DateTimeFormatter printer, BiConsumer roundupParserConsumer, + Boolean canCacheLastParsedFormatter, DateTimeFormatter... parsers ) { if (printer == null) { @@ -128,6 +135,7 @@ JavaDateFormatter getRoundupParser() { } this.printer = printer; this.format = format; + this.canCacheLastParsedFormatter = canCacheLastParsedFormatter; if (parsers.length == 0) { this.parsers = Collections.singletonList(printer); @@ -135,7 +143,16 @@ JavaDateFormatter getRoundupParser() { this.parsers = Arrays.asList(parsers); } List roundUp = createRoundUpParser(format, roundupParserConsumer); - this.roundupParser = new RoundUpFormatter(format, roundUp); + this.roundupParser = new RoundUpFormatter(format, roundUp, canCacheLastParsedFormatter); + } + + JavaDateFormatter( + String format, + DateTimeFormatter printer, + BiConsumer roundupParserConsumer, + DateTimeFormatter... parsers + ) { + this(format, printer, roundupParserConsumer, false, parsers); } /** @@ -164,7 +181,7 @@ private List createRoundUpParser( return null; } - public static DateFormatter combined(String input, List formatters) { + public static DateFormatter combined(String input, List formatters, Boolean canCacheLastParsedFormatter) { assert formatters.size() > 0; List parsers = new ArrayList<>(formatters.size()); @@ -181,19 +198,21 @@ public static DateFormatter combined(String input, List formatter roundUpParsers.addAll(javaDateFormatter.getRoundupParser().getParsers()); } - return new JavaDateFormatter(input, printer, roundUpParsers, parsers); + return new JavaDateFormatter(input, printer, roundUpParsers, parsers, canCacheLastParsedFormatter); } private JavaDateFormatter( String format, DateTimeFormatter printer, List roundUpParsers, - List parsers + List parsers, + Boolean canCacheLastParsedFormatter ) { this.format = format; this.printer = printer; - this.roundupParser = roundUpParsers != null ? new RoundUpFormatter(format, roundUpParsers) : null; + this.roundupParser = roundUpParsers != null ? new RoundUpFormatter(format, roundUpParsers, canCacheLastParsedFormatter) : null; this.parsers = parsers; + this.canCacheLastParsedFormatter = canCacheLastParsedFormatter; } JavaDateFormatter getRoundupParser() { @@ -237,6 +256,12 @@ private TemporalAccessor doParse(String input) { ParsePosition pos = new ParsePosition(0); Object object = formatter.toFormat().parseObject(input, pos); if (parsingSucceeded(object, input, pos)) { + if (FeatureFlags.isEnabled(FeatureFlags.DATETIME_FORMATTER_CACHING) + && canCacheLastParsedFormatter + && formatter != parsers.get(0)) { + parsers.remove(formatter); + parsers.add(0, formatter); + } return (TemporalAccessor) object; } } @@ -260,7 +285,7 @@ public DateFormatter withZone(ZoneId zoneId) { .stream() .map(p -> p.withZone(zoneId)) .collect(Collectors.toList()); - return new JavaDateFormatter(format, printer.withZone(zoneId), roundUpParsers, parsers); + return new JavaDateFormatter(format, printer.withZone(zoneId), roundUpParsers, parsers, this.canCacheLastParsedFormatter); } @Override @@ -274,7 +299,7 @@ public DateFormatter withLocale(Locale locale) { .stream() .map(p -> p.withLocale(locale)) .collect(Collectors.toList()); - return new JavaDateFormatter(format, printer.withLocale(locale), roundUpParsers, parsers); + return new JavaDateFormatter(format, printer.withLocale(locale), roundUpParsers, parsers, this.canCacheLastParsedFormatter); } @Override diff --git a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java index e2663b56c5cca..037b31f1cafc0 100644 --- a/server/src/main/java/org/opensearch/common/util/FeatureFlags.java +++ b/server/src/main/java/org/opensearch/common/util/FeatureFlags.java @@ -62,6 +62,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. */ @@ -109,4 +114,10 @@ public static boolean isEnabled(String featureFlagName) { false, Property.NodeScope ); + + public static final Setting DATETIME_FORMATTER_CACHING_SETTING = Setting.boolSetting( + DATETIME_FORMATTER_CACHING, + false, + Property.NodeScope + ); } diff --git a/server/src/main/java/org/opensearch/index/mapper/DateFieldMapper.java b/server/src/main/java/org/opensearch/index/mapper/DateFieldMapper.java index 393ddf2dd1de0..5ea5c9c319e8a 100644 --- a/server/src/main/java/org/opensearch/index/mapper/DateFieldMapper.java +++ b/server/src/main/java/org/opensearch/index/mapper/DateFieldMapper.java @@ -91,7 +91,10 @@ public final class DateFieldMapper extends ParametrizedFieldMapper { public static final String CONTENT_TYPE = "date"; public static final String DATE_NANOS_CONTENT_TYPE = "date_nanos"; - public static final DateFormatter DEFAULT_DATE_TIME_FORMATTER = DateFormatter.forPattern("strict_date_optional_time||epoch_millis"); + public static final DateFormatter DEFAULT_DATE_TIME_FORMATTER = DateFormatter.forPattern( + "strict_date_time_no_millis||strict_date_optional_time||epoch_millis", + true + ); /** * Resolution of the date time @@ -259,7 +262,7 @@ public Builder( private DateFormatter buildFormatter() { try { - return DateFormatter.forPattern(format.getValue()).withLocale(locale.getValue()); + return DateFormatter.forPattern(format.getValue(), !format.isConfigured()).withLocale(locale.getValue()); } catch (IllegalArgumentException e) { throw new IllegalArgumentException("Error parsing [format] on field [" + name() + "]: " + e.getMessage(), e); }