This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new c59a4b5757 Fix BZ 69532 - Optimise the creation of ExpressionFactory
instances
c59a4b5757 is described below
commit c59a4b5757583e3175c8d3d5a087928990779901
Author: Mark Thomas <[email protected]>
AuthorDate: Mon Feb 3 09:54:01 2025 +0000
Fix BZ 69532 - Optimise the creation of ExpressionFactory instances
Patch provided by John Engebretson.
https://bz.apache.org/bugzilla/show_bug.cgi?id=69532
---
java/jakarta/el/ExpressionFactoryCache.java | 84 +++++++++++++++++++
java/jakarta/el/Util.java | 105 +-----------------------
test/jakarta/el/TestExpressionFactoryCache.java | 62 ++++++++++++++
webapps/docs/changelog.xml | 5 ++
4 files changed, 153 insertions(+), 103 deletions(-)
diff --git a/java/jakarta/el/ExpressionFactoryCache.java
b/java/jakarta/el/ExpressionFactoryCache.java
new file mode 100644
index 0000000000..dd54a82887
--- /dev/null
+++ b/java/jakarta/el/ExpressionFactoryCache.java
@@ -0,0 +1,84 @@
+/*
+ * 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 jakarta.el;
+
+import java.lang.ref.WeakReference;
+import java.util.WeakHashMap;
+import java.util.concurrent.atomic.AtomicReference;
+
+class ExpressionFactoryCache {
+
+ private final
AtomicReference<WeakHashMap<ClassLoader,WeakReference<ExpressionFactory>>>
factoryCache;
+
+ ExpressionFactoryCache() {
+ factoryCache = new AtomicReference<>(new WeakHashMap<>());
+ }
+
+ /**
+ * Retrieves the currently cached ExpressionFactory, or creates a new one
if required. Reads from an immutable
+ * WeakHashMap (which is threadsafe); in the rare cases that mutation is
required, a new copy is created and
+ * modified, then swapped in via AtomicReference. Key performance
characteristics:
+ * <ol>
+ * <li>Reads are uncontended on an immutable object. (threadsafe)</li>
+ * <li>Writes are performed by copying an immutable object, then
inserting. (threadsafe)</li>
+ * <li>The new object is swapped in via AtomicReference, or re-attempted
if the data has since changed.
+ * (threadsafe)</li>
+ * <li>A single call will create 0 or 1 instances of ExpressionFactory.
Simultaneous initialization by multiple
+ * threads may create 2+ instances of ExpressionFactory, but excess
instances are short-lived and harmless.
+ * (memorysafe)</li>
+ * <li>ClassLoaders are weakly held (memorysafe)</li>
+ * <li>ExpressionFactorys are weakly held (memorysafe)</li>
+ * <li>No objects are allocated on cache hits (the common case)</li>
+ * </ol>
+ *
+ * @param cl The classloader for which the cached {@code
ExpressionFactory} is to be created or retrieved
+ *
+ * @return The cached {@code ExpressionFactory} for the given {@code
ClassLoader}
+ */
+ ExpressionFactory getOrCreateExpressionFactory(ClassLoader cl) {
+ WeakHashMap<ClassLoader,WeakReference<ExpressionFactory>> cache;
+ WeakHashMap<ClassLoader,WeakReference<ExpressionFactory>> newCache;
+ ExpressionFactory factory = null;
+ WeakReference<ExpressionFactory> factoryRef;
+ do {
+ // cache cannot be null
+ cache = factoryCache.get();
+
+ factoryRef = cache.get(cl);
+ // factoryRef can be null (could be uninitialized, or the GC
cleaned the weak ref)
+ if (factoryRef != null) {
+ factory = factoryRef.get();
+ // factory can be null (GC may have cleaned the ref)
+ if (factory != null) {
+ return factory;
+ }
+ }
+
+ // something somewhere was uninitialized or GCd
+ if (factory == null) {
+ // only create an instance on the first iteration of the loop
+ factory = ExpressionFactory.newInstance();
+ }
+ factoryRef = new WeakReference<>(factory);
+ newCache = new WeakHashMap<>(cache);
+
+ newCache.put(cl, factoryRef);
+ } while (!factoryCache.compareAndSet(cache, newCache));
+
+ return factory;
+ }
+}
diff --git a/java/jakarta/el/Util.java b/java/jakarta/el/Util.java
index 59bc98b538..63f6f3fba3 100644
--- a/java/jakarta/el/Util.java
+++ b/java/jakarta/el/Util.java
@@ -16,7 +16,6 @@
*/
package jakarta.el;
-import java.lang.ref.WeakReference;
import java.lang.reflect.AccessibleObject;
import java.lang.reflect.Array;
import java.lang.reflect.Constructor;
@@ -31,10 +30,6 @@ import java.util.Map;
import java.util.MissingResourceException;
import java.util.ResourceBundle;
import java.util.Set;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.locks.Lock;
-import java.util.concurrent.locks.ReadWriteLock;
-import java.util.concurrent.locks.ReentrantReadWriteLock;
class Util {
@@ -77,9 +72,7 @@ class Util {
}
}
-
- private static final CacheValue nullTcclFactory = new CacheValue();
- private static final Map<CacheKey,CacheValue> factoryCache = new
ConcurrentHashMap<>();
+ private static final ExpressionFactoryCache factoryCache = new
ExpressionFactoryCache();
/**
* Provides a per class loader cache of ExpressionFactory instances
without pinning any in memory as that could
@@ -89,101 +82,7 @@ class Util {
ClassLoader tccl = Thread.currentThread().getContextClassLoader();
- CacheValue cacheValue = null;
- ExpressionFactory factory = null;
-
- if (tccl == null) {
- cacheValue = nullTcclFactory;
- } else {
- CacheKey key = new CacheKey(tccl);
- cacheValue = factoryCache.get(key);
- if (cacheValue == null) {
- CacheValue newCacheValue = new CacheValue();
- cacheValue = factoryCache.putIfAbsent(key, newCacheValue);
- if (cacheValue == null) {
- cacheValue = newCacheValue;
- }
- }
- }
-
- final Lock readLock = cacheValue.getLock().readLock();
- readLock.lock();
- try {
- factory = cacheValue.getExpressionFactory();
- } finally {
- readLock.unlock();
- }
-
- if (factory == null) {
- final Lock writeLock = cacheValue.getLock().writeLock();
- writeLock.lock();
- try {
- factory = cacheValue.getExpressionFactory();
- if (factory == null) {
- factory = ExpressionFactory.newInstance();
- cacheValue.setExpressionFactory(factory);
- }
- } finally {
- writeLock.unlock();
- }
- }
-
- return factory;
- }
-
-
- /**
- * Key used to cache default ExpressionFactory information per class
loader. The class loader reference is never
- * {@code null}, because {@code null} tccl is handled separately.
- */
- private static class CacheKey {
- private final int hash;
- private final WeakReference<ClassLoader> ref;
-
- CacheKey(ClassLoader key) {
- hash = key.hashCode();
- ref = new WeakReference<>(key);
- }
-
- @Override
- public int hashCode() {
- return hash;
- }
-
- @Override
- public boolean equals(Object obj) {
- if (obj == this) {
- return true;
- }
- if (!(obj instanceof CacheKey)) {
- return false;
- }
- ClassLoader thisKey = ref.get();
- if (thisKey == null) {
- return false;
- }
- return thisKey == ((CacheKey) obj).ref.get();
- }
- }
-
- private static class CacheValue {
- private final ReadWriteLock lock = new ReentrantReadWriteLock();
- private WeakReference<ExpressionFactory> ref;
-
- CacheValue() {
- }
-
- public ReadWriteLock getLock() {
- return lock;
- }
-
- public ExpressionFactory getExpressionFactory() {
- return ref != null ? ref.get() : null;
- }
-
- public void setExpressionFactory(ExpressionFactory factory) {
- ref = new WeakReference<>(factory);
- }
+ return factoryCache.getOrCreateExpressionFactory(tccl);
}
diff --git a/test/jakarta/el/TestExpressionFactoryCache.java
b/test/jakarta/el/TestExpressionFactoryCache.java
new file mode 100644
index 0000000000..b759fa2df7
--- /dev/null
+++ b/test/jakarta/el/TestExpressionFactoryCache.java
@@ -0,0 +1,62 @@
+/*
+ * 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 jakarta.el;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+public class TestExpressionFactoryCache {
+ private static class TestClassLoader extends ClassLoader {
+ }
+
+ @Test
+ public void testCacheWithOneClassLoader() {
+ ExpressionFactory factory;
+ ClassLoader cl = new TestClassLoader();
+ ExpressionFactoryCache cache = new ExpressionFactoryCache();
+
+ factory = cache.getOrCreateExpressionFactory(cl);
+
+ Assert.assertEquals(factory, cache.getOrCreateExpressionFactory(cl));
+ }
+
+ @Test
+ public void testCacheWithInserts() {
+ final int numClassLoaders = 15;
+ ClassLoader[] loaders = new ClassLoader[numClassLoaders];
+ ExpressionFactory[] factories = new ExpressionFactory[numClassLoaders];
+
+ ExpressionFactoryCache cache = new ExpressionFactoryCache();
+
+ for (int i = 0; i < numClassLoaders; i++) {
+ loaders[i] = new TestClassLoader();
+ factories[i] = cache.getOrCreateExpressionFactory(loaders[i]);
+
+ // make a second call, ensure the same instance comes back
+ Assert.assertEquals(factories[i],
cache.getOrCreateExpressionFactory(loaders[i]));
+ }
+
+ // use a Set<> to verify each factory is unique
+ Set<ExpressionFactory> factorySet = new
HashSet<>(Arrays.asList(factories));
+ Assert.assertEquals(numClassLoaders, factorySet.size());
+ }
+
+}
\ No newline at end of file
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 9379414eaf..703b2c1485 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -310,6 +310,11 @@
characters in an EL identifier as defined by the Java Language
Specification. (markt)
</fix>
+ <fix>
+ <bug>69532</bug>: Optimise the creation of
+ <code>ExpressionFactory</code> instances. Patch provided by John
+ Engebretson. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Web applications">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
