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.
Created attachment 29373 [details] Time taken percentage from one sample page
Can somebody please take a look at this patch?
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.
Created attachment 29586 [details] Old JspContextWrapper
Created attachment 29587 [details] Test case comparison for performance tuning
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
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?)
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.
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.
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.
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.
The modified patch has been applied to trunk and will be included in 7.0.36 onwards.