root
/
spring-framework
mirror of https://github.com/spring-projects/spring-framework.git
1
0
Fork 0
Code Issues Actions Packages Projects Releases Wiki Activity

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
This commit is contained in:
Brian Clozel 2023-10-19 15:54:31 +02:00
parent 187b4e5ea6
commit 6ec264252b
1 changed files with 14 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

23
spring-core/src/main/java/org/springframework/util/ConcurrentReferenceHashMap.java
View File

@ -56,6 +56,7 @@ import org.springframework.lang.Nullable;
*
* @author Phillip Webb
* @author Juergen Hoeller
* @author Brian Clozel
* @since 3.2
* @param <K> the key type
* @param <V> the value type
@ -568,7 +569,7 @@ public class ConcurrentReferenceHashMap<K, V> extends AbstractMap<K, V> 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<K, V> ref = this.referenceManager.pollForPurge();
@ -581,7 +582,7 @@ public class ConcurrentReferenceHashMap<K, V> extends AbstractMap<K, V> implemen
boolean needsResize;
lock();
try {
int countAfterRestructure = this.count.get();
int expectedCount = this.count.get();
Set<Reference<K, V>> toPurge = Collections.emptySet();
if (ref != null) {
toPurge = new HashSet<>();
@ -590,11 +591,11 @@ public class ConcurrentReferenceHashMap<K, V> extends AbstractMap<K, V> 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<K, V> extends AbstractMap<K, V> 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<K, V> extends AbstractMap<K, V> implemen
while (ref != null) {
if (!toPurge.contains(ref)) {
Entry<K, V> 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<K, V> extends AbstractMap<K, V> 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<K, V> extends AbstractMap<K, V> 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();
}
}