From 6aeabe4a08a11bc86877eb06527d0e95e22d24ee Mon Sep 17 00:00:00 2001 From: Michael Gibney Date: Mon, 9 Dec 2024 15:53:33 -0500 Subject: [PATCH 1/2] SOLR-17597: Fix `CaffeineCache.put()` ramBytes decrement --- .../src/java/org/apache/solr/search/CaffeineCache.java | 10 +++++++++- .../test/org/apache/solr/search/TestCaffeineCache.java | 4 ++-- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/search/CaffeineCache.java b/solr/core/src/java/org/apache/solr/search/CaffeineCache.java index 1d921030ff7..dc436cb016e 100644 --- a/solr/core/src/java/org/apache/solr/search/CaffeineCache.java +++ b/solr/core/src/java/org/apache/solr/search/CaffeineCache.java @@ -275,7 +275,15 @@ public V computeIfAbsent(K key, IOFunction mappingFuncti public V put(K key, V val) { inserts.increment(); V old = cache.asMap().put(key, val); - recordRamBytes(key, old, val); + // ramBytes decrement for `old` happens via #onRemoval + if (val != old) { + // NOTE: this is conditional on `val != old` in order to work around a + // behavior in the Caffeine library: where there is reference equality + // between `val` and `old`, caffeine does _not_ invoke RemovalListener, + // so the entry is not decremented for the replaced value (hence we + // don't need to increment ram bytes for the entry either). + recordRamBytes(key, null, val); + } return old; } diff --git a/solr/core/src/test/org/apache/solr/search/TestCaffeineCache.java b/solr/core/src/test/org/apache/solr/search/TestCaffeineCache.java index 87ff29d5e6a..b4c69600fbd 100644 --- a/solr/core/src/test/org/apache/solr/search/TestCaffeineCache.java +++ b/solr/core/src/test/org/apache/solr/search/TestCaffeineCache.java @@ -308,7 +308,7 @@ public void testRamBytesSync() throws IOException { cache.put(0, "test"); long nonEmptySize = cache.ramBytesUsed(); - cache.put(0, "test"); + cache.put(0, random().nextBoolean() ? "test" : "rest"); assertEquals(nonEmptySize, cache.ramBytesUsed()); cache.remove(0); @@ -341,7 +341,7 @@ public void testRamBytesAsync() throws IOException { cache.put(0, "test"); long nonEmptySize = cache.ramBytesUsed(); - cache.put(0, "test"); + cache.put(0, random().nextBoolean() ? "test" : "rest"); assertEquals(nonEmptySize, cache.ramBytesUsed()); cache.remove(0); From 36babef455f246023e0cde46f3be904c43926028 Mon Sep 17 00:00:00 2001 From: Michael Gibney Date: Wed, 18 Dec 2024 12:00:16 -0500 Subject: [PATCH 2/2] `oldValue` arg is no longer relevant for `recordRamBytes()` method --- .../org/apache/solr/search/CaffeineCache.java | 28 ++++++++----------- 1 file changed, 12 insertions(+), 16 deletions(-) diff --git a/solr/core/src/java/org/apache/solr/search/CaffeineCache.java b/solr/core/src/java/org/apache/solr/search/CaffeineCache.java index dc436cb016e..091dd984fa5 100644 --- a/solr/core/src/java/org/apache/solr/search/CaffeineCache.java +++ b/solr/core/src/java/org/apache/solr/search/CaffeineCache.java @@ -232,7 +232,7 @@ private V computeAsync(K key, IOFunction mappingFunction // We reserved the slot, so we do the work V value = mappingFunction.apply(key); future.complete(value); // This will update the weight and expiration - recordRamBytes(key, null, value); + recordRamBytes(key, value); inserts.increment(); return value; } catch (Error | RuntimeException | IOException e) { @@ -262,7 +262,7 @@ public V computeIfAbsent(K key, IOFunction mappingFuncti if (value == null) { return null; } - recordRamBytes(key, null, value); + recordRamBytes(key, value); inserts.increment(); return value; }); @@ -282,31 +282,27 @@ public V put(K key, V val) { // between `val` and `old`, caffeine does _not_ invoke RemovalListener, // so the entry is not decremented for the replaced value (hence we // don't need to increment ram bytes for the entry either). - recordRamBytes(key, null, val); + recordRamBytes(key, val); } return old; } /** - * Update the estimate of used memory + * Update the estimate of used memory. + * + *

NOTE: old value (in the event of replacement) adjusts {@link #ramBytes} via {@link + * #onRemoval(Object, Object, RemovalCause)} * * @param key the cache key - * @param oldValue the old cached value to decrement estimate (can be null) * @param newValue the new cached value to increment estimate */ - private void recordRamBytes(K key, V oldValue, V newValue) { + private void recordRamBytes(K key, V newValue) { ramBytes.add( RamUsageEstimator.sizeOfObject(newValue, RamUsageEstimator.QUERY_DEFAULT_RAM_BYTES_USED)); - if (oldValue == null) { - ramBytes.add( - RamUsageEstimator.sizeOfObject(key, RamUsageEstimator.QUERY_DEFAULT_RAM_BYTES_USED)); - ramBytes.add(RamUsageEstimator.LINKED_HASHTABLE_RAM_BYTES_PER_ENTRY); - if (async) ramBytes.add(RAM_BYTES_PER_FUTURE); - } else { - ramBytes.add( - -RamUsageEstimator.sizeOfObject( - oldValue, RamUsageEstimator.QUERY_DEFAULT_RAM_BYTES_USED)); - } + ramBytes.add( + RamUsageEstimator.sizeOfObject(key, RamUsageEstimator.QUERY_DEFAULT_RAM_BYTES_USED)); + ramBytes.add(RamUsageEstimator.LINKED_HASHTABLE_RAM_BYTES_PER_ENTRY); + if (async) ramBytes.add(RAM_BYTES_PER_FUTURE); } @Override