Bug 53895 - Performance solution for ImplicitObjectELResolver
Summary: Performance solution for ImplicitObjectELResolver
Status: RESOLVED WONTFIX
Alias: None
Product: Tomcat 7
Classification: Unclassified
Component: Jasper (show other bugs)
Version: trunk
Hardware: PC All
: P2 enhancement (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-19 06:09 UTC by Sheldon Shao
Modified: 2012-10-31 17:52 UTC (History)
0 users



Attachments
New ImplicitObjectELResolverImpl (15.92 KB, application/octet-stream)
2012-09-19 06:09 UTC, Sheldon Shao
Details
Comparison test case (2.82 KB, application/octet-stream)
2012-09-19 06:12 UTC, Sheldon Shao
Details
Diff -u format of proposed change (2.69 KB, patch)
2012-10-28 19:53 UTC, Mark Thomas
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sheldon Shao 2012-09-19 06:09:59 UTC
Created attachment 29391 [details]
New ImplicitObjectELResolverImpl

ImplicitObjectELResolver uses Arrays.binarySearch to check scope name.
 It isn't very efficient.

It can be improved by using a map to hold the scope to scopeIndex.
 Such as,

 private static final Map<String, Integer> scopeMap = new HashMap<String, Integer>();

 static {
 for (int i = 0; i < SCOPE_NAMES.length; i++) { scopeMap.put(SCOPE_NAMES[i], i); }
 }

More detail please check out the attachment "ImplicitObjectELResolverImpl.java".
Comment 1 Sheldon Shao 2012-09-19 06:12:43 UTC
Created attachment 29392 [details]
Comparison test case

Sample test result:

New ImplicitObjectELResolver:578
Old ImplicitObjectELResolver:795
New ImplicitObjectELResolver:624
Old ImplicitObjectELResolver:889
New ImplicitObjectELResolver:515
Old ImplicitObjectELResolver:999
New ImplicitObjectELResolver:624
Old ImplicitObjectELResolver:1030
New ImplicitObjectELResolver:546
Old ImplicitObjectELResolver:1108
New ImplicitObjectELResolver:546
Old ImplicitObjectELResolver:982
New ImplicitObjectELResolver:640
Old ImplicitObjectELResolver:1060
Comment 2 Mark Thomas 2012-10-28 19:50:07 UTC
There are multiple concerns with this proposal.

1. Patches should be provided in diff -u format and not include formatting changes. It took time to get the proposed new class into a form where a true diff (no formatting changes) was available to review.

2. A few hundred milliseconds difference over 10 million look-ups is not very much. I'd be prepared to make the change in this case as the new code is no more complex than the old.

3. Testing locally I only see a few 10s of ms difference in the provided test case. That makes applying the patch a lot harder to justify.

4. Experimenting with different scopes (rather than pageContext) shows the new version is slower, not faster, for some. Any future proposal (and I am not sure it is worth pursuing)  needs to include a testcase that examines all scopes rather than one that appears to show the most favourable results.
Comment 3 Mark Thomas 2012-10-28 19:53:28 UTC
Created attachment 29514 [details]
Diff -u format of proposed change
Comment 4 Christopher Schultz 2012-10-31 17:52:57 UTC
It's worth noting that the proposed solution requires more heap space than the old solution. If this change is at the compiled-page level, it might not be a big deal. If a new HashMap gets created for every invocation of the JSP then it might be a performance problem under load due to the additional garbage generated.