Skip to content

ConcurrentHashMap.computeIfAbsent used in AdvisedSupport can cause virtual thread pinning #32958

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
andrej-urvantsev opened this issue Jun 5, 2024 · 5 comments
Assignees
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: feedback-provided Feedback has been provided type: regression A bug that is also a regression
Milestone

Comments

@andrej-urvantsev
Copy link

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?

@andrej-urvantsev andrej-urvantsev changed the title ConcurrentMap.computeIfAbsent used in AdvisedSupport causes thread pinning ConcurrentMap.computeIfAbsent used in AdvisedSupport causes virtual thread pinning Jun 5, 2024
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Jun 5, 2024
@bclozel bclozel added the in: core Issues in core modules (aop, beans, core, context, expression) label Jun 5, 2024
@bclozel
Copy link
Member

bclozel commented Jun 5, 2024

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.

@bclozel bclozel added the status: waiting-for-feedback We need additional information before we can continue label Jun 5, 2024
@andrej-urvantsev
Copy link
Author

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.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 5, 2024
@jhoeller
Copy link
Contributor

jhoeller commented Jun 5, 2024

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?

@andrej-urvantsev
Copy link
Author

@jhoeller I guess so - it looks like computeIfAbsent is the one to blame here.

@jhoeller
Copy link
Contributor

jhoeller commented Jun 5, 2024

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).

@jhoeller jhoeller self-assigned this Jun 5, 2024
@jhoeller jhoeller added type: regression A bug that is also a regression and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Jun 5, 2024
@jhoeller jhoeller added this to the 6.1.9 milestone Jun 5, 2024
@jhoeller jhoeller changed the title ConcurrentMap.computeIfAbsent used in AdvisedSupport causes virtual thread pinning ConcurrentHashMap.computeIfAbsent used in AdvisedSupport can cause virtual thread pinning Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core Issues in core modules (aop, beans, core, context, expression) status: feedback-provided Feedback has been provided type: regression A bug that is also a regression
Projects
None yet
Development

No branches or pull requests

4 participants