Improved performance of getter/setter method caching in OgnlRuntime

[danielfernandez]

The current implementation of the cache for getter/setter methods in ognl.OgnlRuntime has three issues, mostly from the performance standpoint:

• The Map implementation being used for the cacheGetMethod and cacheSetMethod static variables is java.util.HashMap, which does not allow concurrent access. It seems that an attempt to solve this was made by adding a synchronized {...} block around the lines of code where new values are put(...) to these maps, but this is not completely correct because it would allow read access to those maps by a thread at the same time some other thread is inside that synchronized block modifying the map, which is a concurrency issue.
• The cache map keys are of a specialized private class called CacheKey, which contains two properties: Class clazz and String propertyName. The problem here is that every execution of getGetMethod(...) or getSetMethod(...) will need to create a new CacheKey object in order to access the cache. And given getGetMethod(...), used for obtaining getter methods, is one of the most executed OGNL methods in most environments, this means huge amounts of CacheKey objects created in heavy-load scenarios, which goes against memory efficiency and latency mitigation.
• Before every cache.get(...) call made in order to obtain the cached method (if it exists in cache), a cache.containsKey() call is performed too. This is done in order to correctly resolve cached Methods which might be null (because they don't exist and must be searched elsewhere), but in a large percent of the cases it means two accesses to the cache map when one (the get) would suffice.

In order to solve these three issues, I've performed the following actions, included in this pull request:

Action 1. Create a new private, internal class called ClassPropertyMethodCache so that both properties cacheGetMethod and cacheSetMethod in OgnlRuntime are instances of this class. This new class will cache the Methods corresponding to a specific propertyName of a clazz by means of a ConcurrentHashMap<Class<ConcurrentHashMap<String,Method>>, this is, a two-level ConcurrentHashMap which resolves indexed by clazz at the first level, and then by propertyName at the second.

By using ConcurrentHashMap, this new class solves the concurrency issue in this cache. And by using a two-level structure it removes the need to create any kind of complex key object, be it a specialized class like CacheKey or a String created by appending the class name and the property name like it was in previous versions. This means there will not be thousands of cache key objects created just for accessing the cache under heavy load, and as a result there will be a noticeable save in memory usage. Besides, both java.lang.Class and java.lang.String --the two classes used as keys in the maps-- have very fast and cached hashCode() and equals() methods, which means Map access will be very fast.

Also, the amount of ConcurrentHashMap instances created using this new structure (one per class) should not be too worrying, as the total amount of different classes for which getter methods are explored is normally very small in most OGNL scenarios. And in any case, it will be a tiny number compared to the number of different CacheKey objects being created right now (one per expression evaluation).

Action 2. Reorganize code in OgnlRuntime#getGetMethod(...) and OgnlRuntime#getSetMethod(...) so that calls to containsKey(...) are avoided when a cached getter actually exists (which is the most common case), and also removed synchronized {...} blocks, now unneeded thanks to the use of ConcurrentHashMapss.

[lukaszlenart]

Let's see how it works in real live apps :)

[perfnorm]

I know this is in, but wanted to get in some belated feedback:

1. Though it will generate a lot less runtime garbage, this solution is going to hold on to a lot more memory as we are going to get an entire ConcurrentHashMap object for each class, even if it contains very few properties. That is a tradeoff that doesn't seem worth it to me, at least for a site such as mine that has a significant number of classes used in ognl.
2. The sub-ConcurrentHashMap is using put rather than putIfAbsent which means we can have a race condition where two threads that are the first to add a property for a class overwrite each other. I actually think this should be fine, but probably worth calling out explicitly in the comments.