This is an automated email from the ASF dual-hosted git repository. ddekany pushed a commit to branch FREEMARKER-35 in repository https://gitbox.apache.org/repos/asf/freemarker.git
commit 0a233022d031e76605cb6ff195d2f2d54f868022 Author: ddekany <[email protected]> AuthorDate: Fri Aug 26 02:04:24 2022 +0200 Continued reworked factory-level caches: Fixing/cleaning previous commit in JavaTemplateDateFormatFactory. Factored out caching logic into its own class. Using that in JavaTemplateNumberFormatFactory too now. --- .../java/freemarker/core/FastLRUKeyValueStore.java | 85 ++++++++++++++++++++++ .../core/JavaTemplateDateFormatFactory.java | 55 ++------------ .../core/JavaTemplateNumberFormatFactory.java | 79 +++++++------------- src/manual/en_US/book.xml | 9 ++- 4 files changed, 124 insertions(+), 104 deletions(-) diff --git a/src/main/java/freemarker/core/FastLRUKeyValueStore.java b/src/main/java/freemarker/core/FastLRUKeyValueStore.java new file mode 100644 index 00000000..db774162 --- /dev/null +++ b/src/main/java/freemarker/core/FastLRUKeyValueStore.java @@ -0,0 +1,85 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package freemarker.core; + +import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; + +/** + * A thread-safe {@link Map}-like object, with get/put-like operations, that implements a rough, but fast + * least-recently-used (LRU) logic to remove old entries automatically, in order to keep the size under a specified + * maximum. It will remember at least the last {@link #guaranteedRecentEntries} accessed entries. Removing entries is + * only guaranteed when the size exceeds the double of {@link #guaranteedRecentEntries}. Actually, if the methods are + * accessed from N threads concurrently, there's a chance to end up with N-1 more remembered entries, though in + * practical applications this is very unlikely to happen. That's also precision given up for speed. + * + * @since 2.3.32 + */ +class FastLRUKeyValueStore<K, V> { + private final int guaranteedRecentEntries; + + private final ConcurrentHashMap<K, V> recentEntries = new ConcurrentHashMap<>(); + private final ConcurrentHashMap<K, V> olderEntries = new ConcurrentHashMap<>(); + + /** + * @param guaranteedRecentEntries + * The number of least recently accessed ("get", or "put") entries that we are guaranteed to remember. The + * actual size can grow bigger than that; see in class documentation. + */ + public FastLRUKeyValueStore(int guaranteedRecentEntries) { + this.guaranteedRecentEntries = guaranteedRecentEntries; + } + + /** + * @return If the value was already in the cache, then it's not replaced, and the old value is returned, otherwise + * the new (argument) value is returned. + */ + V putIfAbsentThenReturnStored(K key, V value) { + if (recentEntries.size() >= guaranteedRecentEntries) { + synchronized (this) { + if (recentEntries.size() >= guaranteedRecentEntries) { + olderEntries.clear(); + olderEntries.putAll(recentEntries); + recentEntries.clear(); + } + } + } + + V prevValue = recentEntries.putIfAbsent(key, value); + return prevValue != null ? prevValue : value; + } + + /** + * @return {@code null} if there's no entry with the given key. + */ + V get(K cacheKey) { + V value = recentEntries.get(cacheKey); + if (value != null) { + return value; + } + + value = olderEntries.remove(cacheKey); + if (value == null) { + return null; + } + + return putIfAbsentThenReturnStored(cacheKey, value); + } +} diff --git a/src/main/java/freemarker/core/JavaTemplateDateFormatFactory.java b/src/main/java/freemarker/core/JavaTemplateDateFormatFactory.java index 62863c2e..f4773ac3 100644 --- a/src/main/java/freemarker/core/JavaTemplateDateFormatFactory.java +++ b/src/main/java/freemarker/core/JavaTemplateDateFormatFactory.java @@ -24,7 +24,6 @@ import java.text.SimpleDateFormat; import java.util.Locale; import java.util.StringTokenizer; import java.util.TimeZone; -import java.util.concurrent.ConcurrentHashMap; import freemarker.log.Logger; import freemarker.template.TemplateDateModel; @@ -35,10 +34,7 @@ class JavaTemplateDateFormatFactory extends TemplateDateFormatFactory { private static final Logger LOG = Logger.getLogger("freemarker.runtime"); - private static final int MAX_CACHE_SIZE = 2; //!!T 512 - - private final ConcurrentHashMap<CacheKey, DateFormat> cache = new ConcurrentHashMap<>(); - private final ConcurrentHashMap<CacheKey, DateFormat> cacheRecallableEntries = new ConcurrentHashMap<>(); + private final FastLRUKeyValueStore<CacheKey, DateFormat> dateFormatCache = new FastLRUKeyValueStore<>(512); private JavaTemplateDateFormatFactory() { // Can't be instantiated @@ -51,41 +47,16 @@ class JavaTemplateDateFormatFactory extends TemplateDateFormatFactory { @Override public TemplateDateFormat get(String params, int dateType, Locale locale, TimeZone timeZone, boolean zonelessInput, Environment env) throws UnknownDateTypeFormattingUnsupportedException, InvalidFormatParametersException { - return new JavaTemplateDateFormat(getJavaDateFormat(dateType, params, locale, timeZone)); - } - - /** - * Returns a "private" copy (not in the global cache) for the given format. - */ - private DateFormat getJavaDateFormat(int dateType, String nameOrPattern, Locale locale, TimeZone timeZone) - throws UnknownDateTypeFormattingUnsupportedException, InvalidFormatParametersException { - - // Get DateFormat from global cache: - CacheKey cacheKey = new CacheKey(dateType, nameOrPattern, locale, timeZone); + CacheKey cacheKey = new CacheKey(dateType, params, locale, timeZone); - DateFormat dateFormat = getFromCache(cacheKey); + DateFormat dateFormat = dateFormatCache.get(cacheKey); if (dateFormat == null) { - dateFormat = getJavaDateFormatNoCache(dateType, nameOrPattern, cacheKey); - dateFormat = addToCacheWithLimitingSize(cacheKey, dateFormat); + dateFormat = getJavaDateFormatNoCache(dateType, params, cacheKey); + dateFormat = dateFormatCache.putIfAbsentThenReturnStored(cacheKey, dateFormat); } // Must clone, as SimpleDateFormat is not thread safe, not even if you don't call setters on it: - return (DateFormat) dateFormat.clone(); - } - - private DateFormat addToCacheWithLimitingSize(CacheKey cacheKey, DateFormat dateFormat) { - if (cache.size() >= MAX_CACHE_SIZE) { - synchronized (JavaTemplateDateFormatFactory.class) { - if (cache.size() >= MAX_CACHE_SIZE) { - cacheRecallableEntries.clear(); - cacheRecallableEntries.putAll(cache); - cache.clear(); - } - } - } - - DateFormat prevDateFormat = cache.putIfAbsent(cacheKey, dateFormat); - return prevDateFormat != null ? prevDateFormat : dateFormat; + return new JavaTemplateDateFormat((DateFormat) dateFormat.clone()); } private DateFormat getJavaDateFormatNoCache(int dateType, String nameOrPattern, CacheKey cacheKey) throws @@ -130,20 +101,6 @@ class JavaTemplateDateFormatFactory extends TemplateDateFormatFactory { } } - private DateFormat getFromCache(CacheKey cacheKey) { - DateFormat dateFormat = cache.get(cacheKey); - if (dateFormat != null) { - return dateFormat; - } - - dateFormat = cacheRecallableEntries.remove(cacheKey); - if (dateFormat == null) { - return null; - } - - return addToCacheWithLimitingSize(cacheKey, dateFormat); - } - private static final class CacheKey { private final int dateType; private final String pattern; diff --git a/src/main/java/freemarker/core/JavaTemplateNumberFormatFactory.java b/src/main/java/freemarker/core/JavaTemplateNumberFormatFactory.java index 8e8a3279..a3de36a3 100644 --- a/src/main/java/freemarker/core/JavaTemplateNumberFormatFactory.java +++ b/src/main/java/freemarker/core/JavaTemplateNumberFormatFactory.java @@ -21,9 +21,6 @@ package freemarker.core; import java.text.NumberFormat; import java.text.ParseException; import java.util.Locale; -import java.util.concurrent.ConcurrentHashMap; - -import freemarker.log.Logger; /** * Deals with {@link TemplateNumberFormat}-s that just wrap a Java {@link NumberFormat}. @@ -34,11 +31,7 @@ class JavaTemplateNumberFormatFactory extends TemplateNumberFormatFactory { static final String COMPUTER = "computer"; - private static final Logger LOG = Logger.getLogger("freemarker.runtime"); - - private static final ConcurrentHashMap<CacheKey, NumberFormat> GLOBAL_FORMAT_CACHE - = new ConcurrentHashMap<>(); - private static final int LEAK_ALERT_NUMBER_FORMAT_CACHE_SIZE = 1024; + private final FastLRUKeyValueStore<CacheKey, NumberFormat> numberFormatCache = new FastLRUKeyValueStore<>(512); private JavaTemplateNumberFormatFactory() { // Not meant to be instantiated @@ -50,51 +43,35 @@ class JavaTemplateNumberFormatFactory extends TemplateNumberFormatFactory { CacheKey cacheKey = new CacheKey( env != null ? env.transformNumberFormatGlobalCacheKey(params) : params, locale); - NumberFormat jFormat = GLOBAL_FORMAT_CACHE.get(cacheKey); - if (jFormat == null) { - if ("number".equals(params)) { - jFormat = NumberFormat.getNumberInstance(locale); - } else if ("currency".equals(params)) { - jFormat = NumberFormat.getCurrencyInstance(locale); - } else if ("percent".equals(params)) { - jFormat = NumberFormat.getPercentInstance(locale); - } else if (COMPUTER.equals(params)) { - jFormat = env.getCNumberFormat(); - } else { - try { - jFormat = ExtendedDecimalFormatParser.parse(params, locale); - } catch (ParseException e) { - String msg = e.getMessage(); - throw new InvalidFormatParametersException( - msg != null ? msg : "Invalid DecimalFormat pattern", e); - } - } - if (GLOBAL_FORMAT_CACHE.size() >= LEAK_ALERT_NUMBER_FORMAT_CACHE_SIZE) { - boolean triggered = false; - synchronized (JavaTemplateNumberFormatFactory.class) { - if (GLOBAL_FORMAT_CACHE.size() >= LEAK_ALERT_NUMBER_FORMAT_CACHE_SIZE) { - triggered = true; - GLOBAL_FORMAT_CACHE.clear(); - } - } - if (triggered) { - LOG.warn("Global Java NumberFormat cache has exceeded " + LEAK_ALERT_NUMBER_FORMAT_CACHE_SIZE - + " entries => cache flushed. " - + "Typical cause: Some template generates high variety of format pattern strings."); - } - } - - NumberFormat prevJFormat = GLOBAL_FORMAT_CACHE.putIfAbsent(cacheKey, jFormat); - if (prevJFormat != null) { - jFormat = prevJFormat; - } - } // if cache miss - - // JFormat-s aren't thread-safe; must clone it - jFormat = (NumberFormat) jFormat.clone(); + NumberFormat numberFormat = numberFormatCache.get(cacheKey); + if (numberFormat == null) { + numberFormat = getNumberFormatNoCache(params, locale, env); + numberFormat = numberFormatCache.putIfAbsentThenReturnStored(cacheKey, numberFormat); + } - return new JavaTemplateNumberFormat(jFormat, params); + return new JavaTemplateNumberFormat((NumberFormat) numberFormat.clone(), params); + } + + private NumberFormat getNumberFormatNoCache(String params, Locale locale, Environment env) throws + InvalidFormatParametersException { + if ("number".equals(params)) { + return NumberFormat.getNumberInstance(locale); + } else if ("currency".equals(params)) { + return NumberFormat.getCurrencyInstance(locale); + } else if ("percent".equals(params)) { + return NumberFormat.getPercentInstance(locale); + } else if (COMPUTER.equals(params)) { + return env.getCNumberFormat(); + } else { + try { + return ExtendedDecimalFormatParser.parse(params, locale); + } catch (ParseException e) { + String msg = e.getMessage(); + throw new InvalidFormatParametersException( + msg != null ? msg : "Invalid DecimalFormat pattern", e); + } + } } private static final class CacheKey { diff --git a/src/manual/en_US/book.xml b/src/manual/en_US/book.xml index d8264608..d6fc9270 100644 --- a/src/manual/en_US/book.xml +++ b/src/manual/en_US/book.xml @@ -29515,10 +29515,11 @@ TemplateModel x = env.getVariable("x"); // get variable x</programlisting> </listitem> <listitem> - <para>Some internal cleanup and slight improves related to - global caching of <literal>SimpleDateFormat</literal> patterns - (in - <literal>freemarker.core.JavaTemplateDateFormatFactory</literal>).</para> + <para>Some internal cleanup, and slightly improved global + caching of <literal>SimpleDateFormat</literal>, and + <literal>DecimalFormat</literal> patterns (in + <literal>freemarker.core.JavaTemplateDateFormatFactory</literal>, + and <literal>JavaTemplateNumberFormatFactory</literal>).</para> </listitem> </itemizedlist> </section>
