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-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
