Reducing memory usage while evaluating reflective EL expressions

[jengebr]

This change fixes https://bz.apache.org/bugzilla/show_bug.cgi?id=69381 by caching the JVM-provided Method[]. The OpenJDK compiler creates duplicate Method objects on every call to Class.getMethods(), which is an expensive task that is only made worse on complex classes that have more Methods.

ClassLoader leaks are avoided by keying off the class' name (String) and storing the Method[] in a WeakReference<Method[]>. The cost of the concurrency is easily outweighed by the savings from not duplicating dozens of objects, as demonstrated from output in the modified TestELParserPerformance provided at the bottom of this PR body.

Key points from the data:

1. Non-reflective expressions are unaffected.
2. For a bean with 10 methods, this logic is 42% faster.
3. For a bean with 20 methods, this logic is 57% faster
4. The new code still scales by number of Methods, but the growth is far less (4% vs 40%).

Original:

${beanA.name} duration=[599ms] or 599 ns each
${beanA.name} duration=[337ms] or 337 ns each
${beanA.name} duration=[330ms] or 330 ns each
${beanA.name} duration=[331ms] or 331 ns each
${beanA.name} duration=[330ms] or 330 ns each
Result: name
${beanA.getName()} duration=[1586ms] or 1586 ns each
${beanA.getName()} duration=[1401ms] or 1401 ns each
${beanA.getName()} duration=[1477ms] or 1477 ns each
${beanA.getName()} duration=[1401ms] or 1401 ns each
${beanA.getName()} duration=[1398ms] or 1398 ns each
Result: name
${beanB.getName()} duration=[1921ms] or 1921 ns each
${beanB.getName()} duration=[1972ms] or 1972 ns each
${beanB.getName()} duration=[1969ms] or 1969 ns each
${beanB.getName()} duration=[1970ms] or 1970 ns each
${beanB.getName()} duration=[1974ms] or 1974 ns each
Result: name

Optimized:

${beanA.name} duration=[570ms] or 570 ns each
${beanA.name} duration=[336ms] or 336 ns each
${beanA.name} duration=[330ms] or 330 ns each
${beanA.name} duration=[330ms] or 330 ns each
${beanA.name} duration=[330ms] or 330 ns each
Result: name
${beanA.getName()} duration=[990ms] or 990 ns each
${beanA.getName()} duration=[807ms] or 807 ns each
${beanA.getName()} duration=[814ms] or 814 ns each
${beanA.getName()} duration=[815ms] or 815 ns each
${beanA.getName()} duration=[812ms] or 812 ns each
Result: name
${beanB.getName()} duration=[883ms] or 883 ns each
${beanB.getName()} duration=[848ms] or 848 ns each
${beanB.getName()} duration=[848ms] or 848 ns each
${beanB.getName()} duration=[848ms] or 848 ns each
${beanB.getName()} duration=[848ms] or 848 ns each
Result: name

[ChristopherSchultz]

I was also surprised recently to discover that the JVM does not cache results of Class.getMethods and similar calls. Maybe it's returning a clone each time, but either way it takes time and generates garbage.

So there is a (positive) GC impact of this change as well.

[markt-asf]

Any caching approach needs to consider the case of two classes in different web applications with the same name but different structures. Essentially, caching needs to be per class loader.

[markt-asf]

I'd also like to see the key remove from the cache when the value expires.

[ChristopherSchultz]

I'd also like to see the key remove from the cache when the value expires.

+1

So I think your technique @jengebr is good, but the cache just needs to be pulled-up into the application scope or similar. Definitely don't cache directly in the Util class. It probably needs to be in a class that is loaded into the application's ClassLoader to be as clean as possible.

Actually, I think we must not change the javax.el.Util class, right?

[jengebr]

Thanks, I get your points about caching scope and cleanup.

@ChristopherSchultz what prevents changes to javax.el.Util?

[markt-asf]

I think the changes will have to go into Util. Something like a map of caches, keyed by class loader with appropriate use of weak references to avoid class loader leaks.

[jengebr]

This complexity led me to try a fallback plan, which can be faster: if the method has zero parameters, retrieve the Method by name. This works because there is no confusion about parameter types. Example:

        if (paramTypes.length == 0) {
            try {
                Method method = clazz.getMethod(methodName, paramTypes);
                return method;
            } catch (NoSuchMethodException | SecurityException e) {
                // fall through to broader, slower logic
            }
        }

Test data shows this is 2x faster than caching for the fast case, and has no appreciable impact on the others. That's an excellent outcome for my application, not sure how you feel about the general case?

[ChristopherSchultz]

@ChristopherSchultz what prevents changes to javax.el.Util?

That package and class don't "belong" to us, they belong to JavaEE/JakartaEE. Depending upon how it changes, we might fail the TCK. It's just better to put it in something under org.apache.whatever.

[markt-asf]

Actually, it does belong to us. It isn't part of the public API. We are free to change it how we wish. If you compare our version to the equivalent code in the EL API JAR from Jakarta EE there are various differences.

[markt-asf]

I'm fine with the zero-arg optimisation. If others want the more general caching solution it can still be implemented.

[jengebr]

Changes are committed as discussed, ready for re-review. Specifically:

1. Cache is removed, fast-path is added for zero-arg cases
2. Unit tests are added for:

• Zero-args method lookups
• One-arg method lookups
• Private method lookups

[markt-asf]

I'm planning on applying this manually to main and then back-porting. So far I've made the following changes:

• refactored the inner test classes to align with the rest of the package
• ignored the caching code that wasn't (fully) removed
• Called Util.getMethod() before returning to address some test failures caused by visibility issues

Just running the full test suite before committing to check I haven't missed anything.

[markt-asf]

Those were the only changes required. Fix applied to main, 11.0.1, 10.1.x and 9.0.x.

[jengebr]

Thank you so much! :)