Bug 62453 - Tomcat tries to resolve uninitialized tag attributes in EL as java class names and this behavior causes performance problems.
Summary: Tomcat tries to resolve uninitialized tag attributes in EL as java class name...
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 9
Classification: Unclassified
Component: EL (show other bugs)
Version: 9.0.8
Hardware: PC All
: P2 enhancement (vote)
Target Milestone: -----
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-06-14 02:55 UTC by katsuta_k
Modified: 2018-07-11 16:03 UTC (History)
0 users



Attachments
Patch for potential optimisation (20.72 KB, patch)
2018-06-19 19:11 UTC, Mark Thomas
Details | Diff
Updated patch with test cases to check class lists (28.31 KB, patch)
2018-06-26 11:40 UTC, Mark Thomas
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description katsuta_k 2018-06-14 02:55:29 UTC
an example of tag file

/WEB-INF/tags/sample.tag
{{{
<%@ taglib prefix="c" uri="http://java.sun.com/jsp/jstl/core" %>
<%@ attribute name="foo" required="true" %>
<%@ attribute name="bar" %>
<%@ attribute name="baz" %>

<div>
  <div>foo:  ${foo.toString()}</div>
  <div>bar:  ${bar.toString()}</div>
  <div>baz:  ${baz.toString()}</div>
</div>
}}}

an example of jsp file calling tag without "bar" and "baz" attributes 
{{{
<%@ taglib prefix="tags" tagdir="/WEB-INF/tags" %>

<html>
  <body>
    <h2>Example of Uninitialized Tag Attributes</h2>
    <tags:sample foo="FOO"/>
  </body>
</html>
}}}

I found that the uninitialized attributes were NOT set into context in Java files generated from tag file.
Besides, when the uninitialized attributes were called in EL, ScopedAttributeELResolver tries to resolve attributes name as java class names.

ref. getValue method in http://svn.apache.org/repos/asf/tomcat/tags/TOMCAT_9_0_8/java/javax/servlet/jsp/el/ScopedAttributeELResolver.java 



I have already confirmed that the uninitialized attributes actually affect performance.
I got jfr to compare between there are uninitialized attributes or not.
It was confirmed that lock wait occurs in java.util.jar.JarFile class and sun.misc.URLClassPath class only when there are uninitialized attributes in tag.


the related bug

Performance issue evaluating EL in custom tags (tagx) due to inefficient calls to java.lang.Class.forName()
https://bz.apache.org/bugzilla/show_bug.cgi?id=57583
Comment 1 Mark Thomas 2018-06-19 16:37:10 UTC
When Tomcat parses "baz.toString()" Tomcat has no way to determine if "baz" is a class name and "toString" is the name of a static method or if "baz" is an attribute and "toString" is an instance method. Tomcat has to do the class lookup to find out.

The optimisation that was implemented in bug 57583 does not apply in this case.

In this simple case removing the "toString()" calls would improve performance (it would allow the optimisation from bug 57583 to work) as there is an implicit toString() in the EL evaluation.

In a more complex case the only way for an application to ensure it avoids the expensive lookup is to use an explicit scope with the attributes. i.e.:

<div>foo:  ${pageScope.foo.toString()}</div>

etc.

Generally, explicitly stating the scope is the best approach. It allows faster execution and does not depend on any container specific optimisation.

I've been taking a look at the code to see if there is anything further that can be done in terms of optimisation. There are additional special cases we could handle in a similar manner to bug 57583 but every special case adds complexity and maintenance overhead. I'm not sure at this point if the additional benefits are worth the cost. I need to run a few tests.
Comment 2 Mark Thomas 2018-06-19 19:11:37 UTC
Created attachment 35975 [details]
Patch for potential optimisation

Whilst I remember, using explicit imports rather than wildcard imports will improve performance as well.

I am attaching a patch the implements an optimisation to reduce the need to class loader lookups for the standard packages imported by all JSPs and tag files.

The overall performance improvement is modest (about a factor of 3 for the resolution). I'm not convinced that the performance improvement justifies the ongoing maintenance. The classes need to be kept in sync or things could break. That is a significant negative in my view.

At the moment, using explicit scopes looks like a much better solution.
Comment 3 Mark Thomas 2018-06-19 19:12:18 UTC
Moving this to an enhancement request as this is a performance optimisation not incorrect behaviour.
Comment 4 Konstantin Kolinko 2018-06-20 11:22:24 UTC
(In reply to Mark Thomas from comment #2)
> Created attachment 35975 [details]
> Patch for potential optimisation

> standardPackages.put("javax.jsp.servlet", servletJspClassNames);

A typo. The package name should be "javax.servlet.jsp".

> // Standard package where we know all the class names

New classes can be added to java.lang in future versions of Java. I see from a comment that you worked from Java 11 EA javadoc,  but it is still fragile.

(I wonder if it is possible to do a consistency check programmatically - similar to OpenSSL cipher suite tests that we have.  The crucial thing is to get a list of classes in a package. I do not see API for this besides ClassLoader.getResources(). The java.lang.Package class does not have a method to get a list of classes contained in that package.)
Comment 5 Rainer Jung 2018-06-20 11:46:41 UTC
(In reply to Konstantin Kolinko from comment #4)
> (I wonder if it is possible to do a consistency check programmatically -
> similar to OpenSSL cipher suite tests that we have.  The crucial thing is to
> get a list of classes in a package. I do not see API for this besides
> ClassLoader.getResources(). The java.lang.Package class does not have a
> method to get a list of classes contained in that package.)

May the jar file names containing those packages are reasonabe stable and one could instead open them as zip files and list the class files they contain with the wanted packages? Just a quick thought.
Comment 6 Mark Thomas 2018-06-20 12:19:14 UTC
That typo is significant. With the typo fixed the performance difference is 4.6s vs ~50ms - two orders of magnitude difference. I think that probably changes the balance.

Future additions to java.lang concern me too. I took a quick look at automatic generation / tracking but opted for the manual version (from the Javadoc as it happens) for this patch.

Given the performance improvement, some more thought on how to maintain this reliably is called for.

The JAR file name could work - as long as rt.jar is in an expected location. I think the location is stable relative to JAVA_HOME. We'd still need to filter public / non-public classes but that should be easy enough.
Comment 7 Konstantin Kolinko 2018-06-20 12:54:06 UTC
I think that the feature of JSP Tag files reported in this BZ ticket is rather confusing. In my opinion, if I have a tag file that declares 

<%@ attribute name="baz" %>

then evaluating "${baz}" should return the value of that attribute that I declared. If the tag file was called without any value for the attribute, return null, without falling back to outer scopes (requestScope.baz, sessionScope.baz, applicationScope.baz).

This fallback behaviour is rather odd.


Specification text, JSP2.3MR.pdf - "JSP.8.3 Semantics of Tag Files" page 209/594:
[quote]
For each attribute declared and specified, a page-scoped variable must be created
in the page scope of the JSP Context Wrapper, unless the attribute is a deferred
value or a deferred method, in which case the VariableMapper obtained
from the ELContext in the current pageContext is used to map the deferred
expression to the attribute name. The name of the variable must be the same
as the declared attribute name. The value of the variable must be the value of
the attribute passed in during invocation. For each attribute declared as optional
and not specified, no variable is created. [...]
[/quote]


If we deviate from the specification, and disable the fallback to outer scopes, technically it can be done in Jasper, without changing the EL implementation:

When EL evaluates an identifier, it consults the VariableMapper first, before looking into `ELResolver`s.

So if one registers such attribute in the VariableMapper as a ValueExpression that evaluates to the constant value of null, the fallback to `ELResolver`s will not happen.
Comment 8 Mark Thomas 2018-06-26 11:40:05 UTC
Created attachment 35990 [details]
Updated patch with test cases to check class lists

Given the general approach of Java EE (which I assume will continue in Jakarta EE) to backwards compatibility, I don't think the behaviour required by the specification is going to change.

The performance implications of adding imports to EL were not realised at the time. They are now part of the spec and, for the same reason as above, I don't think that will change.

Using a non-specification compliant work-around is an option. The work-around proposed looks reasonable to me. However, experience tells me that it is hard to judge what proportion of users would benefit from it vs would see failures with it. Clearly, it would be optional. The question would be whether to enable it by default or not.

One of the benefits of the Java 9 module system is that it is possible to enumerate all the classes in java.lang. I have attached an updated test case that does this and also checks the Servlet/JSP spec classes too.

The patch is undoubtedly a hack but with the test cases ongoing maintenance is minimal and users are saved the hassle of having to go through their code and add explicit scopes everywhere.

Unless there are objections, I plan to commit this to trunk.
Comment 9 Konstantin Kolinko 2018-07-06 13:35:16 UTC
One more idea:
The public classes in Java Platform API all follow the naming conventions and start with an uppercase character A-Z.

Attribute names usually start with a lowercase character.
Comment 10 Remy Maucherat 2018-07-06 13:52:01 UTC
That sounds like the best hack ever.
Comment 11 Mark Thomas 2018-07-09 20:42:24 UTC
That is a neat hack. The downside is that it isn't as complete a fix. It wouldn't help users where the attribute name started with an upper case letter although I suspect this is fairly rare.

Given that we have a patch for a more complete fix and that the code that runs on every lookup isn't that different, I'm leaning towards applying the attached patch.
Comment 12 Mark Thomas 2018-07-11 16:03:32 UTC
Fixed in:
- trunk for 9.0.11 onwards
- 8.5.x for 8.5.33 onwards

Note: 7.0.x and earlier not affected.