Plug in Push/Pop Frame/Binding instructions into the interpreter

* Some other minor cleanup in InterpretedIRBlockBody. * A few more instructions are still required -- coming in future patches. * Instructions aren't yet generated -- coming in future patches.
jruby · Nov 28, 2015 · c63ba19 · c63ba19
1 parent 7796079
commit c63ba19
Showing 6 changed files with 68 additions and 26 deletions.
diff --git a/core/src/main/java/org/jruby/ir/interpreter/BodyInterpreterEngine.java b/core/src/main/java/org/jruby/ir/interpreter/BodyInterpreterEngine.java
@@ -10,6 +10,8 @@
 import org.jruby.ir.instructions.JumpInstr;
 import org.jruby.ir.instructions.LineNumberInstr;
 import org.jruby.ir.instructions.NonlocalReturnInstr;
+import org.jruby.ir.instructions.PopBlockFrameInstr;
+import org.jruby.ir.instructions.PushBlockFrameInstr;
 import org.jruby.ir.instructions.ResultInstr;
 import org.jruby.ir.instructions.ReturnBase;
 import org.jruby.ir.instructions.RuntimeHelperCall;
@@ -20,6 +22,7 @@
 import org.jruby.parser.StaticScope;
 import org.jruby.runtime.Block;
 import org.jruby.runtime.DynamicScope;
+import org.jruby.runtime.Frame;
 import org.jruby.runtime.Helpers;
 import org.jruby.runtime.ThreadContext;
 import org.jruby.runtime.Visibility;
@@ -58,6 +61,7 @@ public IRubyObject interpret(ThreadContext context, Block block, IRubyObject sel
             ipc++;
 
             try {
+                Frame f;
                 switch (operation) {
                     case RETURN:
                         return (IRubyObject) retrieveOp(((ReturnBase) instr).getReturnValue(), context, self, currDynScope, currScope, temp);
@@ -77,6 +81,14 @@ public IRubyObject interpret(ThreadContext context, Block block, IRubyObject sel
                     case THROW:
                         instr.interpret(context, currScope, currDynScope, self, temp);
                         break;
+                    case PUSH_BLOCK_FRAME:
+                        f = context.preYieldNoScope(block.getBinding());
+                        setResult(temp, currDynScope, ((PushBlockFrameInstr)instr).getResult(), f);
+                        break;
+                    case POP_BLOCK_FRAME:
+                        f = (Frame)retrieveOp(((PopBlockFrameInstr)instr).getFrame(), context, self, currDynScope, currScope, temp);
+                        context.postYieldNoScope(f);
+                        break;
                     case PUSH_METHOD_FRAME:
                         context.preMethodFrameOnly(implClass, name, self, blockArg);
                         // Only the top-level script scope has PRIVATE visibility.

diff --git a/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java b/core/src/main/java/org/jruby/ir/interpreter/InterpreterEngine.java
@@ -16,6 +16,8 @@
 import org.jruby.ir.instructions.JumpInstr;
 import org.jruby.ir.instructions.LineNumberInstr;
 import org.jruby.ir.instructions.NonlocalReturnInstr;
+import org.jruby.ir.instructions.PopBlockFrameInstr;
+import org.jruby.ir.instructions.PushBlockFrameInstr;
 import org.jruby.ir.instructions.ReceiveArgBase;
 import org.jruby.ir.instructions.ReceivePostReqdArgInstr;
 import org.jruby.ir.instructions.ReceivePreReqdArgInstr;
@@ -53,7 +55,9 @@
 import org.jruby.ir.operands.Variable;
 import org.jruby.ir.runtime.IRRuntimeHelpers;
 import org.jruby.parser.StaticScope;
+import org.jruby.EvalType;
 import org.jruby.runtime.Block;
+import org.jruby.runtime.Frame;
 import org.jruby.runtime.DynamicScope;
 import org.jruby.runtime.Helpers;
 import org.jruby.runtime.ThreadContext;
@@ -166,8 +170,16 @@ public IRubyObject interpret(ThreadContext context, Block block, IRubyObject sel
                             // which will now use the updated value of currDynScope.
                             currDynScope = interpreterContext.newDynamicScope(context);
                             context.pushScope(currDynScope);
+                        } else if (operation == Operation.PUSH_BLOCK_BINDING) {
+                            DynamicScope newScope = block.getBinding().getDynamicScope();
+                            if (interpreterContext.pushNewDynScope()) {
+                                context.pushScope(block.allocScope(newScope));
+                            } else if (interpreterContext.reuseParentDynScope()) {
+                                // Reuse! We can avoid the push only if surrounding vars aren't referenced!
+                                context.pushScope(newScope);
+                            }
                         } else {
-                            processBookKeepingOp(context, block, instr, operation, name, args, self, blockArg, implClass);
+                            processBookKeepingOp(context, block, instr, operation, name, args, self, blockArg, implClass, currDynScope, temp, currScope);
                         }
                         break;
                     case OTHER_OP:
@@ -323,12 +335,21 @@ protected static void processCall(ThreadContext context, Instr instr, Operation
     }
 
     protected static void processBookKeepingOp(ThreadContext context, Block block, Instr instr, Operation operation,
-                                             String name, IRubyObject[] args, IRubyObject self, Block blockArg,
-                                             RubyModule implClass) {
+                                             String name, IRubyObject[] args, IRubyObject self, Block blockArg, RubyModule implClass,
+                                             DynamicScope currDynScope, Object[] temp, StaticScope currScope) {
+        Frame f;
         Block.Type blockType = block == null ? null : block.type;
         switch(operation) {
             case LABEL:
                 break;
+            case PUSH_BLOCK_FRAME:
+                f = context.preYieldNoScope(block.getBinding());
+                setResult(temp, currDynScope, ((PushBlockFrameInstr)instr).getResult(), f);
+                break;
+            case POP_BLOCK_FRAME:
+                f = (Frame)retrieveOp(((PopBlockFrameInstr)instr).getFrame(), context, self, currDynScope, currScope, temp);
+                context.postYieldNoScope(f);
+                break;
             case PUSH_METHOD_FRAME:
                 context.preMethodFrameOnly(implClass, name, self, blockArg);
                 // Only the top-level script scope has PRIVATE visibility.

diff --git a/core/src/main/java/org/jruby/ir/interpreter/StartupInterpreterEngine.java b/core/src/main/java/org/jruby/ir/interpreter/StartupInterpreterEngine.java
@@ -108,7 +108,7 @@ public IRubyObject interpret(ThreadContext context, Block block, IRubyObject sel
                                 rescuePCs.pop();
                                 break;
                             default:
-                                processBookKeepingOp(context, block, instr, operation, name, args, self, blockArg, implClass);
+                                processBookKeepingOp(context, block, instr, operation, name, args, self, blockArg, implClass, currDynScope, temp, currScope);
                         }
                         break;
                     case OTHER_OP:

diff --git a/core/src/main/java/org/jruby/runtime/Block.java b/core/src/main/java/org/jruby/runtime/Block.java
@@ -94,6 +94,20 @@ public Block(BlockBody body) {
         this.binding = null;
     }
 
+    public DynamicScope allocScope(DynamicScope parentScope) {
+        // SSS: Important!  Use getStaticScope() to use a copy of the static-scope stored in the block-body.
+        // Do not use 'closure.getStaticScope()' -- that returns the original copy of the static scope.
+        // This matters because blocks created for Thread bodies modify the static-scope field of the block-body
+        // that records additional state about the block body.
+        //
+        // FIXME: Rather than modify static-scope, it seems we ought to set a field in block-body which is then
+        // used to tell dynamic-scope that it is a dynamic scope for a thread body.  Anyway, to be revisited later!
+        EvalType evalType = ((IRBlockBody)body).getEvalType();
+        DynamicScope newScope = DynamicScope.newDynamicScope(body.getStaticScope(), parentScope, evalType);
+        if (type == Block.Type.LAMBDA) newScope.setLambda(true);
+        return newScope;
+    }
+
     public void setEvalType(EvalType evalType) {
         body.setEvalType(evalType);
     }

diff --git a/core/src/main/java/org/jruby/runtime/IRBlockBody.java b/core/src/main/java/org/jruby/runtime/IRBlockBody.java
@@ -22,6 +22,10 @@ public IRBlockBody(IRScope closure, Signature signature) {
         this.evalType.set(EvalType.NONE);
     }
 
+    public EvalType getEvalType() {
+        return this.evalType.get();
+    }
+
     public void setEvalType(EvalType evalType) {
         this.evalType.set(evalType);
     }

diff --git a/core/src/main/java/org/jruby/runtime/InterpretedIRBlockBody.java b/core/src/main/java/org/jruby/runtime/InterpretedIRBlockBody.java
@@ -1,7 +1,7 @@
 package org.jruby.runtime;
 
-import org.jruby.EvalType;
 import org.jruby.RubyModule;
+import org.jruby.EvalType;
 import org.jruby.compiler.Compilable;
 import org.jruby.ir.IRClosure;
 import org.jruby.ir.IRScope;
@@ -77,41 +77,32 @@ public String getName() {
     }
 
     protected IRubyObject commonYieldPath(ThreadContext context, Block block, IRubyObject[] args, IRubyObject self, Block blockArg) {
-        Binding binding = block.getBinding();
         if (callCount >= 0) promoteToFullBuild(context);
 
-        // SSS: Important!  Use getStaticScope() to use a copy of the static-scope stored in the block-body.
-        // Do not use 'closure.getStaticScope()' -- that returns the original copy of the static scope.
-        // This matters because blocks created for Thread bodies modify the static-scope field of the block-body
-        // that records additional state about the block body.
-        //
-        // FIXME: Rather than modify static-scope, it seems we ought to set a field in block-body which is then
-        // used to tell dynamic-scope that it is a dynamic scope for a thread body.  Anyway, to be revisited later!
+        InterpreterContext ic = ensureInstrsReady();
+
+        Binding binding = block.getBinding();
         Visibility oldVis = binding.getFrame().getVisibility();
         Frame prevFrame = context.preYieldNoScope(binding);
 
-        // SSS FIXME: Why is self null in non-binding-eval contexts?
-        if (self == null || this.evalType.get() == EvalType.BINDING_EVAL) {
-            self = useBindingSelf(binding);
-        }
-
         // SSS FIXME: Maybe, we should allocate a NoVarsScope/DummyScope for for-loop bodies because the static-scope here
         // probably points to the parent scope? To be verified and fixed if necessary. There is no harm as it is now. It
         // is just wasteful allocation since the scope is not used at all.
-
-        InterpreterContext ic = ensureInstrsReady();
-
-        // Pass on eval state info to the dynamic scope and clear it on the block-body
         DynamicScope actualScope = binding.getDynamicScope();
         if (ic.pushNewDynScope()) {
-            actualScope = DynamicScope.newDynamicScope(getStaticScope(), actualScope, this.evalType.get());
-            if (block.type == Block.Type.LAMBDA) actualScope.setLambda(true);
-            context.pushScope(actualScope);
+            context.pushScope(block.allocScope(actualScope));
         } else if (ic.reuseParentDynScope()) {
             // Reuse! We can avoid the push only if surrounding vars aren't referenced!
             context.pushScope(actualScope);
         }
-        this.evalType.set(EvalType.NONE);
+
+        // SSS FIXME: Why is self null in non-binding-eval contexts?
+        if (self == null || getEvalType() == EvalType.BINDING_EVAL) {
+            useBindingSelf(binding);
+        }
+
+        // Clear evaltype now that it has been set on dyn-scope
+        block.setEvalType(EvalType.NONE);
 
         try {
             return Interpreter.INTERPRET_BLOCK(context, block, self, ic, args, binding.getMethod(), blockArg);