When running Spring Boot application with -Djdk.tracePinnedThreads=full, this log message appears in the logs:
VirtualThread[#218,backgroundjob-worker]/runnable@ForkJoinPool-1-worker-11 reason:MONITOR
java.base/java.lang.VirtualThread$VThreadContinuation.onPinned(Unknown Source)
java.base/jdk.internal.vm.Continuation.onPinned0(Unknown Source)
java.base/java.lang.VirtualThread.park(Unknown Source)
...
java.base/java.util.concurrent.ConcurrentHashMap.computeIfAbsent(Unknown Source) <== monitors:1
org.springframework.aop.framework.AdvisedSupport.getInterceptorsAndDynamicInterceptionAdvice(AdvisedSupport.java:503)
synchronized usage in ConcurrentHashMap seems to be a known problem, but still not solved.
Would it be possible to avoid usage of ConcurrentHashMap in org.springframework.aop.framework.AdvisedSupport.java:503 and use something like HashMap and ReadWriteLock instead?
Comment From: bclozel
The message you've shared mentions:
As you've noticed, using a ReentrantLock per node would add unacceptable overhead, and would be especially unwelcome for the huge number of non-loom usages.
Isn't this going to be a huge performance drawback for everyone here?
Also, another message in the thread points to the fact that the Javadoc explicitly says that "Some attempted update operations on this map by other threads may be blocked while computation is in progress, so the computation should be short and simple."
Doesn't this mean that the pinning is very short because computation should be very short?
In my opinion, we should wait for the JDK team and not work on a sub-optimal solution in this case.
Comment From: andrej-urvantsev
Isn't this going to be a huge performance drawback for everyone here?
That really depends on the use case of ConcurrentHashMap in AdvisedSupport. From what I see(but I'm not a Spring guru in any way) - that map is a method cache and will be populated quite quickly. After that mapping function won't be called ever.
I don't see usage of ReadWriteLock as sub-optimal, unless there are real drawback to it.
If this possible, then it will allow Spring to be fully VT compatible now and not somewhere in the future.
Comment From: jhoeller
In some locations, we perform independent get/put steps against a ConcurrentHashMap instead of calling computeIfAbsent, accepting potential concurrent recomputation. Ironically we used to do this in AdvisedSupport before 6.1 and then accepted a PR that turned it into computeIfAbsent... I'm inclined to change that part back to independent get/put steps, assuming that this has no drawback since we used to do this for two decades. I suppose this would avoid the thread pinning that you are seeing?
Comment From: andrej-urvantsev
@jhoeller I guess so - it looks like computeIfAbsent is the one to blame here.
Comment From: jhoeller
Alright, I'll revise this as a concurrency performance regression for 6.1.9 then. It is effectively a regression since 6.0.x and 5.3.x do not show that effect, and it's also an inconsistency with the shared cache code path in AdvisedSupport that we introduced in 6.1 (where we do separate read/write steps against the volatile cachedInterceptors field).