[GR-16505] Fixed the evalBindingAddsVarsCached specialization to also allow blocks.

eregon · eregon · commit 21742d9c26dc · 2019-06-16T12:08:13.000-07:00
PullRequest: truffleruby/898
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -25,6 +25,10 @@ Changes:
 
 * Interactive sources (like the GraalVM polyglot shell) now all share the same binding (#1695).
 
+Performance:
+
+* `eval(code, binding)` for a fixed `code` containing blocks is now much faster. This improves the performance of rendering `ERB` templates containing loops.
+
 # 20.0.0 beta 1
 
 Bug fixes:
diff --git a/src/main/java/org/truffleruby/core/binding/BindingNodes.java b/src/main/java/org/truffleruby/core/binding/BindingNodes.java
@@ -125,9 +125,18 @@ public static void insertAncestorFrame(RubyContext context, DynamicObject bindin
         newFrame(binding, newFrameDescriptor(context));
     }
 
-    public static boolean hiddenVariable(String name) {
+    public static boolean isHiddenVariable(Object name) {
+        if (name instanceof String) {
+            return isHiddenVariable((String) name);
+        } else {
+            return true;
+        }
+    }
+
+    private static boolean isHiddenVariable(String name) {
         assert !name.isEmpty();
-        return name.startsWith("$") || name.charAt(0) == TranslatorEnvironment.TEMP_PREFIX;
+        return name.charAt(0) == '$' || // Frame-local global variable
+                name.charAt(0) == TranslatorEnvironment.TEMP_PREFIX;
     }
 
     @CoreMethod(names = { "dup", "clone" })
@@ -158,7 +167,7 @@ public RubyNode coerceToString(RubyNode name) {
 
         @Specialization(guards = {
                 "name == cachedName",
-                "!hiddenVariable(cachedName)",
+                "!isHiddenVariable(cachedName)",
                 "getFrameDescriptor(binding) == descriptor"
         }, limit = "getCacheLimit()")
         public boolean localVariableDefinedCached(DynamicObject binding, String name,
@@ -169,13 +178,13 @@ public boolean localVariableDefinedCached(DynamicObject binding, String name,
         }
 
         @TruffleBoundary
-        @Specialization(guards = "!hiddenVariable(name)")
+        @Specialization(guards = "!isHiddenVariable(name)")
         public boolean localVariableDefinedUncached(DynamicObject binding, String name) {
             return FindDeclarationVariableNodes.findFrameSlotOrNull(name, getFrame(binding)) != null;
         }
 
         @TruffleBoundary
-        @Specialization(guards = "hiddenVariable(name)")
+        @Specialization(guards = "isHiddenVariable(name)")
         public Object localVariableDefinedLastLine(DynamicObject binding, String name) {
             throw new RaiseException(getContext(), coreExceptions().nameError("Bad local variable name", binding, name, this));
         }
@@ -197,7 +206,7 @@ public RubyNode coerceToString(RubyNode name) {
             return NameToJavaStringNodeGen.create(name);
         }
 
-        @Specialization(guards = "!hiddenVariable(name)")
+        @Specialization(guards = "!isHiddenVariable(name)")
         public Object localVariableGetUncached(DynamicObject binding, String name,
                 @Cached("create(null)") FindDeclarationVariableNodes.FindAndReadDeclarationVariableNode readNode) {
             MaterializedFrame frame = getFrame(binding);
@@ -209,7 +218,7 @@ public Object localVariableGetUncached(DynamicObject binding, String name,
         }
 
         @TruffleBoundary
-        @Specialization(guards = "hiddenVariable(name)")
+        @Specialization(guards = "isHiddenVariable(name)")
         public Object localVariableGetLastLine(DynamicObject binding, String name) {
             throw new RaiseException(getContext(), coreExceptions().nameError("Bad local variable name", binding, name, this));
         }
@@ -234,7 +243,7 @@ public RubyNode coerceToString(RubyNode name) {
 
         @Specialization(guards = {
                 "name == cachedName",
-                "!hiddenVariable(cachedName)",
+                "!isHiddenVariable(cachedName)",
                 "getFrameDescriptor(binding) == cachedFrameDescriptor",
                 "cachedFrameSlot != null"
         }, limit = "getCacheLimit()")
@@ -249,7 +258,7 @@ public Object localVariableSetCached(DynamicObject binding, String name, Object
 
         @Specialization(guards = {
                 "name == cachedName",
-                "!hiddenVariable(cachedName)",
+                "!isHiddenVariable(cachedName)",
                 "getFrameDescriptor(binding) == cachedFrameDescriptor",
                 "cachedFrameSlot == null"
         }, limit = "getCacheLimit()")
@@ -265,7 +274,7 @@ public Object localVariableSetNewCached(DynamicObject binding, String name, Obje
         }
 
         @TruffleBoundary
-        @Specialization(guards = "!hiddenVariable(name)")
+        @Specialization(guards = "!isHiddenVariable(name)")
         public Object localVariableSetUncached(DynamicObject binding, String name, Object value) {
             MaterializedFrame frame = getFrame(binding);
             final FrameSlotAndDepth frameSlot = FindDeclarationVariableNodes.findFrameSlotOrNull(name, frame);
@@ -282,7 +291,7 @@ public Object localVariableSetUncached(DynamicObject binding, String name, Objec
         }
 
         @TruffleBoundary
-        @Specialization(guards = "hiddenVariable(name)")
+        @Specialization(guards = "isHiddenVariable(name)")
         public Object localVariableSetLastLine(DynamicObject binding, String name, Object value) {
             throw new RaiseException(getContext(), coreExceptions().nameError("Bad local variable name", binding, name, this));
         }
@@ -325,8 +334,7 @@ public static DynamicObject listLocalVariables(RubyContext context, Materialized
 
         private static void addNamesFromFrame(RubyContext context, Frame frame, final Set<Object> names) {
             for (FrameSlot slot : frame.getFrameDescriptor().getSlots()) {
-                if (slot.getIdentifier() instanceof String &&
-                        !hiddenVariable((String) slot.getIdentifier())) {
+                if (!isHiddenVariable(slot.getIdentifier())) {
                     names.add(context.getSymbolTable().getSymbol((String) slot.getIdentifier()));
                 }
             }
diff --git a/src/main/java/org/truffleruby/core/kernel/KernelNodes.java b/src/main/java/org/truffleruby/core/kernel/KernelNodes.java
@@ -128,7 +128,6 @@
 import org.truffleruby.language.objects.PropertyFlags;
 import org.truffleruby.language.objects.ReadObjectFieldNode;
 import org.truffleruby.language.objects.ReadObjectFieldNodeGen;
-import org.truffleruby.language.objects.SelfNode;
 import org.truffleruby.language.objects.ShapeCachingGuards;
 import org.truffleruby.language.objects.SingletonClassNode;
 import org.truffleruby.language.objects.TaintNode;
@@ -571,13 +570,16 @@ public RubyRootNode getRootNode() {
             }
         }
 
-        public abstract Object execute(VirtualFrame frame, Object self, DynamicObject str, DynamicObject binding, DynamicObject file, int line);
+        public abstract Object execute(VirtualFrame frame, Object self, DynamicObject source, DynamicObject binding, DynamicObject file, int line);
+
+        // If the source defines new local variables, those should be set in the Binding.
+        // So we have 2 specializations for whether or not the code defines new local variables.
 
         @Specialization(guards = {
                 "equalNode.execute(rope(source), cachedSource)",
                 "equalNode.execute(rope(file), cachedFile)",
                 "line == cachedLine",
-                "assignsNoNewVariables(cachedRootNode)",
+                "!assignsNewUserVariables(getDescriptor(cachedRootNode))",
                 "bindingDescriptor == getBindingDescriptor(binding)"
         }, limit = "getCacheLimit()")
         public Object evalBindingNoAddsVarsCached(
@@ -602,8 +604,8 @@ public Object evalBindingNoAddsVarsCached(
                 "equalNode.execute(rope(source), cachedSource)",
                 "equalNode.execute(rope(file), cachedFile)",
                 "line == cachedLine",
-                "!assignsNoNewVariables(cachedRootNode)",
-                "assignsNoNewVariables(rootNodeToEval)",
+                "assignsNewUserVariables(getDescriptor(cachedRootNode))",
+                "!assignsNewUserVariables(getDescriptor(rootNodeToEval))",
                 "bindingDescriptor == getBindingDescriptor(binding)"
         }, limit = "getCacheLimit()")
         public Object evalBindingAddsVarsCached(
@@ -617,13 +619,12 @@ public Object evalBindingAddsVarsCached(
                 @Cached("line") int cachedLine,
                 @Cached("getBindingDescriptor(binding)") FrameDescriptor bindingDescriptor,
                 @Cached("compileSource(cachedSource, getBindingFrame(binding), cachedFile, cachedLine)") RootNodeWrapper cachedRootNode,
-                @Cached("newBindingDescriptor(getContext(), cachedRootNode)") FrameDescriptor newBindingDescriptor,
+                @Cached("getDescriptor(cachedRootNode).copy()") FrameDescriptor newBindingDescriptor,
                 @Cached("compileSource(cachedSource, getBindingFrame(binding), newBindingDescriptor, cachedFile, cachedLine)") RootNodeWrapper rootNodeToEval,
                 @Cached("createCallTarget(rootNodeToEval)") RootCallTarget cachedCallTarget,
                 @Cached("create(cachedCallTarget)") DirectCallNode callNode,
                 @Cached("create()") RopeNodes.EqualNode equalNode) {
-            final MaterializedFrame parentFrame = BindingNodes.newFrame(binding,
-                    newBindingDescriptor);
+            final MaterializedFrame parentFrame = BindingNodes.newFrame(binding,  newBindingDescriptor);
             return eval(self, rootNodeToEval, cachedCallTarget, callNode, parentFrame);
         }
 
@@ -658,7 +659,7 @@ private CodeLoader.DeferredCall doEvalX(Object self, Rope source,
             final DeclarationContext declarationContext = RubyArguments.getDeclarationContext(frame);
             final FrameDescriptor descriptor = frame.getFrameDescriptor();
             RubyRootNode rootNode = buildRootNode(source, frame, file, line, false);
-            if (!frameHasOnlySelf(descriptor)) {
+            if (assignsNewUserVariables(descriptor)) {
                 Layouts.BINDING.setFrame(binding, frame);
             }
             return getContext().getCodeLoader().prepareExecute(
@@ -687,32 +688,26 @@ protected FrameDescriptor getBindingDescriptor(DynamicObject binding) {
             return BindingNodes.getFrameDescriptor(binding);
         }
 
-        protected FrameDescriptor newBindingDescriptor(RubyContext context, RootNodeWrapper rootNode) {
-            FrameDescriptor descriptor = rootNode.getRootNode().getFrameDescriptor();
-            FrameDescriptor newDescriptor = new FrameDescriptor(context.getCoreLibrary().getNil());
-            for (FrameSlot frameSlot : descriptor.getSlots()) {
-                newDescriptor.findOrAddFrameSlot(frameSlot.getIdentifier());
-            }
-            return newDescriptor;
+        protected FrameDescriptor getDescriptor(RootNodeWrapper rootNode) {
+            return rootNode.getRootNode().getFrameDescriptor();
         }
 
         protected MaterializedFrame getBindingFrame(DynamicObject binding) {
             return BindingNodes.getFrame(binding);
         }
 
-        protected int getCacheLimit() {
-            return getContext().getOptions().EVAL_CACHE;
-        }
-
-        protected boolean assignsNoNewVariables(RootNodeWrapper rootNode) {
-            FrameDescriptor descriptor = rootNode.getRootNode().getFrameDescriptor();
-            return frameHasOnlySelf(descriptor);
+        protected static boolean assignsNewUserVariables(FrameDescriptor descriptor) {
+            for (FrameSlot slot : descriptor.getSlots()) {
+                if (!BindingNodes.isHiddenVariable(slot.getIdentifier())) {
+                    return true;
+                }
+            }
+            return false;
         }
 
-        private boolean frameHasOnlySelf(FrameDescriptor descriptor) {
-            return descriptor.getSize() == 1 && SelfNode.SELF_IDENTIFIER.equals(descriptor.getSlots().get(0).getIdentifier());
+        protected int getCacheLimit() {
+            return getContext().getOptions().EVAL_CACHE;
         }
-
     }
 
     @CoreMethod(names = "freeze")
diff --git a/src/main/java/org/truffleruby/debug/LexicalScope.java b/src/main/java/org/truffleruby/debug/LexicalScope.java
@@ -24,8 +24,8 @@
 import com.oracle.truffle.api.nodes.Node;
 import com.oracle.truffle.api.nodes.RootNode;
 import org.truffleruby.RubyContext;
+import org.truffleruby.core.binding.BindingNodes;
 import org.truffleruby.language.arguments.RubyArguments;
-import org.truffleruby.parser.TranslatorEnvironment;
 
 import java.util.Collections;
 import java.util.HashMap;
@@ -88,20 +88,7 @@ private static Object getArguments(Frame frame) {
     }
 
     private static boolean isInternal(FrameSlot slot) {
-        return isInternal(slot.getIdentifier());
-    }
-
-    private static boolean isInternal(Object identifier) {
-        if (identifier instanceof String) {
-            return isInternal((String) identifier);
-        } else {
-            return true;
-        }
-    }
-
-    private static boolean isInternal(String identifier) {
-        assert !identifier.isEmpty();
-        return identifier.charAt(0) == TranslatorEnvironment.TEMP_PREFIX;
+        return BindingNodes.isHiddenVariable(slot.getIdentifier());
     }
 
     @ExportLibrary(InteropLibrary.class)
