Fixed the evalBindingAddsVarsCached specialization to also allow blocks

* Centralize checks for hidden variables in BindingNodes.
* Use FrameDescriptor.copy() instead of doing it manually.

Author: eregon

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:

src/main/java/org/truffleruby/core/binding/BindingNodes.java

@@ -125,6 +125,14 @@ public static void insertAncestorFrame(RubyContext context, DynamicObject bindin
         newFrame(binding, newFrameDescriptor(context));
     }
 
+    public static boolean hiddenVariable(Object name) {
+        if (name instanceof String) {
+            return hiddenVariable((String) name);
+        } else {
+            return true;
+        }
+    }
+
     public static boolean hiddenVariable(String name) {
         assert !name.isEmpty();
         return name.startsWith("$") || name.charAt(0) == TranslatorEnvironment.TEMP_PREFIX;

src/main/java/org/truffleruby/core/kernel/KernelNodes.java

@@ -577,7 +577,7 @@ public RubyRootNode getRootNode() {
                 "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 +602,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 +617,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 +657,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 +686,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.hiddenVariable(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")

src/main/java/org/truffleruby/debug/LexicalScope.java

@@ -24,6 +24,7 @@
 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;
 
@@ -88,20 +89,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.hiddenVariable(slot.getIdentifier());
     }
 
     @ExportLibrary(InteropLibrary.class)