From a659fce8ff4140e4ffdd86f7d5f7ec487f21f86e Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Mon, 29 Apr 2013 13:21:17 +0200 Subject: [PATCH] 8013419: Streamline handling of with and eval Reviewed-by: hannesw, lagergren --- .../jdk/nashorn/internal/codegen/Attr.java | 62 ++++++- .../internal/codegen/CodeGenerator.java | 172 ++++++++++++++---- .../internal/codegen/MethodEmitter.java | 11 +- .../src/jdk/nashorn/internal/ir/CallNode.java | 16 +- .../jdk/nashorn/internal/ir/FunctionNode.java | 41 +---- .../nashorn/internal/ir/LexicalContext.java | 8 - .../jdk/nashorn/internal/parser/Parser.java | 21 +-- .../linker/NashornCallSiteDescriptor.java | 5 +- 8 files changed, 219 insertions(+), 117 deletions(-) diff --git a/nashorn/src/jdk/nashorn/internal/codegen/Attr.java b/nashorn/src/jdk/nashorn/internal/codegen/Attr.java index 41da3cf2963..9b17c47f1a5 100644 --- a/nashorn/src/jdk/nashorn/internal/codegen/Attr.java +++ b/nashorn/src/jdk/nashorn/internal/codegen/Attr.java @@ -79,6 +79,7 @@ import jdk.nashorn.internal.ir.TernaryNode; import jdk.nashorn.internal.ir.TryNode; import jdk.nashorn.internal.ir.UnaryNode; import jdk.nashorn.internal.ir.VarNode; +import jdk.nashorn.internal.ir.WithNode; import jdk.nashorn.internal.ir.visitor.NodeOperatorVisitor; import jdk.nashorn.internal.ir.visitor.NodeVisitor; import jdk.nashorn.internal.parser.TokenType; @@ -495,10 +496,8 @@ final class Attr extends NodeOperatorVisitor { } identNode.setSymbol(symbol); - // non-local: we need to put symbol in scope (if it isn't already) - if (!isLocal(lc.getCurrentFunction(), symbol) && !symbol.isScope()) { - Symbol.setSymbolIsScope(lc, symbol); - } + // if symbol is non-local or we're in a with block, we need to put symbol in scope (if it isn't already) + maybeForceScope(symbol); } else { LOG.info("No symbol exists. Declare undefined: ", symbol); symbol = defineSymbol(block, name, IS_GLOBAL, identNode); @@ -520,6 +519,50 @@ final class Attr extends NodeOperatorVisitor { return false; } + /** + * If the symbol isn't already a scope symbol, and it is either not local to the current function, or it is being + * referenced from within a with block, we force it to be a scope symbol. + * @param symbol the symbol that might be scoped + */ + private void maybeForceScope(final Symbol symbol) { + if(!symbol.isScope() && symbolNeedsToBeScope(symbol)) { + Symbol.setSymbolIsScope(getLexicalContext(), symbol); + } + } + + private boolean symbolNeedsToBeScope(Symbol symbol) { + if(symbol.isThis() || symbol.isInternal()) { + return false; + } + boolean previousWasBlock = false; + for(final Iterator it = getLexicalContext().getAllNodes(); it.hasNext();) { + final LexicalContextNode node = it.next(); + if(node instanceof FunctionNode) { + // We reached the function boundary without seeing a definition for the symbol - it needs to be in + // scope. + return true; + } else if(node instanceof WithNode) { + if(previousWasBlock) { + // We reached a WithNode; the symbol must be scoped. Note that if the WithNode was not immediately + // preceded by a block, this means we're currently processing its expression, not its body, + // therefore it doesn't count. + return true; + } + previousWasBlock = false; + } else if(node instanceof Block) { + if(((Block)node).getExistingSymbol(symbol.getName()) == symbol) { + // We reached the block that defines the symbol without reaching either the function boundary, or a + // WithNode. The symbol need not be scoped. + return false; + } + previousWasBlock = true; + } else { + previousWasBlock = false; + } + } + throw new AssertionError(); + } + private void setBlockScope(final String name, final Symbol symbol) { assert symbol != null; if (symbol.isGlobal()) { @@ -963,18 +1006,17 @@ final class Attr extends NodeOperatorVisitor { final Node lhs = binaryNode.lhs(); if (lhs instanceof IdentNode) { - final LexicalContext lc = getLexicalContext(); - final Block block = lc.getCurrentBlock(); - final IdentNode ident = (IdentNode)lhs; - final String name = ident.getName(); + final Block block = getLexicalContext().getCurrentBlock(); + final IdentNode ident = (IdentNode)lhs; + final String name = ident.getName(); Symbol symbol = findSymbol(block, name); if (symbol == null) { symbol = defineSymbol(block, name, IS_GLOBAL, ident); binaryNode.setSymbol(symbol); - } else if (!isLocal(lc.getCurrentFunction(), symbol)) { - Symbol.setSymbolIsScope(lc, symbol); + } else { + maybeForceScope(symbol); } addLocalDef(name); diff --git a/nashorn/src/jdk/nashorn/internal/codegen/CodeGenerator.java b/nashorn/src/jdk/nashorn/internal/codegen/CodeGenerator.java index e8ca8c3a910..5d4b8f9846c 100644 --- a/nashorn/src/jdk/nashorn/internal/codegen/CodeGenerator.java +++ b/nashorn/src/jdk/nashorn/internal/codegen/CodeGenerator.java @@ -87,6 +87,7 @@ import jdk.nashorn.internal.ir.IdentNode; import jdk.nashorn.internal.ir.IfNode; import jdk.nashorn.internal.ir.IndexNode; import jdk.nashorn.internal.ir.LexicalContext; +import jdk.nashorn.internal.ir.LexicalContextNode; import jdk.nashorn.internal.ir.LineNumberNode; import jdk.nashorn.internal.ir.LiteralNode; import jdk.nashorn.internal.ir.LiteralNode.ArrayLiteralNode; @@ -201,6 +202,7 @@ final class CodeGenerator extends NodeOperatorVisitor { * @param compiler */ CodeGenerator(final Compiler compiler) { + super(new DynamicScopeTrackingLexicalContext()); this.compiler = compiler; this.callSiteFlags = compiler.getEnv()._callsite_flags; } @@ -284,6 +286,53 @@ final class CodeGenerator extends NodeOperatorVisitor { } } + /** + * A lexical context that also tracks if we have any dynamic scopes in the context. Such scopes can have new + * variables introduced into them at run time - a with block or a function directly containing an eval call. + */ + private static class DynamicScopeTrackingLexicalContext extends LexicalContext { + int dynamicScopeCount = 0; + + @Override + public T push(T node) { + if(isDynamicScopeBoundary(node)) { + ++dynamicScopeCount; + } + return super.push(node); + } + + @Override + public T pop(T node) { + final T popped = super.pop(node); + if(isDynamicScopeBoundary(popped)) { + --dynamicScopeCount; + } + return popped; + } + + private boolean isDynamicScopeBoundary(LexicalContextNode node) { + if(node instanceof Block) { + // Block's immediate parent is a with node. Note we aren't testing for a WithNode, as that'd capture + // processing of WithNode.expression too, but it should be unaffected. + return !isEmpty() && peek() instanceof WithNode; + } else if(node instanceof FunctionNode) { + // Function has a direct eval in it (so a top-level "var ..." in the eval code can introduce a new + // variable into the function's scope), and it isn't strict (as evals in strict functions get an + // isolated scope). + return isFunctionDynamicScope((FunctionNode)node); + } + return false; + } + } + + boolean inDynamicScope() { + return ((DynamicScopeTrackingLexicalContext)getLexicalContext()).dynamicScopeCount > 0; + } + + static boolean isFunctionDynamicScope(FunctionNode fn) { + return fn.hasEval() && !fn.isStrict(); + } + /** * Check if this symbol can be accessed directly with a putfield or getfield or dynamic load * @@ -291,17 +340,46 @@ final class CodeGenerator extends NodeOperatorVisitor { * @return true if fast scope */ private boolean isFastScope(final Symbol symbol) { - if (!symbol.isScope() || !getLexicalContext().getDefiningBlock(symbol).needsScope()) { + if(!symbol.isScope()) { return false; } - // Allow fast scope access if no function contains with or eval - for (final Iterator it = getLexicalContext().getFunctions(); it.hasNext();) { - final FunctionNode func = it.next(); - if (func.hasWith() || func.hasEval()) { - return false; + final LexicalContext lc = getLexicalContext(); + if(!inDynamicScope()) { + // If there's no with or eval in context, and the symbol is marked as scoped, it is fast scoped. Such a + // symbol must either be global, or its defining block must need scope. + assert symbol.isGlobal() || lc.getDefiningBlock(symbol).needsScope() : symbol.getName(); + return true; + } + if(symbol.isGlobal()) { + // Shortcut: if there's a with or eval in context, globals can't be fast scoped + return false; + } + // Otherwise, check if there's a dynamic scope between use of the symbol and its definition + final String name = symbol.getName(); + boolean previousWasBlock = false; + for (final Iterator it = lc.getAllNodes(); it.hasNext();) { + final LexicalContextNode node = it.next(); + if(node instanceof Block) { + // If this block defines the symbol, then we can fast scope the symbol. + final Block block = (Block)node; + if(block.getExistingSymbol(name) == symbol) { + assert block.needsScope(); + return true; + } + previousWasBlock = true; + } else { + if((node instanceof WithNode && previousWasBlock) || (node instanceof FunctionNode && isFunctionDynamicScope((FunctionNode)node))) { + // If we hit a scope that can have symbols introduced into it at run time before finding the defining + // block, the symbol can't be fast scoped. A WithNode only counts if we've immediately seen a block + // before - its block. Otherwise, we are currently processing the WithNode's expression, and that's + // obviously not subjected to introducing new symbols. + return false; + } + previousWasBlock = false; } } - return true; + // Should've found the symbol defined in a block + throw new AssertionError(); } private MethodEmitter loadSharedScopeVar(final Type valueType, final Symbol symbol, final int flags) { @@ -671,7 +749,7 @@ final class CodeGenerator extends NodeOperatorVisitor { evalCall(node, flags); } else if (useCount <= SharedScopeCall.FAST_SCOPE_CALL_THRESHOLD || (!isFastScope(symbol) && useCount <= SharedScopeCall.SLOW_SCOPE_CALL_THRESHOLD) - || callNode.inWithBlock()) { + || CodeGenerator.this.inDynamicScope()) { scopeCall(node, flags); } else { sharedScopeCall(node, flags); @@ -2114,9 +2192,11 @@ final class CodeGenerator extends NodeOperatorVisitor { } private void closeWith() { - method.loadCompilerConstant(SCOPE); - method.invoke(ScriptRuntime.CLOSE_WITH); - method.storeCompilerConstant(SCOPE); + if(method.hasScope()) { + method.loadCompilerConstant(SCOPE); + method.invoke(ScriptRuntime.CLOSE_WITH); + method.storeCompilerConstant(SCOPE); + } } @Override @@ -2124,38 +2204,58 @@ final class CodeGenerator extends NodeOperatorVisitor { final Node expression = withNode.getExpression(); final Node body = withNode.getBody(); - final Label tryLabel = new Label("with_try"); - final Label endLabel = new Label("with_end"); - final Label catchLabel = new Label("with_catch"); - final Label exitLabel = new Label("with_exit"); + // It is possible to have a "pathological" case where the with block does not reference *any* identifiers. It's + // pointless, but legal. In that case, if nothing else in the method forced the assignment of a slot to the + // scope object, its' possible that it won't have a slot assigned. In this case we'll only evaluate expression + // for its side effect and visit the body, and not bother opening and closing a WithObject. + final boolean hasScope = method.hasScope(); - method.label(tryLabel); - - method.loadCompilerConstant(SCOPE); - load(expression); - - assert expression.getType().isObject() : "with expression needs to be object: " + expression; - - method.invoke(ScriptRuntime.OPEN_WITH); - method.storeCompilerConstant(SCOPE); - - body.accept(this); - - if (!body.isTerminal()) { - closeWith(); - method._goto(exitLabel); + final Label tryLabel; + if(hasScope) { + tryLabel = new Label("with_try"); + method.label(tryLabel); + method.loadCompilerConstant(SCOPE); + } else { + tryLabel = null; } - method.label(endLabel); + load(expression); + assert expression.getType().isObject() : "with expression needs to be object: " + expression; - method._catch(catchLabel); - closeWith(); - method.athrow(); + if(hasScope) { + // Construct a WithObject if we have a scope + method.invoke(ScriptRuntime.OPEN_WITH); + method.storeCompilerConstant(SCOPE); + } else { + // We just loaded the expression for its side effect; discard it + method.pop(); + } - method.label(exitLabel); - method._try(tryLabel, endLabel, catchLabel); + // Always process body + body.accept(this); + if(hasScope) { + // Ensure we always close the WithObject + final Label endLabel = new Label("with_end"); + final Label catchLabel = new Label("with_catch"); + final Label exitLabel = new Label("with_exit"); + + if (!body.isTerminal()) { + closeWith(); + method._goto(exitLabel); + } + + method.label(endLabel); + + method._catch(catchLabel); + closeWith(); + method.athrow(); + + method.label(exitLabel); + + method._try(tryLabel, endLabel, catchLabel); + } return false; } diff --git a/nashorn/src/jdk/nashorn/internal/codegen/MethodEmitter.java b/nashorn/src/jdk/nashorn/internal/codegen/MethodEmitter.java index 91e2a40a0f6..4fbb57a68cf 100644 --- a/nashorn/src/jdk/nashorn/internal/codegen/MethodEmitter.java +++ b/nashorn/src/jdk/nashorn/internal/codegen/MethodEmitter.java @@ -818,7 +818,7 @@ public class MethodEmitter implements Emitter { } /** - * Push an local variable to the stack. If the symbol representing + * Push a local variable to the stack. If the symbol representing * the local variable doesn't have a slot, this is a NOP * * @param symbol the symbol representing the local variable. @@ -900,6 +900,15 @@ public class MethodEmitter implements Emitter { return functionNode.getBody().getExistingSymbol(cc.symbolName()); } + /** + * True if this method has a slot allocated for the scope variable (meaning, something in the method actually needs + * the scope). + * @return if this method has a slot allocated for the scope variable. + */ + boolean hasScope() { + return compilerConstant(SCOPE).hasSlot(); + } + MethodEmitter loadCompilerConstant(final CompilerConstants cc) { final Symbol symbol = compilerConstant(cc); if (cc == SCOPE && peekType() == Type.SCOPE) { diff --git a/nashorn/src/jdk/nashorn/internal/ir/CallNode.java b/nashorn/src/jdk/nashorn/internal/ir/CallNode.java index 4dfbbb50481..9a730aa862f 100644 --- a/nashorn/src/jdk/nashorn/internal/ir/CallNode.java +++ b/nashorn/src/jdk/nashorn/internal/ir/CallNode.java @@ -50,9 +50,6 @@ public final class CallNode extends LexicalContextNode implements TypeOverride args, final int flags) { + public CallNode(final Source source, final long token, final int finish, final Node function, final List args) { super(source, token, finish); this.function = function; this.args = args; - this.flags = flags; + this.flags = 0; this.type = null; this.evalArgs = null; } @@ -333,14 +329,6 @@ public final class CallNode extends LexicalContextNode implements TypeOverridewith statement - * or an eval call. - * - * @see #hasWith() - * @see #hasEval() - * @return true if this or a nested function contains with or eval - */ - public boolean hasDeepWithOrEval() { - return getFlag(HAS_DEEP_WITH_OR_EVAL); - } - /** * Get the first token for this function * @return the first token diff --git a/nashorn/src/jdk/nashorn/internal/ir/LexicalContext.java b/nashorn/src/jdk/nashorn/internal/ir/LexicalContext.java index a02763445f4..c3f096f50d9 100644 --- a/nashorn/src/jdk/nashorn/internal/ir/LexicalContext.java +++ b/nashorn/src/jdk/nashorn/internal/ir/LexicalContext.java @@ -419,14 +419,6 @@ public class LexicalContext { return null; } - /** - * Check if lexical context is currently inside a with block - * @return true if in a with block - */ - public boolean inWith() { - return getScopeNestingLevelTo(null) > 0; - } - /** * Count the number of with scopes until a given node * @param until node to stop counting at, or null if all nodes should be counted diff --git a/nashorn/src/jdk/nashorn/internal/parser/Parser.java b/nashorn/src/jdk/nashorn/internal/parser/Parser.java index ddc235af7f4..70066853b37 100644 --- a/nashorn/src/jdk/nashorn/internal/parser/Parser.java +++ b/nashorn/src/jdk/nashorn/internal/parser/Parser.java @@ -398,7 +398,7 @@ loop: final String name = ident.getName(); if (EVAL.symbolName().equals(name)) { - markWithOrEval(lc, FunctionNode.HAS_EVAL); + markEval(lc); } } @@ -1353,7 +1353,6 @@ loop: // Get WITH expression. WithNode withNode = new WithNode(source, withToken, finish); - markWithOrEval(lc, FunctionNode.HAS_WITH); try { lc.push(withNode); @@ -1742,7 +1741,7 @@ loop: // Skip ending of edit string expression. expect(RBRACE); - return new CallNode(source, primaryToken, finish, execIdent, arguments, 0); + return new CallNode(source, primaryToken, finish, execIdent, arguments); } /** @@ -2041,10 +2040,6 @@ loop: return new PropertyNode(source, propertyToken, finish, propertyName, assignmentExpression(false), null, null); } - private int callNodeFlags() { - return lc.inWith() ? CallNode.IN_WITH_BLOCK : 0; - } - /** * LeftHandSideExpression : * NewExpression @@ -2074,7 +2069,7 @@ loop: detectSpecialFunction((IdentNode)lhs); } - lhs = new CallNode(source, callToken, finish, lhs, arguments, callNodeFlags()); + lhs = new CallNode(source, callToken, finish, lhs, arguments); } loop: @@ -2088,7 +2083,7 @@ loop: final List arguments = argumentList(); // Create call node. - lhs = new CallNode(source, callToken, finish, lhs, arguments, callNodeFlags()); + lhs = new CallNode(source, callToken, finish, lhs, arguments); break; @@ -2167,7 +2162,7 @@ loop: arguments.add(objectLiteral()); } - final CallNode callNode = new CallNode(source, constructor.getToken(), finish, constructor, arguments, callNodeFlags()); + final CallNode callNode = new CallNode(source, constructor.getToken(), finish, constructor, arguments); return new UnaryNode(source, newToken, callNode); } @@ -2822,16 +2817,16 @@ loop: return "[JavaScript Parsing]"; } - private static void markWithOrEval(final LexicalContext lc, int flag) { + private static void markEval(final LexicalContext lc) { final Iterator iter = lc.getFunctions(); boolean flaggedCurrentFn = false; while (iter.hasNext()) { final FunctionNode fn = iter.next(); if (!flaggedCurrentFn) { - lc.setFlag(fn, flag); + lc.setFlag(fn, FunctionNode.HAS_EVAL); flaggedCurrentFn = true; } else { - lc.setFlag(fn, FunctionNode.HAS_DESCENDANT_WITH_OR_EVAL); + lc.setFlag(fn, FunctionNode.HAS_NESTED_EVAL); } lc.setFlag(lc.getFunctionBody(fn), Block.NEEDS_SCOPE); } diff --git a/nashorn/src/jdk/nashorn/internal/runtime/linker/NashornCallSiteDescriptor.java b/nashorn/src/jdk/nashorn/internal/runtime/linker/NashornCallSiteDescriptor.java index a2513912441..8cb360c4fcf 100644 --- a/nashorn/src/jdk/nashorn/internal/runtime/linker/NashornCallSiteDescriptor.java +++ b/nashorn/src/jdk/nashorn/internal/runtime/linker/NashornCallSiteDescriptor.java @@ -43,9 +43,8 @@ public class NashornCallSiteDescriptor extends AbstractCallSiteDescriptor { public static final int CALLSITE_SCOPE = 0x01; /** Flags that the call site is in code that uses ECMAScript strict mode. */ public static final int CALLSITE_STRICT = 0x02; - /** Flags that a property getter or setter call site references a scope variable that is not in the global scope - * (it is in a function lexical scope), and the function's scope object class is fixed and known in advance. Such - * getters and setters can often be linked more optimally using these assumptions. */ + /** Flags that a property getter or setter call site references a scope variable that is located at a known distance + * in the scope chain. Such getters and setters can often be linked more optimally using these assumptions. */ public static final int CALLSITE_FAST_SCOPE = 0x400; /** Flags that the call site is profiled; Contexts that have {@code "profile.callsites"} boolean property set emit