Bug 52998

Summary: Performance issue with ExpressionFactory.newInstance()
Product: Tomcat 7 Reporter: Konstantin Kolinko <knst.kolinko>
Component: JasperAssignee: Tomcat Developers Mailing List <dev>
Severity: normal    
Priority: P2    
Version: 7.0.26   
Target Milestone: ---   
Hardware: PC   
OS: Windows XP   
Attachments: 2012-03-30_tc8_52998_ExpressionFactory.patch

Description Konstantin Kolinko 2012-03-28 21:16:03 UTC
javax.el.ExpressionFactory.newInstance() implementation in Tomcat 7 does not cache created instance and performs class name discovery on every invocation.

The discovery process involves looking for the file named
"META-INF/services/javax.el.ExpressionFactory". So every invocation of the method involves looking for and maybe reading the file.

This issue was reported on the dev list, see thread:

[1] "Two performance problems (found during myfaces testing)", starting on 2012-03-08,
- http://tomcat.markmail.org/thread/7bbvzmkvyvryvn44
- http://marc.info/?t=133124021200002&r=1&w=2

The above thread [1] also references this one of myfaces:
[2] "EL method invocation performance", 2010-08-25:
- http://www.mail-archive.com/dev@myfaces.apache.org/msg48482.html

My evaluation is that
1. This problem is specific for Tomcat 7. - The ExpressionFactory.newInstance() method was added in EL 2.2 and Tomcat 6 does not have it.

2. It hits javax.el.BeanELResolver#invoke() the most, as ExpressionFactory.newInstance() is called on each invocation.

3. There are 2 places where the factory instance is stored in a static field. This is good for performance, but breaks the discovery process and may cause consequences if implementation is bundled in a web application. It is a bug. The places:

Comment 1 Konstantin Kolinko 2012-03-30 10:15:35 UTC
Created attachment 28518 [details]

Patch against trunk that implements caching in ExpressionFactory.

1. r1307310 should be ported to 7.0.x before this patch.

Maybe whitespace cleanup r1187778 shall be ported as well.

2. This patch only implements the caching.

The issue with static references mentioned in Comment 0 is not addressed.

3. There is no way to clean stale cache entries from stopped webapps,
but the cache entry objects are small, so I think we can live with it.

Weak references are used, so the class loaders from undeployed apps should be eligible for garbage collection.
Comment 2 Mark Thomas 2012-03-30 19:34:35 UTC
I've taken care of the static references in Jasper. I haven't reviewed the patch for the remaining issues yet.
Comment 3 Mark Thomas 2012-03-30 20:12:54 UTC
Patch looks good to me. I applied it so I can get on with the pre-release testing.