You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2016/03/10 15:22:51 UTC
svn commit: r1734418 - in /tomcat/trunk:
java/javax/servlet/jsp/el/ScopedAttributeELResolver.java
java/org/apache/el/parser/AstIdentifier.java webapps/docs/changelog.xml
Author: markt
Date: Thu Mar 10 14:22:51 2016
New Revision: 1734418
URL: http://svn.apache.org/viewvc?rev=1734418&view=rev
Log:
Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=57583
Improve long standing performance issue with EL and undefined attributes.
Modified:
tomcat/trunk/java/javax/servlet/jsp/el/ScopedAttributeELResolver.java
tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java
tomcat/trunk/webapps/docs/changelog.xml
Modified: tomcat/trunk/java/javax/servlet/jsp/el/ScopedAttributeELResolver.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/javax/servlet/jsp/el/ScopedAttributeELResolver.java?rev=1734418&r1=1734417&r2=1734418&view=diff
==============================================================================
--- tomcat/trunk/java/javax/servlet/jsp/el/ScopedAttributeELResolver.java (original)
+++ tomcat/trunk/java/javax/servlet/jsp/el/ScopedAttributeELResolver.java Thu Mar 10 14:22:51 2016
@@ -35,6 +35,20 @@ import javax.servlet.jsp.PageContext;
*/
public class ScopedAttributeELResolver extends ELResolver {
+ // Indicates if a performance short-cut is available
+ private static final Class<?> AST_IDENTIFIER_KEY;
+
+ static {
+ Class<?> key = null;
+ try {
+ key = Class.forName("org.apache.el.parser.AstIdentifier");
+ } catch (Exception e) {
+ // Ignore: Expected if not running on Tomcat. Not a problem since
+ // this just allows a short-cut.
+ }
+ AST_IDENTIFIER_KEY = key;
+ }
+
@Override
public Object getValue(ELContext context, Object base, Object property) {
if (context == null) {
@@ -51,10 +65,26 @@ public class ScopedAttributeELResolver e
result = page.findAttribute(key);
if (result == null) {
+ boolean resolveClass = true;
+ // Performance short-cut available when running on Tomcat
+ if (AST_IDENTIFIER_KEY != null) {
+ // Tomcat will set this key to Boolean.TRUE if the
+ // identifier is a stand-alone identifier (i.e.
+ // identifier) rather than part of an AstValue (i.e.
+ // identifier.something). Imports do not need to be
+ // checked if this is a stand-alone identifier
+ Boolean value = (Boolean) context.getContext(AST_IDENTIFIER_KEY);
+ if (value != null && value.booleanValue()) {
+ resolveClass = false;
+ }
+ }
// This might be the name of an imported class
ImportHandler importHandler = context.getImportHandler();
if (importHandler != null) {
- Class<?> clazz = importHandler.resolveClass(key);
+ Class<?> clazz = null;
+ if (resolveClass) {
+ clazz = importHandler.resolveClass(key);
+ }
if (clazz != null) {
result = new ELClass(clazz);
}
Modified: tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java?rev=1734418&r1=1734417&r2=1734418&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java (original)
+++ tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java Thu Mar 10 14:22:51 2016
@@ -77,7 +77,27 @@ public final class AstIdentifier extends
// EL Resolvers
ctx.setPropertyResolved(false);
- Object result = ctx.getELResolver().getValue(ctx, null, this.image);
+ Object result;
+ /* Putting the Boolean into the ELContext is part of a performance
+ * optimisation for ScopedAttributeELResolver. When looking up "foo",
+ * the resolver can't differentiate between ${ foo } and ${ foo.bar }.
+ * This is important because the expensive class lookup only needs to
+ * be performed in the later case. This flag tells the resolver if the
+ * lookup can be skipped.
+ */
+ if (parent instanceof AstValue) {
+ ctx.putContext(this.getClass(), Boolean.FALSE);
+ } else {
+ ctx.putContext(this.getClass(), Boolean.TRUE);
+ }
+ try {
+ result = ctx.getELResolver().getValue(ctx, null, this.image);
+ } finally {
+ // Always reset the flag to false so the optimisation is not applied
+ // inappropriately
+ ctx.putContext(this.getClass(), Boolean.FALSE);
+ }
+
if (ctx.isPropertyResolved()) {
return result;
}
Modified: tomcat/trunk/webapps/docs/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1734418&r1=1734417&r2=1734418&view=diff
==============================================================================
--- tomcat/trunk/webapps/docs/changelog.xml (original)
+++ tomcat/trunk/webapps/docs/changelog.xml Thu Mar 10 14:22:51 2016
@@ -222,6 +222,12 @@
<update>
Update to the Eclipse JDT Compiler 4.5.1. (markt)
</update>
+ <fix>
+ <bug>57583</bug>: Improve the performance of
+ <code>javax.servlet.jsp.el.ScopedAttributeELResolver</code> when
+ resolving attributes that do not exist. This improvement only works when
+ Jasper is used with with Tomcat's EL implementation. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="WebSocket">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1734418 - in /tomcat/trunk: java/javax/servlet/jsp/el/ScopedAttributeELResolver.java
java/org/apache/el/parser/AstIdentifier.java webapps/docs/changelog.xml
Posted by Konstantin Kolinko <kn...@gmail.com>.
2016-03-10 19:45 GMT+03:00 Mark Thomas <ma...@apache.org>:
> On 10/03/2016 15:02, Konstantin Kolinko wrote:
>> 2016-03-10 17:22 GMT+03:00 <ma...@apache.org>:
>>> Author: markt
>>> Date: Thu Mar 10 14:22:51 2016
>>> New Revision: 1734418
>>>
>>> URL: http://svn.apache.org/viewvc?rev=1734418&view=rev
>>> Log:
>>> Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=57583
>>> Improve long standing performance issue with EL and undefined attributes.
>
> <snip/>
>
>>> Modified: tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java
>>> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java?rev=1734418&r1=1734417&r2=1734418&view=diff
>>> ==============================================================================
>>> --- tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java (original)
>>> +++ tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java Thu Mar 10 14:22:51 2016
>>> @@ -77,7 +77,27 @@ public final class AstIdentifier extends
>>>
>>> // EL Resolvers
>>> ctx.setPropertyResolved(false);
>>> - Object result = ctx.getELResolver().getValue(ctx, null, this.image);
>>> + Object result;
>>> + /* Putting the Boolean into the ELContext is part of a performance
>>> + * optimisation for ScopedAttributeELResolver. When looking up "foo",
>>> + * the resolver can't differentiate between ${ foo } and ${ foo.bar }.
>>> + * This is important because the expensive class lookup only needs to
>>> + * be performed in the later case. This flag tells the resolver if the
>>> + * lookup can be skipped.
>>> + */
>>> + if (parent instanceof AstValue) {
>>> + ctx.putContext(this.getClass(), Boolean.FALSE);
>>> + } else {
>>> + ctx.putContext(this.getClass(), Boolean.TRUE);
>>> + }
>>
>> Honestly, I do not understand the above if/else block.
>>
>> I do not see how the difference of "${ foo } vs ${ foo.bar }" maps
>> into "(parent instanceof AstValue)" check.
>>
>> What is the meaning of "parent" in Ast?
>
> It is the parent node of this node in the parse tree.
>
>> What possible values can it have?
>
> In theory, an instance of anything that extends SimpleNode. In practice,
> we are only interested in instances of AstValue since that is the type
> we will see when we need to check if the identifier represents a class
> and do the expensive class lookup.
>
>> Does this handle method calls, such as ${System.currentTimeMillis()} ?
>
> Yes.
OK. Understood. Reading the JJTree [1] and JavaCC docs was worth it.
[1] https://javacc.java.net/doc/JJTree.html
Summarizing the important bits.
The grammar - ELParser.jjt - says:
void Value() : {}
{
(ValuePrefix() (ValueSuffix())*) #Value(>1)
}
void ValuePrefix() : {}
{
Literal()
| NonLiteral()
}
void NonLiteral() : {}
{
LOOKAHEAD(5) LambdaExpressionOrInvocation()
| <LPAREN> Expression() <RPAREN>
| LOOKAHEAD((<IDENTIFIER> <COLON>)? <IDENTIFIER> <LPAREN>) Function()
| Identifier()
| LOOKAHEAD(3)SetData()
| ListData()
| MapData()
}
void Identifier() #Identifier : { Token t = null; }
{
t=<IDENTIFIER> { jjtThis.setImage(t.image); }
}
The following is important:
1. Each rule (aka non-terminal production) generates a same-named
method in ELParser.java class.
2. Each #Foo token produces code that creates an instance of AstFoo
node class. If there is no #Foo token, no node is generated.
This "no node is generated" is thanks to "NODE_DEFAULT_VOID=true;"
option specified at top of the grammar file.
It means that ValuePrefix() and NonLiteral() do not generate any Ast*
classes and AstIdentifier (generated by Identifier()) will be directly
nested into AstValue (generated by Value()).
BTW, ValueSuffix() generates either a AstDotSuffix or a
AstBracketSuffix node. It cannot generate an AstIdentifier.
4. The "#Value(>1)" token in Value() rule is a "ConditionalNode" (as
documented in [1]) and generates the node only if there is more than
one child.
It means that an AstValue node always has at least two children. It
means if there is only one token ("${foo}"), an AstValue node is not
generated, an AstIdentifier falls through.
Thus it is indeed the case of "foo" in ${foo.bar}, not a "bar" and not
a single ${foo}.
>>> + try {
>>> + result = ctx.getELResolver().getValue(ctx, null, this.image);
>>> + } finally {
>>> + // Always reset the flag to false so the optimisation is not applied
>>> + // inappropriately
>>> + ctx.putContext(this.getClass(), Boolean.FALSE);
>>> + }
>>> +
>>> if (ctx.isPropertyResolved()) {
>>> return result;
>>> }
>>
>> Below these lines the AstIdentifier does its own
>> ImportHandler.resolveClass(), resolveStatic() calls.
>>
>> This duplicates the work performed by ScopedAttributeELResolver and I
>> wonder whether it is necessary.
>
> Yes it is necessary.
>
> ScopedAttributeELResolver is defined to always resolve the property (so
> it has to do these lookups itself) but most resolvers don't.
1) Add a comment that the resolution is usually handled by
ScopedAttributeELResolver?
2) This code can benefit from the same optimization, skipping a
resolveClass() call.
3) I wonder whether we can reorder calls and call resolveStatic()
before resolveClass(). The resolveStatic() does not suffer from
synchronization (and it is usually noop, as there are no static
imports by default, and it is not easy to add ones).
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1734418 - in /tomcat/trunk:
java/javax/servlet/jsp/el/ScopedAttributeELResolver.java
java/org/apache/el/parser/AstIdentifier.java webapps/docs/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
On 10/03/2016 15:02, Konstantin Kolinko wrote:
> 2016-03-10 17:22 GMT+03:00 <ma...@apache.org>:
>> Author: markt
>> Date: Thu Mar 10 14:22:51 2016
>> New Revision: 1734418
>>
>> URL: http://svn.apache.org/viewvc?rev=1734418&view=rev
>> Log:
>> Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=57583
>> Improve long standing performance issue with EL and undefined attributes.
<snip/>
>> Modified: tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java
>> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java?rev=1734418&r1=1734417&r2=1734418&view=diff
>> ==============================================================================
>> --- tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java (original)
>> +++ tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java Thu Mar 10 14:22:51 2016
>> @@ -77,7 +77,27 @@ public final class AstIdentifier extends
>>
>> // EL Resolvers
>> ctx.setPropertyResolved(false);
>> - Object result = ctx.getELResolver().getValue(ctx, null, this.image);
>> + Object result;
>> + /* Putting the Boolean into the ELContext is part of a performance
>> + * optimisation for ScopedAttributeELResolver. When looking up "foo",
>> + * the resolver can't differentiate between ${ foo } and ${ foo.bar }.
>> + * This is important because the expensive class lookup only needs to
>> + * be performed in the later case. This flag tells the resolver if the
>> + * lookup can be skipped.
>> + */
>> + if (parent instanceof AstValue) {
>> + ctx.putContext(this.getClass(), Boolean.FALSE);
>> + } else {
>> + ctx.putContext(this.getClass(), Boolean.TRUE);
>> + }
>
> Honestly, I do not understand the above if/else block.
>
> I do not see how the difference of "${ foo } vs ${ foo.bar }" maps
> into "(parent instanceof AstValue)" check.
>
> What is the meaning of "parent" in Ast?
It is the parent node of this node in the parse tree.
> What possible values can it have?
In theory, an instance of anything that extends SimpleNode. In practice,
we are only interested in instances of AstValue since that is the type
we will see when we need to check if the identifier represents a class
and do the expensive class lookup.
> Does this handle method calls, such as ${System.currentTimeMillis()} ?
Yes.
>> + try {
>> + result = ctx.getELResolver().getValue(ctx, null, this.image);
>> + } finally {
>> + // Always reset the flag to false so the optimisation is not applied
>> + // inappropriately
>> + ctx.putContext(this.getClass(), Boolean.FALSE);
>> + }
>> +
>> if (ctx.isPropertyResolved()) {
>> return result;
>> }
>
> Below these lines the AstIdentifier does its own
> ImportHandler.resolveClass(), resolveStatic() calls.
>
> This duplicates the work performed by ScopedAttributeELResolver and I
> wonder whether it is necessary.
Yes it is necessary.
ScopedAttributeELResolver is defined to always resolve the property (so
it has to do these lookups itself) but most resolvers don't.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1734418 - in /tomcat/trunk: java/javax/servlet/jsp/el/ScopedAttributeELResolver.java
java/org/apache/el/parser/AstIdentifier.java webapps/docs/changelog.xml
Posted by Konstantin Kolinko <kn...@gmail.com>.
2016-03-10 17:22 GMT+03:00 <ma...@apache.org>:
> Author: markt
> Date: Thu Mar 10 14:22:51 2016
> New Revision: 1734418
>
> URL: http://svn.apache.org/viewvc?rev=1734418&view=rev
> Log:
> Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=57583
> Improve long standing performance issue with EL and undefined attributes.
>
> Modified:
> tomcat/trunk/java/javax/servlet/jsp/el/ScopedAttributeELResolver.java
> tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java
> tomcat/trunk/webapps/docs/changelog.xml
>
> Modified: tomcat/trunk/java/javax/servlet/jsp/el/ScopedAttributeELResolver.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/javax/servlet/jsp/el/ScopedAttributeELResolver.java?rev=1734418&r1=1734417&r2=1734418&view=diff
> ==============================================================================
> --- tomcat/trunk/java/javax/servlet/jsp/el/ScopedAttributeELResolver.java (original)
> +++ tomcat/trunk/java/javax/servlet/jsp/el/ScopedAttributeELResolver.java Thu Mar 10 14:22:51 2016
> @@ -35,6 +35,20 @@ import javax.servlet.jsp.PageContext;
> */
> public class ScopedAttributeELResolver extends ELResolver {
>
> + // Indicates if a performance short-cut is available
> + private static final Class<?> AST_IDENTIFIER_KEY;
> +
> + static {
> + Class<?> key = null;
> + try {
> + key = Class.forName("org.apache.el.parser.AstIdentifier");
> + } catch (Exception e) {
> + // Ignore: Expected if not running on Tomcat. Not a problem since
> + // this just allows a short-cut.
> + }
> + AST_IDENTIFIER_KEY = key;
> + }
> +
> @Override
> public Object getValue(ELContext context, Object base, Object property) {
> if (context == null) {
> @@ -51,10 +65,26 @@ public class ScopedAttributeELResolver e
> result = page.findAttribute(key);
>
> if (result == null) {
> + boolean resolveClass = true;
> + // Performance short-cut available when running on Tomcat
> + if (AST_IDENTIFIER_KEY != null) {
> + // Tomcat will set this key to Boolean.TRUE if the
> + // identifier is a stand-alone identifier (i.e.
> + // identifier) rather than part of an AstValue (i.e.
> + // identifier.something). Imports do not need to be
> + // checked if this is a stand-alone identifier
> + Boolean value = (Boolean) context.getContext(AST_IDENTIFIER_KEY);
> + if (value != null && value.booleanValue()) {
> + resolveClass = false;
> + }
> + }
> // This might be the name of an imported class
> ImportHandler importHandler = context.getImportHandler();
> if (importHandler != null) {
> - Class<?> clazz = importHandler.resolveClass(key);
> + Class<?> clazz = null;
> + if (resolveClass) {
> + clazz = importHandler.resolveClass(key);
> + }
> if (clazz != null) {
> result = new ELClass(clazz);
> }
>
> Modified: tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java?rev=1734418&r1=1734417&r2=1734418&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java (original)
> +++ tomcat/trunk/java/org/apache/el/parser/AstIdentifier.java Thu Mar 10 14:22:51 2016
> @@ -77,7 +77,27 @@ public final class AstIdentifier extends
>
> // EL Resolvers
> ctx.setPropertyResolved(false);
> - Object result = ctx.getELResolver().getValue(ctx, null, this.image);
> + Object result;
> + /* Putting the Boolean into the ELContext is part of a performance
> + * optimisation for ScopedAttributeELResolver. When looking up "foo",
> + * the resolver can't differentiate between ${ foo } and ${ foo.bar }.
> + * This is important because the expensive class lookup only needs to
> + * be performed in the later case. This flag tells the resolver if the
> + * lookup can be skipped.
> + */
> + if (parent instanceof AstValue) {
> + ctx.putContext(this.getClass(), Boolean.FALSE);
> + } else {
> + ctx.putContext(this.getClass(), Boolean.TRUE);
> + }
Honestly, I do not understand the above if/else block.
I do not see how the difference of "${ foo } vs ${ foo.bar }" maps
into "(parent instanceof AstValue)" check.
What is the meaning of "parent" in Ast?
What possible values can it have?
Does this handle method calls, such as ${System.currentTimeMillis()} ?
> + try {
> + result = ctx.getELResolver().getValue(ctx, null, this.image);
> + } finally {
> + // Always reset the flag to false so the optimisation is not applied
> + // inappropriately
> + ctx.putContext(this.getClass(), Boolean.FALSE);
> + }
> +
> if (ctx.isPropertyResolved()) {
> return result;
> }
Below these lines the AstIdentifier does its own
ImportHandler.resolveClass(), resolveStatic() calls.
This duplicates the work performed by ScopedAttributeELResolver and I
wonder whether it is necessary.
Those lines originate from
http://svn.apache.org/viewvc?diff_format=l&view=revision&revision=1504286
> Modified: tomcat/trunk/webapps/docs/changelog.xml
> URL: http://svn.apache.org/viewvc/tomcat/trunk/webapps/docs/changelog.xml?rev=1734418&r1=1734417&r2=1734418&view=diff
> ==============================================================================
> --- tomcat/trunk/webapps/docs/changelog.xml (original)
> +++ tomcat/trunk/webapps/docs/changelog.xml Thu Mar 10 14:22:51 2016
> @@ -222,6 +222,12 @@
> <update>
> Update to the Eclipse JDT Compiler 4.5.1. (markt)
> </update>
> + <fix>
> + <bug>57583</bug>: Improve the performance of
> + <code>javax.servlet.jsp.el.ScopedAttributeELResolver</code> when
> + resolving attributes that do not exist. This improvement only works when
> + Jasper is used with with Tomcat's EL implementation. (markt)
> + </fix>
> </changelog>
> </subsection>
> <subsection name="WebSocket">
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org