CaffeineCacheManager getCache method cause thread block

[huangxfchn]

• spring-context-support 5.3.18
• jdk version：17.0.4

here is my thread stack, block 193 threads

CaffeineCacheManager.java

public Cache getCache(String name) {
return this.cacheMap.computeIfAbsent(name, cacheName ->
		this.dynamic ? createCaffeineCache(cacheName) : null);
}

here is my code:

@CacheConfig(cacheNames = "branch")
public class BranchServiceImpl {
    @Cacheable(key = "#exp.id")
    public List<Branch> findById(Experiment exp) {
         // ……
    }
    
    @CachePut(key = "#exp.id")
    public List<Branch> updateCacheByExpId(Experiment exp, List<Branch> branches) {
         // ……
    }
}

[bclozel]

Could you provide a sample application that reproduces the problem? It should be a small and simple application, ideally created with https://start.spring.io. You can share it here as a zip or a git repository. Thanks!

[huangxfchn]

Could you provide a sample application that reproduces the problem? It should be a small and simple application, ideally created with https://start.spring.io. You can share it here as a zip or a git repository. Thanks!

Ok, wait a moment.

[huangxfchn]

• git repository：https://github.com/huangxfchn/spring-cache-block-demo
• run CacheTest

Then, use the jstack command to view the thread status. You can see that there are a lot of block threads. Here's one of the thread stacks

"pool-1-thread-52" #71 prio=5 os_prio=0 cpu=328.13ms elapsed=14.74s tid=0x000001994a54f540 nid=0x785c waiting for monitor entry  [0x0000007e704ff000]
   java.lang.Thread.State: BLOCKED (on object monitor)
        at java.util.concurrent.ConcurrentHashMap.computeIfAbsent([email protected]/ConcurrentHashMap.java:1726)
        - waiting to lock <0x0000000683cfdb58> (a java.util.concurrent.ConcurrentHashMap$Node)
        at org.springframework.cache.caffeine.CaffeineCacheManager.getCache(CaffeineCacheManager.java:191)
        at com.example.demo.CacheTest.lambda$test$0(CacheTest.java:30)
        at com.example.demo.CacheTest$$Lambda$663/0x0000000800ef9238.call(Unknown Source)
        at java.util.concurrent.FutureTask.run([email protected]/FutureTask.java:264)
        at java.util.concurrent.ThreadPoolExecutor.runWorker([email protected]/ThreadPoolExecutor.java:1136)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run([email protected]/ThreadPoolExecutor.java:635)
        at java.lang.Thread.run([email protected]/Thread.java:833)

And, I found this problem in both jdk1.8 and jdk17.

I think, the CaffeineCacheManager#getCache() method can be optimized like this:

public Cache getCache(String name) {
	Cache cache = this.cacheMap.get(name);
	if (cache != null) {
		return cache;
	}
	return (Cache)this.cacheMap.computeIfAbsent(name, (cacheName) -> {
		return this.dynamic ? this.createCaffeineCache(cacheName) : null;
	});
}

[huangxfchn]

The java.util.concurrent.ConcurrentHashMap#computeIfAbsent method has lock contention, especially if concurrency is very high

[huangxfchn]

This problem leads to a lot of block threads in our online environment.

[bclozel]

Thanks for the sample @huangxfchn .

As explained in this PR comment, the Javadoc for this class seems to indicate that read operations are completely lock free. I've reproduced the behavior you're describing even after pre-populating caches with the set of cache names.

We'll revisit the implementation there to avoid using computeIfAbsent when it's not needed and to avoid storing null entries in the map altogether. This will be backported to 53.x in #30085.

Thanks for your report!

[ben-manes]

FYI, for context on computeIfAbsent(), in 2014:

Performance-wise, computeIfAbsent pessimistically locks instead of optimistically trying to return an existing entry.

There are lots of tradeoffs. With the current implementation, if you are implementing a cache, it may be better to code cache.get to itself do a pre-screen, as in:
V v = map.get(key);
return (v != null) ? v : map.computeIfAbsent(key, function);

However, the exact benefit depends on access patterns.
For example, I reran your benchmark cases (urls below) on a 32way x86, and got throughputs (ops/sec) that are dramatically better with pre-screen for the case of a single key, but worse with your Zipf-distributed keys. As an intermediate test, I measured the impact of adding a single-node prescreen ("1cif") before locking inside CHM.computeIfAbsent, that is similar to what was done in some pre-release versions:

Same key
cif: 1402559
get+cif: 3775886700
1cif: 1760916148

Zipf-distributed keys
cif: 1414945003
get+cif: 882477874
1cif: 618668961

One might think (I did until running similar experiments)
that the "1cif" version would be the best compromise.
But currently it isn't.
This is in part due to interactions with biased locking,
that in some cases basically provide "free" prescreens, but in other cases add safepoint/GC pressure in addition to lock contention. This is all up for re-consideration though.

-Doug

Caffeine therefore does this precscreen as shown in the benchmarks. Doug decided on adopting 1cif in recent jdks. He is currently pondering a per-entry lock vs the current bin lock, which should resolve this entirely.