ConcurrentHashMap.computeIfAbsent used in AdvisedSupport can cause virtual thread pinning

[lazystone]

ConcurrentHashMap.computeIfAbsent used in AdvisedSupport can cause virtual thread pinning

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

[lazystone]

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.

[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?

[lazystone]

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

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