You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by bu...@apache.org on 2018/06/14 02:55:29 UTC
[Bug 62453] New: Tomcat tries to resolve uninitialized tag
attributes in EL as java class names and this behavior causes performance
problems.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62453
Bug ID: 62453
Summary: Tomcat tries to resolve uninitialized tag attributes
in EL as java class names and this behavior causes
performance problems.
Product: Tomcat 9
Version: 9.0.8
Hardware: PC
Status: NEW
Severity: normal
Priority: P2
Component: EL
Assignee: dev@tomcat.apache.org
Reporter: katsuta_k@worksap.co.jp
Target Milestone: -----
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
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 62453] Tomcat tries to resolve uninitialized tag attributes in
EL as java class names and this behavior causes performance problems.
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62453
Mark Thomas <ma...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #35975|0 |1
is obsolete| |
--- Comment #8 from Mark Thomas <ma...@apache.org> ---
Created attachment 35990
--> https://bz.apache.org/bugzilla/attachment.cgi?id=35990&action=edit
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 62453] Tomcat tries to resolve uninitialized tag attributes in
EL as java class names and this behavior causes performance problems.
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62453
--- Comment #9 from Konstantin Kolinko <kn...@gmail.com> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 62453] Tomcat tries to resolve uninitialized tag attributes in
EL as java class names and this behavior causes performance problems.
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62453
Mark Thomas <ma...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
OS| |All
--- Comment #1 from Mark Thomas <ma...@apache.org> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 62453] Tomcat tries to resolve uninitialized tag attributes in
EL as java class names and this behavior causes performance problems.
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62453
--- Comment #4 from Konstantin Kolinko <kn...@gmail.com> ---
(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.)
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 62453] Tomcat tries to resolve uninitialized tag attributes in
EL as java class names and this behavior causes performance problems.
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62453
--- Comment #11 from Mark Thomas <ma...@apache.org> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 62453] Tomcat tries to resolve uninitialized tag attributes in
EL as java class names and this behavior causes performance problems.
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62453
--- Comment #2 from Mark Thomas <ma...@apache.org> ---
Created attachment 35975
--> https://bz.apache.org/bugzilla/attachment.cgi?id=35975&action=edit
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 62453] Tomcat tries to resolve uninitialized tag attributes in
EL as java class names and this behavior causes performance problems.
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62453
--- Comment #7 from Konstantin Kolinko <kn...@gmail.com> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 62453] Tomcat tries to resolve uninitialized tag attributes in
EL as java class names and this behavior causes performance problems.
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62453
Mark Thomas <ma...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |RESOLVED
Resolution|--- |FIXED
--- Comment #12 from Mark Thomas <ma...@apache.org> ---
Fixed in:
- trunk for 9.0.11 onwards
- 8.5.x for 8.5.33 onwards
Note: 7.0.x and earlier not affected.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 62453] Tomcat tries to resolve uninitialized tag attributes in
EL as java class names and this behavior causes performance problems.
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62453
Mark Thomas <ma...@apache.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Severity|normal |enhancement
--- Comment #3 from Mark Thomas <ma...@apache.org> ---
Moving this to an enhancement request as this is a performance optimisation not
incorrect behaviour.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 62453] Tomcat tries to resolve uninitialized tag attributes in
EL as java class names and this behavior causes performance problems.
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62453
--- Comment #5 from Rainer Jung <ra...@kippdata.de> ---
(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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 62453] Tomcat tries to resolve uninitialized tag attributes in
EL as java class names and this behavior causes performance problems.
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62453
--- Comment #6 from Mark Thomas <ma...@apache.org> ---
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.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
[Bug 62453] Tomcat tries to resolve uninitialized tag attributes in
EL as java class names and this behavior causes performance problems.
Posted by bu...@apache.org.
https://bz.apache.org/bugzilla/show_bug.cgi?id=62453
--- Comment #10 from Remy Maucherat <re...@apache.org> ---
That sounds like the best hack ever.
--
You are receiving this mail because:
You are the assignee for the bug.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org