Description

This commit fixes a performance problem in CachedIntrospectionResults. If multiple threads call CachedIntrospectionResults::forClass at the same time, more than one CachedIntrospectionResults will be created redundantly and consume a lot of CPU time.

Reproduce

I have a server with only one CPU. Assume that 100 requests come simultaneously and each request will call BeanUtils::getPropertyDescriptor directly and CachedIntrospectionResults::forClass indirectly. When loading the same keys by different threads, keys are not locked. In my test, each CachedIntrospectionResults::forClass call consumes more than 100ms CPU time, so 100 requests consume 10000ms CPU time in total.

Comment From: sdeleuze

Such change could indeed avoid redundant creation of CachedIntrospectionResults when multiple threads are involved, but ConcurrentHashMap#computeIfAbsent Javadoc mentions:

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.

I am not sure CachedIntrospectionResults constructors can be qualified as "short and simple". IMO the risk to introduce performance regression for multi-threading use cases due to locking issues looks to high to me even if your tests seems to be ok. If very strong evidence the risk of regression is negligible, I am open to reconsider, so far it looks like the tradeoff provided by the current implementation is fine.

As a consequence, I decline this PR.