Our large, high-traffic system has 1000+ jsps, and more than one class file is > 1MB (not my fault :) ). Examination of the generated code identified an isolated changed that will slightly reduce code size, although with near-zero runtime impact. The primary benefit to our system will be smaller class files, meaning lower aggregate code cache usage. Referencing generated code such as this: private boolean _jspx_meth_c_005fotherwise_005f30(javax.servlet.jsp.tagext.JspTag _jspx_th_c_005fchoose_005f34, javax.servlet.jsp.PageContext _jspx_page_context, int[] _jspx_push_body_count_c_005fforEach_005f21) throws java.lang.Throwable { ... _jsp_getInstanceManager().newInstance(_jspx_th_c_005fotherwise_005f30); try { ... } finally { org.apache.jasper.runtime.JspRuntimeLibrary.releaseTag(_jspx_th_c_005fotherwise_005f30, _jsp_getInstanceManager(), false); } return false; } The finally block triggers the JSPRuntimeLibrary.releaseTag() method: public static void releaseTag(Tag tag, InstanceManager instanceManager, boolean reused) { // Caller ensures pool is non-null if reuse is true if (!reused) { releaseTag(tag, instanceManager); } } The generated JSP code includes the hard-coded value "false", which short-circuits the releaseTag method and guarantees the line achieves nothing. One can argue that the JIT compiler will notice this and remove it; however, this line actually contains two method calls, JspRuntimeLibrary.releaseTag and _jsp_getInstanceManager(). public org.apache.tomcat.InstanceManager _jsp_getInstanceManager() { if (_jsp_instancemanager == null) { synchronized (this) { if (_jsp_instancemanager == null) { _jsp_instancemanager = org.apache.jasper.runtime.InstanceManagerFactory.getInstanceManager(getServletConfig()); } } } return _jsp_instancemanager; } This method call will almost always return the cached value and therefore be a very highly-optimized branch... however it must always be checked, and therefore always be executed. In addition, the JIT is not guaranteed to inline the releaseTag() and _jsp_getInstanceManager() methods, in which case it cannot detect the available optimizations. Put it all together and this is an unnecessary line of code, repeated over and over, that has a non-zero impact. This analysis applies only when the generated argument is "false", and this line is clearly required when set to "true". Preferred solution is to remove the line in all cases when the value is "false". If the try/finally can be simultaneously removed, that's even better, and removes yet more instructions.
(In reply to John Engebretson from comment #0) > public static void releaseTag(Tag tag, InstanceManager instanceManager, > boolean reused) { > // Caller ensures pool is non-null if reuse is true > if (!reused) { > releaseTag(tag, instanceManager); > } > } > > The generated JSP code includes the hard-coded value "false", which > short-circuits the releaseTag method and guarantees the line achieves > nothing. !false is ... false? And the body of the if-block is skipped? Maybe I'm misunderstanding what you are saying. > Put it all together and this is an unnecessary line of code, repeated over > and over, that has a non-zero impact. This analysis applies only when the > generated argument is "false", and this line is clearly required when set to > "true". > > Preferred solution is to remove the line in all cases when the value is > "false". If the try/finally can be simultaneously removed, that's even > better, and removes yet more instructions. I think I would agree, as long as the logic is flipped-around.
Yes, sorry for the error. :)
Do your tests show that suppressing these calls when the parameter will be *true* that it still gives a benefit?
My tests show a reduction in .class file size and a small reduction in the JVM's code cache, but that may be a margin-of-error situation. This is definitely not a high-impact change. I'm okay to close if you think the cost-benefit is poor.
Looking at the generated source and the code the generates it, I don't see why we need the try/catch/finally. Local testing indicates we can remove the try/catch/finally. We can also remove the 3-arg releaseTag() method and call the 2-arg directly. I have a few more tests to run but should be in a position to commit the fix tomorrow if the tests go well.
I'm fairly sure that try/catch/finally don't add any overhead in terms of method-code-bytes. It expands the size of the exception-handling table, but it doesn't reduce code size. Perhaps overall .class file size, sure. Usually complaints about JSPs are due to a single method becoming too long to fit into the .class file format. In your case, you were concerned about code cache usage which, I think, will still be the same with or without the try/catch/finally blocks. I'm not saying there isn't a change worth making here; quite the contrary. Any simplification of any code is always a win IMHO. I just want to make sure to manage expectations of what any change will actually accomplish.
> I just want to make sure to manage expectations of what any change will actually accomplish. Understood, agreed, and appreciated. :)
Fixed in: - 11.0.x for 11.0.0 onwards - 10.1.x for 10.1.31 onwards - 9.0.x for 9.0.96 onwards
I've got a custom tag library which has worked literally for decades in Tomcat. Actually it worked from v4 up to v9.0.95. Now in v9.0.96 which contains this change it's not working correctly any longer. The problem seems to be that your change is calling release() on cached tags and reuses the tag after this. To all what I know this is breaking the convention that release() should be called only when the JSP is definately done with the tag. It's the official place to release resources before the tag is later garbage collected.
Please do not reopen another BZ, even if it is related to your issue. The issue you are describing has already been fixed (BZ 69399) and will be fixed in 9.0.97.