This is an automated email from the ASF dual-hosted git repository.
markt-asf 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 d44d49338b Review "Cap eden promotion in ConcurrentCache.get() ..."
d44d49338b is described below
commit d44d49338b5da6d855428b5607f71bc6fce29df3
Author: Mark Thomas <[email protected]>
AuthorDate: Mon Nov 2 22:33:04 2026 +0000
Review "Cap eden promotion in ConcurrentCache.get() ..."
There are trade-offs here. Limiting the eden size() is good but
read-heavy usage could result in a high proportion of requests going to
the synchronized longterm storage. Emptying eden when it is full, how
ever it fills, prevents that although the read that triggers that will
pay the cost of updating longterm.
---
java/jakarta/el/BeanELResolver.java | 13 ++
java/org/apache/el/util/ConcurrentCache.java | 18 ++-
.../tomcat/util/collections/ConcurrentCache.java | 23 +++-
.../el/TestBeanELResolverConcurrentCache.java | 143 +++++++++++++++++++++
.../util}/TestConcurrentCache.java | 64 +++++++--
.../util/collections/TestConcurrentCache.java | 62 ++++++++-
6 files changed, 302 insertions(+), 21 deletions(-)
diff --git a/java/jakarta/el/BeanELResolver.java
b/java/jakarta/el/BeanELResolver.java
index 3b10b771c9..62686e7463 100644
--- a/java/jakarta/el/BeanELResolver.java
+++ b/java/jakarta/el/BeanELResolver.java
@@ -286,6 +286,19 @@ public class BeanELResolver extends ELResolver {
value = this.longterm.get(key);
}
if (value != null) {
+ // Handle a full eden in the same way as put(). Otherwise,
a
+ // workload dominated by cache hits could grow eden far
+ // beyond the configured size. Not promoting when eden is
+ // full is not an option either as, without any calls to
+ // put(), eden would then be stuck with its current content
+ // and every other hot entry would be read from the
+ // long-term store, which requires a lock, for ever.
+ if (this.eden.size() >= this.size) {
+ synchronized (longterm) {
+ this.longterm.putAll(this.eden);
+ }
+ this.eden.clear();
+ }
this.eden.put(key, value);
}
}
diff --git a/java/org/apache/el/util/ConcurrentCache.java
b/java/org/apache/el/util/ConcurrentCache.java
index 49b6d98300..a6d8981d77 100644
--- a/java/org/apache/el/util/ConcurrentCache.java
+++ b/java/org/apache/el/util/ConcurrentCache.java
@@ -49,7 +49,10 @@ public final class ConcurrentCache<K, V> {
/**
* Retrieves a value from the cache. If found in the long-term store, it is
- * promoted to the eden space for faster subsequent access.
+ * promoted to the eden space for faster subsequent access. As for
+ * {@link #put(Object, Object)}, if the eden space is full, its contents
are
+ * first moved to the long-term store so that the eden space never grows
+ * beyond the configured size.
*
* @param k the key to look up
* @return the cached value, or {@code null} if not found
@@ -61,6 +64,19 @@ public final class ConcurrentCache<K, V> {
v = this.longterm.get(k);
}
if (v != null) {
+ // Handle a full eden in the same way as put(). Otherwise, a
+ // workload dominated by cache hits could grow eden far beyond
+ // the configured size. Not promoting when eden is full is not
+ // an option either as, without any calls to put(), eden would
+ // then be stuck with its current content and every other hot
+ // entry would be read from the long-term store, which
+ // requires a lock, for ever.
+ if (this.eden.size() >= size) {
+ synchronized (longterm) {
+ this.longterm.putAll(this.eden);
+ }
+ this.eden.clear();
+ }
this.eden.put(k, v);
}
}
diff --git a/java/org/apache/tomcat/util/collections/ConcurrentCache.java
b/java/org/apache/tomcat/util/collections/ConcurrentCache.java
index d9a3a5b7d2..a1fd5d9295 100644
--- a/java/org/apache/tomcat/util/collections/ConcurrentCache.java
+++ b/java/org/apache/tomcat/util/collections/ConcurrentCache.java
@@ -49,7 +49,10 @@ public final class ConcurrentCache<K, V> {
/**
* Retrieves a value from the cache. If found in the long-term store, it is
- * promoted to the eden space for faster subsequent access.
+ * promoted to the eden space for faster subsequent access. As for
+ * {@link #put(Object, Object)}, if the eden space is full, its contents
are
+ * first moved to the long-term store so that the eden space never grows
+ * beyond the configured size.
*
* @param k the key to look up
* @return the cached value, or {@code null} if not found
@@ -60,10 +63,20 @@ public final class ConcurrentCache<K, V> {
synchronized (longterm) {
v = this.longterm.get(k);
}
- // Only promote into eden while it still has room. Otherwise, a
- // workload dominated by cache hits could grow eden far beyond the
- // configured size, since only put() trims eden.
- if (v != null && this.eden.size() < size) {
+ if (v != null) {
+ // Handle a full eden in the same way as put(). Otherwise, a
+ // workload dominated by cache hits could grow eden far beyond
+ // the configured size. Not promoting when eden is full is not
+ // an option either as, without any calls to put(), eden would
+ // then be stuck with its current content and every other hot
+ // entry would be read from the long-term store, which
+ // requires a lock, for ever.
+ if (this.eden.size() >= size) {
+ synchronized (longterm) {
+ this.longterm.putAll(this.eden);
+ }
+ this.eden.clear();
+ }
this.eden.put(k, v);
}
}
diff --git a/test/jakarta/el/TestBeanELResolverConcurrentCache.java
b/test/jakarta/el/TestBeanELResolverConcurrentCache.java
new file mode 100644
index 0000000000..4b99d48265
--- /dev/null
+++ b/test/jakarta/el/TestBeanELResolverConcurrentCache.java
@@ -0,0 +1,143 @@
+/*
+ * 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.reflect.Constructor;
+import java.lang.reflect.Field;
+import java.lang.reflect.Method;
+import java.util.Map;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+/**
+ * Test cases for the private cache used by {@link BeanELResolver}. Reflection
is used since the class is private.
+ */
+public class TestBeanELResolverConcurrentCache {
+
+ private static final Class<?> CACHE_CLASS = getCacheClass();
+
+ private static Class<?> getCacheClass() {
+ try {
+ return Class.forName("jakarta.el.BeanELResolver$ConcurrentCache");
+ } catch (ClassNotFoundException e) {
+ throw new IllegalStateException(e);
+ }
+ }
+
+
+ private static Object newCache(int size) throws Exception {
+ Constructor<?> constructor =
CACHE_CLASS.getDeclaredConstructor(int.class);
+ constructor.setAccessible(true);
+ return constructor.newInstance(Integer.valueOf(size));
+ }
+
+
+ private static Object get(Object cache, Object key) throws Exception {
+ Method method = CACHE_CLASS.getDeclaredMethod("get", Object.class);
+ method.setAccessible(true);
+ return method.invoke(cache, key);
+ }
+
+
+ private static void put(Object cache, Object key, Object value) throws
Exception {
+ Method method = CACHE_CLASS.getDeclaredMethod("put", Object.class,
Object.class);
+ method.setAccessible(true);
+ method.invoke(cache, key, value);
+ }
+
+
+ private static Map<?,?> eden(Object cache) throws Exception {
+ Field edenField = CACHE_CLASS.getDeclaredField("eden");
+ edenField.setAccessible(true);
+ return (Map<?,?>) edenField.get(cache);
+ }
+
+
+ @Test
+ public void testGetPromotesLongtermHit() throws Exception {
+ Object cache = newCache(2);
+
+ put(cache, "k1", "v1");
+ put(cache, "k2", "v2");
+ // Eden has reached the configured size. This put promotes the
previous eden contents to the long-term store.
+ put(cache, "k3", "v3");
+ Assert.assertEquals(1, eden(cache).size());
+
+ // Promoted back into eden since there is room
+ Assert.assertEquals("v1", get(cache, "k1"));
+ Assert.assertEquals(2, eden(cache).size());
+
+ // Eden is full. Its content is moved to the long-term store and k2 is
promoted into the empty eden.
+ Assert.assertEquals("v2", get(cache, "k2"));
+ Assert.assertEquals(1, eden(cache).size());
+ Assert.assertTrue(eden(cache).containsKey("k2"));
+
+ // Everything is still available
+ Assert.assertEquals("v1", get(cache, "k1"));
+ Assert.assertEquals("v3", get(cache, "k3"));
+ Assert.assertEquals("v2", get(cache, "k2"));
+ }
+
+
+ @Test
+ public void testGetWorkingSetMovesToEden() throws Exception {
+ Object cache = newCache(3);
+
+ String[] keys = { "a", "b", "c", "d", "e" };
+ for (String key : keys) {
+ put(cache, key, "v" + key);
+ }
+ Assert.assertFalse(eden(cache).containsKey("a"));
+ Assert.assertFalse(eden(cache).containsKey("b"));
+
+ for (int i = 0; i < 3; i++) {
+ Assert.assertEquals("va", get(cache, "a"));
+ Assert.assertEquals("vb", get(cache, "b"));
+ }
+
+ Assert.assertTrue(eden(cache).containsKey("a"));
+ Assert.assertTrue(eden(cache).containsKey("b"));
+ Assert.assertTrue(eden(cache).size() <= 3);
+
+ for (String key : keys) {
+ Assert.assertEquals("v" + key, get(cache, key));
+ }
+ }
+
+
+ @Test
+ public void testGetPromotionRespectsEdenSize() throws Exception {
+ int size = 10;
+ Object cache = newCache(size);
+
+ // Hold strong references to the keys so the long-term store cannot
lose entries via GC
+ String[] keys = new String[100];
+ for (int i = 0; i < keys.length; i++) {
+ keys[i] = "k" + i;
+ }
+ for (int i = 0; i < keys.length; i++) {
+ put(cache, keys[i], "v" + i);
+ }
+ for (int i = 0; i < keys.length; i++) {
+ Assert.assertEquals("v" + i, get(cache, keys[i]));
+ }
+
+ Assert.assertTrue("Eden size [" + eden(cache).size() + "] should not
exceed the configured size [" + size
+ + "]", eden(cache).size() <= size);
+ }
+}
diff --git a/test/org/apache/tomcat/util/collections/TestConcurrentCache.java
b/test/org/apache/el/util/TestConcurrentCache.java
similarity index 65%
copy from test/org/apache/tomcat/util/collections/TestConcurrentCache.java
copy to test/org/apache/el/util/TestConcurrentCache.java
index 2b1d467752..a88730dbda 100644
--- a/test/org/apache/tomcat/util/collections/TestConcurrentCache.java
+++ b/test/org/apache/el/util/TestConcurrentCache.java
@@ -14,7 +14,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-package org.apache.tomcat.util.collections;
+package org.apache.el.util;
import java.lang.reflect.Field;
import java.util.Map;
@@ -27,11 +27,17 @@ import org.junit.Test;
*/
public class TestConcurrentCache {
- private static int edenSize(ConcurrentCache<String,String> cache)
+ private static Map<?,?> eden(ConcurrentCache<String,String> cache)
throws Exception {
Field edenField = ConcurrentCache.class.getDeclaredField("eden");
edenField.setAccessible(true);
- return ((Map<?,?>) edenField.get(cache)).size();
+ return (Map<?,?>) edenField.get(cache);
+ }
+
+
+ private static int edenSize(ConcurrentCache<String,String> cache)
+ throws Exception {
+ return eden(cache).size();
}
@@ -65,11 +71,12 @@ public class TestConcurrentCache {
/*
- * A long-term hit must be promoted into eden while eden still has room,
- * and served without promotion once eden is full.
+ * A long-term hit must be promoted into eden. If eden is full, its content
+ * is moved to the long-term store first, as for put(), so eden never
+ * exceeds the configured size.
*/
@Test
- public void testGetPromotesLongtermHitWithHeadroom() throws Exception {
+ public void testGetPromotesLongtermHit() throws Exception {
ConcurrentCache<String,String> cache = new ConcurrentCache<>(2);
cache.put("k1", "v1");
@@ -86,9 +93,50 @@ public class TestConcurrentCache {
Assert.assertEquals("v1", cache.get("k1"));
Assert.assertEquals(2, edenSize(cache));
- // No room left, so this long-term hit is served without promotion
+ // Eden is full. The content of eden is moved to the long-term store
+ // and then k2 is promoted into the now empty eden.
Assert.assertEquals("v2", cache.get("k2"));
- Assert.assertEquals(2, edenSize(cache));
+ Assert.assertEquals(1, edenSize(cache));
+ Assert.assertTrue(eden(cache).containsKey("k2"));
+
+ // The entries that were moved out of eden are still available
+ Assert.assertEquals("v1", cache.get("k1"));
+ Assert.assertEquals("v3", cache.get("k3"));
+ Assert.assertEquals("v2", cache.get("k2"));
+ }
+
+
+ /*
+ * Without any calls to put(), the entries that are read must end up in
eden
+ * (where they can be read without taking a lock) rather than eden staying
+ * as it was once it had filled.
+ */
+ @Test
+ public void testGetWorkingSetMovesToEden() throws Exception {
+ ConcurrentCache<String,String> cache = new ConcurrentCache<>(3);
+
+ String[] keys = { "a", "b", "c", "d", "e" };
+ for (String key : keys) {
+ cache.put(key, "v" + key);
+ }
+ // a, b and c are in the long-term store. d and e are in eden.
+ Assert.assertFalse(eden(cache).containsKey("a"));
+ Assert.assertFalse(eden(cache).containsKey("b"));
+
+ // Read the working set of a and b a number of times
+ for (int i = 0; i < 3; i++) {
+ Assert.assertEquals("va", cache.get("a"));
+ Assert.assertEquals("vb", cache.get("b"));
+ }
+
+ Assert.assertTrue(eden(cache).containsKey("a"));
+ Assert.assertTrue(eden(cache).containsKey("b"));
+ Assert.assertTrue(edenSize(cache) <= 3);
+
+ // Everything is still available
+ for (String key : keys) {
+ Assert.assertEquals("v" + key, cache.get(key));
+ }
}
diff --git a/test/org/apache/tomcat/util/collections/TestConcurrentCache.java
b/test/org/apache/tomcat/util/collections/TestConcurrentCache.java
index 2b1d467752..7f69ac4e5b 100644
--- a/test/org/apache/tomcat/util/collections/TestConcurrentCache.java
+++ b/test/org/apache/tomcat/util/collections/TestConcurrentCache.java
@@ -27,11 +27,17 @@ import org.junit.Test;
*/
public class TestConcurrentCache {
- private static int edenSize(ConcurrentCache<String,String> cache)
+ private static Map<?,?> eden(ConcurrentCache<String,String> cache)
throws Exception {
Field edenField = ConcurrentCache.class.getDeclaredField("eden");
edenField.setAccessible(true);
- return ((Map<?,?>) edenField.get(cache)).size();
+ return (Map<?,?>) edenField.get(cache);
+ }
+
+
+ private static int edenSize(ConcurrentCache<String,String> cache)
+ throws Exception {
+ return eden(cache).size();
}
@@ -65,11 +71,12 @@ public class TestConcurrentCache {
/*
- * A long-term hit must be promoted into eden while eden still has room,
- * and served without promotion once eden is full.
+ * A long-term hit must be promoted into eden. If eden is full, its content
+ * is moved to the long-term store first, as for put(), so eden never
+ * exceeds the configured size.
*/
@Test
- public void testGetPromotesLongtermHitWithHeadroom() throws Exception {
+ public void testGetPromotesLongtermHit() throws Exception {
ConcurrentCache<String,String> cache = new ConcurrentCache<>(2);
cache.put("k1", "v1");
@@ -86,9 +93,50 @@ public class TestConcurrentCache {
Assert.assertEquals("v1", cache.get("k1"));
Assert.assertEquals(2, edenSize(cache));
- // No room left, so this long-term hit is served without promotion
+ // Eden is full. The content of eden is moved to the long-term store
+ // and then k2 is promoted into the now empty eden.
Assert.assertEquals("v2", cache.get("k2"));
- Assert.assertEquals(2, edenSize(cache));
+ Assert.assertEquals(1, edenSize(cache));
+ Assert.assertTrue(eden(cache).containsKey("k2"));
+
+ // The entries that were moved out of eden are still available
+ Assert.assertEquals("v1", cache.get("k1"));
+ Assert.assertEquals("v3", cache.get("k3"));
+ Assert.assertEquals("v2", cache.get("k2"));
+ }
+
+
+ /*
+ * Without any calls to put(), the entries that are read must end up in
eden
+ * (where they can be read without taking a lock) rather than eden staying
+ * as it was once it had filled.
+ */
+ @Test
+ public void testGetWorkingSetMovesToEden() throws Exception {
+ ConcurrentCache<String,String> cache = new ConcurrentCache<>(3);
+
+ String[] keys = { "a", "b", "c", "d", "e" };
+ for (String key : keys) {
+ cache.put(key, "v" + key);
+ }
+ // a, b and c are in the long-term store. d and e are in eden.
+ Assert.assertFalse(eden(cache).containsKey("a"));
+ Assert.assertFalse(eden(cache).containsKey("b"));
+
+ // Read the working set of a and b a number of times
+ for (int i = 0; i < 3; i++) {
+ Assert.assertEquals("va", cache.get("a"));
+ Assert.assertEquals("vb", cache.get("b"));
+ }
+
+ Assert.assertTrue(eden(cache).containsKey("a"));
+ Assert.assertTrue(eden(cache).containsKey("b"));
+ Assert.assertTrue(edenSize(cache) <= 3);
+
+ // Everything is still available
+ for (String key : keys) {
+ Assert.assertEquals("v" + key, cache.get(key));
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
