From 6ec264252baacde1084e3bfd229332881a01d923 Mon Sep 17 00:00:00 2001 From: Brian Clozel Date: Thu, 19 Oct 2023 15:54:31 +0200 Subject: [PATCH] Ensure consistent value count in ConcurrentReferenceHashMap#Segment Prior to this commit, the `ConcurrentReferenceHashMap#Segment` would recalculate its element count during the restructure phase by subtracting the number of polled elements from the queue to the current count. Later, the newly restructured `references` array would only contain references that 1) are not in the set of elements to purge and 2) that hold a non-null value. The issues with this approach are multiple. The newly calculated count is only an estimate, as some situations can make it invalid, even temporarily. * since we initially collected all elements to be purged from the queue, the GC might have collected new values. This means that we might filter out more references that we initially intended to * because the restructure operation re-creates new references for all elements in the original array, we might later get references from the queue that are not in the array anymore. This could lead to "duplicate" removals for the same value Because several methods in the Segment class have special no-op behavior when `count == 0`, an invalid count can lead to keys appearing missing when they are actually still present. In some scenarios, this can decrease the performance of the cache since values need to be recalculated. This commit fixes this inconsistency count issue by first using an estimate in order to decide whether the array needs a resize and then by counting the actual numbers of elements inserted in the restructured array as the new count. Fixes gh-31373 --- .../util/ConcurrentReferenceHashMap.java | 23 +++++++++++-------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/spring-core/src/main/java/org/springframework/util/ConcurrentReferenceHashMap.java b/spring-core/src/main/java/org/springframework/util/ConcurrentReferenceHashMap.java index 4337aca2630..7dcebf7a68d 100644 --- a/spring-core/src/main/java/org/springframework/util/ConcurrentReferenceHashMap.java +++ b/spring-core/src/main/java/org/springframework/util/ConcurrentReferenceHashMap.java @@ -56,6 +56,7 @@ import org.springframework.lang.Nullable; * * @author Phillip Webb * @author Juergen Hoeller + * @author Brian Clozel * @since 3.2 * @param the key type * @param the value type @@ -568,7 +569,7 @@ public class ConcurrentReferenceHashMap extends AbstractMap implemen * references that have been garbage collected. * @param allowResize if resizing is permitted */ - protected final void restructureIfNecessary(boolean allowResize) { + void restructureIfNecessary(boolean allowResize) { int currCount = this.count.get(); boolean needsResize = allowResize && (currCount > 0 && currCount >= this.resizeThreshold); Reference ref = this.referenceManager.pollForPurge(); @@ -581,7 +582,7 @@ public class ConcurrentReferenceHashMap extends AbstractMap implemen boolean needsResize; lock(); try { - int countAfterRestructure = this.count.get(); + int expectedCount = this.count.get(); Set> toPurge = Collections.emptySet(); if (ref != null) { toPurge = new HashSet<>(); @@ -590,11 +591,11 @@ public class ConcurrentReferenceHashMap extends AbstractMap implemen ref = this.referenceManager.pollForPurge(); } } - countAfterRestructure -= toPurge.size(); + expectedCount -= toPurge.size(); - // Recalculate taking into account count inside lock and items that - // will be purged - needsResize = (countAfterRestructure > 0 && countAfterRestructure >= this.resizeThreshold); + // Estimate new count, taking into account count inside lock and items that + // will be purged. + needsResize = (expectedCount > 0 && expectedCount >= this.resizeThreshold); boolean resizing = false; int restructureSize = this.references.length; if (allowResize && needsResize && restructureSize < MAXIMUM_SEGMENT_SIZE) { @@ -607,6 +608,7 @@ public class ConcurrentReferenceHashMap extends AbstractMap implemen (resizing ? createReferenceArray(restructureSize) : this.references); // Restructure + int newCount = 0; for (int i = 0; i < this.references.length; i++) { ref = this.references[i]; if (!resizing) { @@ -615,10 +617,13 @@ public class ConcurrentReferenceHashMap extends AbstractMap implemen while (ref != null) { if (!toPurge.contains(ref)) { Entry entry = ref.get(); + // Also filter out null references that are now null + // they should be polled the queue in a later restructure call. if (entry != null) { int index = getIndex(ref.getHash(), restructured); restructured[index] = this.referenceManager.createReference( entry, ref.getHash(), restructured[index]); + newCount++; } } ref = ref.getNext(); @@ -630,7 +635,7 @@ public class ConcurrentReferenceHashMap extends AbstractMap implemen this.references = restructured; this.resizeThreshold = (int) (this.references.length * getLoadFactor()); } - this.count.set(Math.max(countAfterRestructure, 0)); + this.count.set(Math.max(newCount, 0)); } finally { unlock(); @@ -667,14 +672,14 @@ public class ConcurrentReferenceHashMap extends AbstractMap implemen /** * Return the size of the current references array. */ - public final int getSize() { + public int getSize() { return this.references.length; } /** * Return the total number of references in this segment. */ - public final int getCount() { + public int getCount() { return this.count.get(); } }