Bug 50192 - performance issue after revision 746425
Summary: performance issue after revision 746425
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Jasper (show other bugs)
Version: trunk
Hardware: PC Windows XP
: P2 normal (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-01 15:17 UTC by Robert Goff
Modified: 2010-11-12 13:30 UTC (History)
0 users



Attachments
shared defaultResolver for each ELResolverImpl instance and reverts ELContextImpl (4.62 KB, patch)
2010-11-11 10:43 UTC, Robert Goff
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Goff 2010-11-01 15:17:42 UTC
Revision 746425 changed at least 2 files: ELResolverImpl and ELContextImpl.  I believe this causes an unnecessary performance regression in these two files.

In ELResolverImpl, every time that getDefaultResolver is called with security enabled, a new Object is going to be created.  It appears as if getDefaultResolver is going to be called a lot and this could significantly hurt performance.  I understand changing the DefaultResolver to be private and allowing access to it through the getter method, however, I don't believe a new object needs to be created every time the method is called.

In ELContextImpl, why does a new FunctionMapper need to be created for each ELContextImpl?

Thanks.
Comment 1 Mark Thomas 2010-11-09 10:13:17 UTC
The reasoning for this change is in the archives:
http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/STATUS.txt?r1=729825&r2=729824&pathrev=729825
Comment 2 Robert Goff 2010-11-11 10:43:05 UTC
Created attachment 26282 [details]
shared defaultResolver for each ELResolverImpl instance and reverts ELContextImpl
Comment 3 Robert Goff 2010-11-11 10:45:02 UTC
The NullFunctionMapper in ELContextImpl is a final object.  Therefore, it can not be overridden and the only method that it has is resolveFunction which will return null.  There is nothing about this object that could present a security issue.

As far as the ELResolverImpl is concerned, I can see how the static ELResolver could be accessed and it is possible that it would be altered by a malicious user.  However, I think that there could still be a performance gain here by not having each method withing the ELResolverImpl call getDefaultResolver().  Instead, you could create one defaultResolver for this particular instance (in the constructor) allowing all the methods for this instance to use that object, and you could still provide the static getDefaultResolver() method to return a new object each time.  This would at least save a few new constructions of the ELResolver when you're reusing the same ELResolverImpl object.

Please look at the patch to see the recommended change.
Comment 4 Mark Thomas 2010-11-12 13:30:36 UTC
Thanks for the analysis and the patch. I applied a slightly different version to 7.0.x that will be include din 7.0.5 onwards.