From e4b6b3956ed4f7e1404e4523c6629216b2384476 Mon Sep 17 00:00:00 2001 From: Henri Biestro Date: Fri, 17 Nov 2023 20:22:17 -0800 Subject: [PATCH] JEXL-414: added cache interface, synchronized & concurrent implementations and factory handling; --- RELEASE-NOTES.txt | 1 + pom.xml | 5 + src/changes/changes.xml | 5 +- .../org/apache/commons/jexl3/JexlBuilder.java | 24 +++- .../org/apache/commons/jexl3/JexlCache.java | 85 +++++++++++++ .../jexl3/internal/ConcurrentCache.java | 58 +++++++++ .../apache/commons/jexl3/internal/Engine.java | 12 +- .../commons/jexl3/internal/SoftCache.java | 118 ++++++++++-------- .../jexl3/internal/TemplateEngine.java | 5 +- .../org/apache/commons/jexl3/CacheTest.java | 15 ++- 10 files changed, 264 insertions(+), 64 deletions(-) create mode 100644 src/main/java/org/apache/commons/jexl3/JexlCache.java create mode 100644 src/main/java/org/apache/commons/jexl3/internal/ConcurrentCache.java diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index 1dfb7d879..1d173c6f3 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -37,6 +37,7 @@ New Features in 3.3.1: Bugs Fixed in 3.3.1: =================== +* JEXL-415: Incorrect template eval result * JEXL-412: Ambiguous syntax between namespace function call and map object definition. * JEXL-410: JexlFeatures: ctor does not enable all features * JEXL-409: Disable LEXICAL should disable LEXICAL_SHADE diff --git a/pom.xml b/pom.xml index c5a534dde..04f3cf3b8 100644 --- a/pom.xml +++ b/pom.xml @@ -122,6 +122,11 @@ 2.10.1 test + + com.googlecode.concurrentlinkedhashmap + concurrentlinkedhashmap-lru + 1.4.2 + diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 9d3c4c38e..80fc9107b 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -42,9 +42,12 @@ Allow 'trailing commas' or ellipsis while defining array, map and set literals + + Incorrect template eval result. + Ambiguous syntax between namespace function call and map object definition. - action> + JexlFeatures: ctor does not enable all features diff --git a/src/main/java/org/apache/commons/jexl3/JexlBuilder.java b/src/main/java/org/apache/commons/jexl3/JexlBuilder.java index 01656fefa..b67640d75 100644 --- a/src/main/java/org/apache/commons/jexl3/JexlBuilder.java +++ b/src/main/java/org/apache/commons/jexl3/JexlBuilder.java @@ -21,6 +21,7 @@ import java.util.Arrays; import java.util.Collection; import java.util.Map; +import java.util.function.IntFunction; import org.apache.commons.jexl3.internal.Engine; import org.apache.commons.jexl3.introspection.JexlPermissions; @@ -132,6 +133,9 @@ public static void setDefaultPermissions(final JexlPermissions permissions) { /** The cache size. */ private int cache = -1; + /** The cache class factory. */ + private IntFunction> cacheFactory = JexlCache.createConcurrent(); + /** The stack overflow limit. */ private int stackOverflow = Integer.MAX_VALUE; @@ -615,11 +619,29 @@ public JexlBuilder cache(final int size) { return this; } + /** + * Sets the expression cache size the engine will use. + * + * @param factory the function to produce a cache. + * @return this builder + */ + public JexlBuilder cacheFactory(final IntFunction> factory) { + this.cacheFactory = factory; + return this; + } + /** * @return the cache size */ public int cache() { - return cache; + return cache; + } + + /** + * @return the cache factory + */ + public IntFunction> cacheFactory() { + return this.cacheFactory; } /** diff --git a/src/main/java/org/apache/commons/jexl3/JexlCache.java b/src/main/java/org/apache/commons/jexl3/JexlCache.java new file mode 100644 index 000000000..9bd9a7343 --- /dev/null +++ b/src/main/java/org/apache/commons/jexl3/JexlCache.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 org.apache.commons.jexl3; + +import org.apache.commons.jexl3.internal.ConcurrentCache; +import org.apache.commons.jexl3.internal.SoftCache; + +import java.util.Collection; +import java.util.Collections; +import java.util.Map; +import java.util.function.IntFunction; + +/** + * Caching scripts or templates interface. + * @param source + * @param script or template + */ +public interface JexlCache { + /** + * Returns the cache size. + * + * @return the cache size + */ + int size(); + + /** + * Clears the cache. + */ + void clear(); + + /** + * Gets a value from cache. + * + * @param key the cache entry key + * @return the cache entry value + */ + V get(K key); + + /** + * Puts a value in cache. + * + * @param key the cache entry key + * @param script the cache entry value + */ + V put(K key, V script); + + /** + * Produces the cache entry set. + *

+ * For implementations testing only + *

+ * @return the cache entry list + */ + default Collection> entries() { + return Collections.emptyList(); + } + + /** + * @return a synchronized cache factory amenable to low concurrency usage + */ + static IntFunction> createSynchronized() { + return SoftCache::new; + } + + /** + * @return a concurrent cache factory amenable to high concurrency usage + */ + static IntFunction> createConcurrent() { + return ConcurrentCache::new; + } +} diff --git a/src/main/java/org/apache/commons/jexl3/internal/ConcurrentCache.java b/src/main/java/org/apache/commons/jexl3/internal/ConcurrentCache.java new file mode 100644 index 000000000..03a391663 --- /dev/null +++ b/src/main/java/org/apache/commons/jexl3/internal/ConcurrentCache.java @@ -0,0 +1,58 @@ +/* + * 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 org.apache.commons.jexl3.internal; + +import com.googlecode.concurrentlinkedhashmap.ConcurrentLinkedHashMap; + +import java.lang.ref.SoftReference; +import java.util.Collection; +import java.util.Collections; +import java.util.Map; + +/** + * A cache whose underlying map is a ConcurrentLinkedHashMap. + * + * @param the cache key entry type + * @param the cache key value type + */ +public class ConcurrentCache extends SoftCache { + /** + * Creates a new instance of a concurrent cache. + * + * @param theSize the cache size + */ + public ConcurrentCache(final int theSize) { + super(theSize); + } + + @Override + public Collection> entries() { + final SoftReference> ref = reference; + final Map map = ref != null ? ref.get() : null; + return map == null? Collections.emptyList() : map.entrySet(); + } + + @Override + public Map createMap(final int cacheSize) { + return new ConcurrentLinkedHashMap.Builder() + .concurrencyLevel(Runtime.getRuntime().availableProcessors()) + .maximumWeightedCapacity(cacheSize) + .build(); + } +} + + diff --git a/src/main/java/org/apache/commons/jexl3/internal/Engine.java b/src/main/java/org/apache/commons/jexl3/internal/Engine.java index fa304700b..bb355b429 100644 --- a/src/main/java/org/apache/commons/jexl3/internal/Engine.java +++ b/src/main/java/org/apache/commons/jexl3/internal/Engine.java @@ -31,8 +31,10 @@ import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; import java.util.function.Consumer; +import java.util.function.IntFunction; import java.util.function.Predicate; +import org.apache.commons.jexl3.JexlCache; import org.apache.commons.jexl3.JexlArithmetic; import org.apache.commons.jexl3.JexlBuilder; import org.apache.commons.jexl3.JexlContext; @@ -160,7 +162,7 @@ private UberspectHolder() {} /** * The expression cache. */ - protected final SoftCache cache; + protected final JexlCache cache; /** * The default jxlt engine. */ @@ -173,6 +175,10 @@ private UberspectHolder() {} * A cached version of the options. */ protected final JexlOptions options; + /** + * The cache factory method. + */ + protected final IntFunction> cacheFactory; /** * Creates an engine with default arguments. @@ -229,7 +235,9 @@ public Engine(final JexlBuilder conf) { this.scriptFeatures = new JexlFeatures(features).script(true).namespaceTest(nsTest); this.charset = conf.charset(); // caching: - this.cache = conf.cache() <= 0 ? null : new SoftCache<>(conf.cache()); + IntFunction> factory = conf.cacheFactory(); + this.cacheFactory = factory == null ? SoftCache::new : factory; + this.cache = (JexlCache) (conf.cache() > 0 ? factory.apply(conf.cache()) : null); this.cacheThreshold = conf.cacheThreshold(); if (uberspect == null) { throw new IllegalArgumentException("uberspect can not be null"); diff --git a/src/main/java/org/apache/commons/jexl3/internal/SoftCache.java b/src/main/java/org/apache/commons/jexl3/internal/SoftCache.java index f298959a3..60c808848 100644 --- a/src/main/java/org/apache/commons/jexl3/internal/SoftCache.java +++ b/src/main/java/org/apache/commons/jexl3/internal/SoftCache.java @@ -16,25 +16,32 @@ */ package org.apache.commons.jexl3.internal; +import org.apache.commons.jexl3.JexlCache; + import java.lang.ref.SoftReference; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.locks.ReadWriteLock; -import java.util.concurrent.locks.ReentrantReadWriteLock; /** * A soft referenced cache. *

- * The actual cache is held through a soft reference, allowing it to be GCed - * under memory pressure.

+ * The actual cache is held through a soft reference, allowing it to be GCed + * under memory pressure. + *

+ *

+ * Note that the underlying map is a synchronized LinkedHashMap. + * The reason is that a get() will reorder elements (the LRU queue) and thus + * needs to be guarded to be thread-safe. + *

* * @param the cache key entry type * @param the cache key value type */ -public class SoftCache { +public class SoftCache implements JexlCache { /** * The default cache load factor. */ @@ -42,24 +49,19 @@ public class SoftCache { /** * The cache size. */ - private final int size; + protected final int size; /** * The soft reference to the cache map. */ - private SoftReference> ref; - /** - * The cache r/w lock. - */ - private final ReadWriteLock lock; + protected volatile SoftReference> reference; /** * Creates a new instance of a soft cache. * * @param theSize the cache size */ - SoftCache(final int theSize) { + public SoftCache(final int theSize) { size = theSize; - lock = new ReentrantReadWriteLock(); } /** @@ -67,6 +69,7 @@ public class SoftCache { * * @return the cache size */ + @Override public int size() { return size; } @@ -74,12 +77,15 @@ public int size() { /** * Clears the cache. */ + @Override public void clear() { - lock.writeLock().lock(); - try { - ref = null; - } finally { - lock.writeLock().unlock(); + final SoftReference> ref = reference; + if (ref != null ) { + reference = null; + final Map map = ref.get(); + if (map != null) { + map.clear(); + } } } @@ -89,14 +95,11 @@ public void clear() { * @param key the cache entry key * @return the cache entry value */ + @Override public V get(final K key) { - lock.readLock().lock(); - try { - final Map map = ref != null ? ref.get() : null; - return map != null ? map.get(key) : null; - } finally { - lock.readLock().unlock(); - } + final SoftReference> ref = reference; + final Map map = ref != null ? ref.get() : null; + return map != null ? map.get(key) : null; } /** @@ -105,18 +108,21 @@ public V get(final K key) { * @param key the cache entry key * @param script the cache entry value */ - public void put(final K key, final V script) { - lock.writeLock().lock(); - try { - Map map = ref != null ? ref.get() : null; - if (map == null) { - map = createCache(size); - ref = new SoftReference<>(map); + @Override + public V put(final K key, final V script) { + SoftReference> ref = reference; + Map map = ref != null ? ref.get() : null; + if (map == null) { + synchronized (this) { + ref = reference; + map = ref != null ? ref.get() : null; + if (map == null) { + map = createMap(size); + reference = new SoftReference<>(map); + } } - map.put(key, script); - } finally { - lock.writeLock().unlock(); } + return map.put(key, script); } /** @@ -126,21 +132,20 @@ public void put(final K key, final V script) { * * @return the cache entry list */ - public List> entries() { - lock.readLock().lock(); - try { - final Map map = ref != null ? ref.get() : null; - if (map == null) { - return Collections.emptyList(); - } + @Override + public Collection> entries() { + final SoftReference> ref = reference; + final Map map = ref != null ? ref.get() : null; + if (map == null) { + return Collections.emptyList(); + } + synchronized(map) { final Set> set = map.entrySet(); final List> entries = new ArrayList<>(set.size()); for (final Map.Entry e : set) { entries.add(new SoftCacheEntry<>(e)); } return entries; - } finally { - lock.readLock().unlock(); } } @@ -152,18 +157,20 @@ public List> entries() { * @param cacheSize the cache size, must be > 0 * @return a Map usable as a cache bounded to the given size */ - public Map createCache(final int cacheSize) { - return new java.util.LinkedHashMap(cacheSize, LOAD_FACTOR, true) { - /** - * Serial version UID. - */ - private static final long serialVersionUID = 1L; - - @Override - protected boolean removeEldestEntry(final Map.Entry eldest) { - return super.size() > cacheSize; + public Map createMap(final int cacheSize) { + return Collections.synchronizedMap( + new java.util.LinkedHashMap(cacheSize, LOAD_FACTOR, true) { + /** + * Serial version UID. + */ + private static final long serialVersionUID = 1L; + + @Override + protected boolean removeEldestEntry(final Map.Entry eldest) { + return super.size() > cacheSize; + } } - }; + ); } } @@ -209,3 +216,4 @@ public V setValue(final V v) { } } + diff --git a/src/main/java/org/apache/commons/jexl3/internal/TemplateEngine.java b/src/main/java/org/apache/commons/jexl3/internal/TemplateEngine.java index 6737f11e8..1504123b7 100644 --- a/src/main/java/org/apache/commons/jexl3/internal/TemplateEngine.java +++ b/src/main/java/org/apache/commons/jexl3/internal/TemplateEngine.java @@ -26,6 +26,7 @@ import java.util.List; import java.util.Set; +import org.apache.commons.jexl3.JexlCache; import org.apache.commons.jexl3.JexlContext; import org.apache.commons.jexl3.JexlException; import org.apache.commons.jexl3.JexlInfo; @@ -42,7 +43,7 @@ */ public final class TemplateEngine extends JxltEngine { /** The TemplateExpression cache. */ - final SoftCache cache; + final JexlCache cache; /** The JEXL engine instance. */ final Engine jexl; /** The logger. */ @@ -69,7 +70,7 @@ public TemplateEngine(final Engine aJexl, final char deferred) { this.jexl = aJexl; this.logger = aJexl.logger; - this.cache = new SoftCache<>(cacheSize); + this.cache = (JexlCache) aJexl.cacheFactory.apply(cacheSize); immediateChar = immediate; deferredChar = deferred; noscript = noScript; diff --git a/src/test/java/org/apache/commons/jexl3/CacheTest.java b/src/test/java/org/apache/commons/jexl3/CacheTest.java index cb1ccb2a9..4ae8ddd49 100644 --- a/src/test/java/org/apache/commons/jexl3/CacheTest.java +++ b/src/test/java/org/apache/commons/jexl3/CacheTest.java @@ -525,9 +525,18 @@ static class TestCacheArguments { 3, 3, 3, 4, 4, 4, 0, 0, 1, 1, 2, 2, 3, 3, 4, 4, 2, 2, 3, 3, 0 }; - private static final JexlEngine jexlCache = new JexlBuilder().cache(1024).debug(true).strict(true).create(); - - private static final JexlEngine jexlNoCache = new JexlBuilder().cache(0).debug(true).strict(true).create(); + private static final JexlEngine jexlCache = new JexlBuilder() + .cache(1024) + .cacheFactory(JexlCache.createSynchronized()) + .debug(true) + .strict(true) + .create(); + + private static final JexlEngine jexlNoCache = new JexlBuilder() + .cache(0) + .debug(true) + .strict(true) + .create(); private static JexlEngine jexl = jexlCache;