jakartaee
Performance issues from repeating ImportHandler.resolveClass calls for null attributes
This is an issue like https://bz.apache.org/bugzilla/show_bug.cgi?id=57583 that I currently reproduce on EAP 7.4 (using pages 2.3.x) and EAP 8.0 (using pages 3.1.x). An easy stress test is a jsp referring to many null attributes:
<td><p style="color: red">${error1}</p></td> \<td><p style="color: red">${error2}</p></td> ... \<td><p style="color: red">${error999}</p></td> \<td><p style="color: red">${error1000}</p></td>
This performs much worse (10-20x longer) on EAP 7/8 where the ImportHandler is now used compared to EAP 6 that did not . Requests spend all their time in Class.forName calls from the ImportHandler:
"default task-10" #157 prio=5 os_prio=0 cpu=2098.07ms elapsed=21.51s tid=0x0000562376ddf800 nid=0x37ded runnable [0x00007f15ab838000] java.lang.Thread.State: RUNNABLE at java.lang.Class.forName0([email protected]/Native Method) at java.lang.Class.forName([email protected]/Class.java:398) at jakarta.el.ImportHandler.getClassFor(ImportHandler.java:169) at jakarta.el.ImportHandler.resolveClassFor(ImportHandler.java:145) at jakarta.el.ImportHandler.resolveClass(ImportHandler.java:109) at jakarta.servlet.jsp.el.ImportELResolver.getValue(ImportELResolver.java:62) at org.apache.jasper.el.JasperELResolver.getValue(JasperELResolver.java:123) at org.glassfish.expressly.parser.AstIdentifier.getValue(AstIdentifier.java:97) at org.glassfish.expressly.ValueExpressionImpl.getValue(ValueExpressionImpl.java:138) at org.apache.jasper.runtime.PageContextImpl.proprietaryEvaluate(PageContextImpl.java:925) at org.apache.jsp.test_jsp._jspService(test_jsp.java:1077)
So could some form of caching be considered here for a performance improvement?
@markt-asf can you check this to see if it can be merged, this issue was addressed in Tomcat https://bz.apache.org/bugzilla/show_bug.cgi?id=57583 https://github.com/apache/tomcat80/commit/63665340bb6fb123d93627cf4eac62dadd1545ab https://github.com/apache/tomcat80/commit/b69ed7e5f1bfd20ac09f8ea70e77731e1f1fcb44
There are multiple issues here, not just related to the immediate issue.
We can't normally just copy code from Tomcat into this project. While the licenses are compatible, additional legal hoop jumping is required to do that. However, we can bypass that in this case since I am the original author and copyright holder for the changes in Tomcat so I could opt to donate those changes to this project under my ECA which would avoid the hoop jumping necessary to copy them from Tomcat. Same end result. Simpler route to get there.
However...
The first patch is for the EL API and, as far as I can tell, an equivalent change has already been made to to the EL API. That change was present in the initial contribution to Eclipse so that goes back as far as at least Java EE 8 - possibly earlier.
The second patch is more problematic. It requires changes to both the JSP API and EL Implementation. While that sort of coupling was easy to implement in Tomcat, it would take rather more discussion and coordination to do it here. I'm not even sure it is the right solution for the general case. Either way, a patch along those lines is highly unlikely to make it into Jakarta EE 11 as EL needs to start release review towards the end of this week.
My preference would be for a solution wholly within the JSP API. I suspect that won't be possible.
I see Arjan has already commented on #246/#247. I'm not currently convinced either of those will help much but I'll look at them in more detail tomorrow.
I have created https://github.com/jakartaee/expression-language/issues/211 to track the addition of caching of null return values in the ImportHandler. We already have caching there and don't need two layers of caching.
I will be closing #246 and #247 that adding caching to ImportELResolver.
From memory the caching (including null caching) help a bit with Tomcat but what really made the difference was skipping the ImportHandler altogether for identifiers. With that in mind, I'd like to propose the following solution:
- define an
ELContextcontext object (in the EL Spec) that, if present and set toBoolean.TRUEindicates to anyELResolverinstances that a look-up via theImportHandleris not required for this expression - require in the EL specification that this new context object must be set when evaluating lone identifiers
- update the
ScopedAttributeELResolverin this specification to honour this new context object
The above essentially formalises the approach that Tomcat has been using successfully for a number of years. My only dislike of this approach is that it requires changes in the EL API, JSP API and all EL implementations. If anyone can see a less invasive way to achieve the same result please so speak up.