Bug 53869 - Performance tuning solution to resolve too many cascaded JspContextWrapper issue
Summary: Performance tuning solution to resolve too many cascaded JspContextWrapper issue
Status: RESOLVED FIXED
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-13 13:56 UTC by Sheldon Shao
Modified: 2013-01-25 12:08 UTC (History)
1 user (show)



Attachments
Patch for JspContextWrapper.java (4.51 KB, patch)
2012-09-13 13:56 UTC, Sheldon Shao
Details | Diff
Time taken percentage from one sample page (17.84 KB, image/png)
2012-09-13 13:57 UTC, Sheldon Shao
Details
Old JspContextWrapper (14.28 KB, text/plain)
2012-11-12 05:56 UTC, Sheldon Shao
Details
Test case comparison for performance tuning (6.46 KB, text/plain)
2012-11-12 05:56 UTC, Sheldon Shao
Details
Screenshot from JProfiler (87.88 KB, image/png)
2012-11-19 02:04 UTC, Sheldon Shao
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Sheldon Shao 2012-09-13 13:56:24 UTC
Created attachment 29372 [details]
Patch for JspContextWrapper.java

JspContextWrapper.getServletContext, JspContextWrapper.findAttribute and JspContextWrapper.getELContext take much time because there are many cascaded tag files. 

For example:
 <tag1>
 <tag2>
 <tag3>
 <tag4/>
 </tag3>
 </tag2>
 </tag1>

When calling JspContextWrapper(tag4).getServletContext from tag4, it will call JspContextWrapper(tag3).getServletContext --> JspContextWrapper(tag2).getServletContext --> JspContextWrapper(tag1).getServletContext --> PageContext.getServletContext.

If the root PageContext can be held in JspConextWrapper, those page,session,application scopes calling can go directy to this root PageContext.

Please check out the attached diff file (JspContextWrapper.diff) to get more detail about this change.
Comment 1 Sheldon Shao 2012-09-13 13:57:20 UTC
Created attachment 29373 [details]
Time taken percentage from one sample page
Comment 2 Jarek Gawor 2012-09-18 19:56:01 UTC
Can somebody please take a look at this patch?
Comment 3 Mark Thomas 2012-10-28 21:08:17 UTC
Please provide a test case that demonstrates the difference. The test case doesn't need to compare the old and new. As long as it generates timing information for the current implementation then both implementations may be tested in turn.
Comment 4 Sheldon Shao 2012-11-12 05:56:09 UTC
Created attachment 29586 [details]
Old JspContextWrapper
Comment 5 Sheldon Shao 2012-11-12 05:56:42 UTC
Created attachment 29587 [details]
Test case comparison for performance tuning
Comment 6 Sheldon Shao 2012-11-12 05:59:17 UTC
Hi Mark.

I added the test case.
Here is a result of this case:

Duration:422
DurationOld:748
Duration:203
DurationOld:717
Duration:187
DurationOld:702
Duration:187
DurationOld:686
Comment 7 Konstantin Kolinko 2012-11-12 08:56:28 UTC
1. This change in JspContextWrapper constructor means that it will create EL context regardless of whether it will be used or not:

+ this.elContext = rootJspCtxt.getELContext();

See PageContextImpl.getELContext(), JspApplicationContextImpl.createELContext(..).

The EL Context creation operation involves using a PrivilegedAction and firing an event through listeners. It is not cheap.

I think this change in behaviour is undesireable.


2. I do not expect much from your proposed optimization. Using a delegation is a common pattern. Replacing 4 chained redirects with 1 redirect is not much of a change.

3. If someone extends JspContextWrapper, its behaviour will be broken by this change. (Is it a concern? Would anyone extend it? Isn't it Jasper internals?)
Comment 8 Mark Thomas 2012-11-12 22:28:43 UTC
To get performance improvements above has required 5 levels of nesting and 10^7 iterations. That works out to around 50 nanoseconds per request.

I am far from convinced that the test case used to justify this changes represents a typical use case.

I share Konstantin's concerns regarding the ELContext.

I do not believe folks would be extending JspContextWrapper.

Without a better justification for this change, I am leaning towards WONTFIX.
Comment 9 Sheldon Shao 2012-11-19 02:04:08 UTC
Created attachment 29604 [details]
Screenshot from JProfiler

Actually, There are 8-level tag files cascaded in our page. You can refer this screenshot.
In this screenshot. Index.jsp was hit 80 times.  It has almost 10000 ELs in runtime. In this situation, JspContextWrapper.findAttribute becomes a big one of EL evaluation.
Comment 10 Sheldon Shao 2012-11-21 08:36:40 UTC
Actually, There are 8-level tag files cascaded in our page. You can refer this screenshot.
In this screenshot. Index.jsp was hit 80 times.  It has almost 10000 ELs in runtime. In this situation, JspContextWrapper.findAttribute becomes a big one of EL evaluation.
Comment 11 Mark Thomas 2013-01-25 12:02:25 UTC
The test case used to evaluate this patch is flawed. It fails to take account of the call to rootJspCtxt.getELContext() and does not compare the performance of the new implementation for a simple JSP page that does not use EL and does not have nested tags. When these additional issues are taken into consideration the proposed patch significantly impacts performance for these simple pages. Performance (as measured by this test case) is 1.5 to 2 times slower for simple pages.

However, it is not all doom and gloom. A small tweak to the proposed patch to use lazy instantiation for the EL context and performance is (as near as makes no difference) back to where it was for simple pages and still noticeably improved for the more complex case.
Comment 12 Mark Thomas 2013-01-25 12:08:43 UTC
The modified patch has been applied to trunk and will be included in 7.0.36 onwards.