Modify yield logic to be aware of the on-stack block.

Previously, because we only had one yield instruction and one
way to prepare any incoming block, the lowest-common-denominator
was to always turn passed blocks into a Proc object before sending
it on to a yield. This is less overhead than in 1.7, and resulted
in our yield performance being worse than it should be. See #2442.

In order to fix this I have made the following changes.

* ReceiveClosure, the instruction which reifies an implicit block
  into a Proc, has been renamed to ReifyClosure. It is now used
  only for block args, as in def foo(&b).
* LoadImplicitClosure and LoadFrameClosure are added to retrieve
  a block from either the JVM stack or from our frame stack.
  Method bodies and singleton class bodies use the stack, and all
  other scopes use frame. This allows us to load the block used
  for yielding in an efficient way regardless of which scope we
  are in.
* IRScope temporarily aggregates two variables: one for the
  "yield" closure, the one used for yielding (which may come from
  either JVM stack or our frame); and one for the "implicit"
  closure, which is the one passed directly to this scope on the
  JVM stack. This additional closure is needed to represent
  two-block scopes like a block that receives a block.
* ProcessModuleBody now has an operand to load a block passed to
  the module/class body. For normal module and class bodies this
  is always NullBlock (new operand for Block.NULL_BLOCK), but
  singleton class bodies (class << obj) inherit the surrounding
  scope's yield closure (not the implicit closure, since that
  would capture nearest block's block rather than outer method's
  block).

This results in a substantial reduction of allocation, and a
decent performance improvement:

BEFORE

               yield      6.901M (±19.0%) i/s -     32.351M
          block.call      6.509M (±10.6%) i/s -     31.999M
     method dispatch     18.988M (±16.8%) i/s -     90.797M

AFTER

               yield      8.002M (±22.4%) i/s -     36.745M
          block.call      6.399M (±12.8%) i/s -     31.314M
     method dispatch     18.939M (±10.5%) i/s -     92.585M

Author: headius

core/src/main/java/org/jruby/ir/IRBuilder.java

@@ -312,24 +312,6 @@ public void addInstrAtBeginning(IRScope s, Instr i) {
         }
     }
 
-    // FIXME: This all seems wrong to me.  IRClosures can receive explicit closures why are we searching only methods
-    // and by-passing closures
-    private Operand getImplicitBlockArg(IRScope s) {
-        int n = 0;
-        while (s instanceof IRClosure || s instanceof IRMetaClassBody) {
-            n++;
-            s = s.getLexicalParent();
-        }
-
-        if (s != null) {
-            LocalVariable v = s instanceof IRMethod ? s.getLocalVariable(Variable.BLOCK, 0) : null;
-
-            if (v != null) return n == 0 ? v : v.cloneForDepth(n);
-        }
-
-        return manager.getNil();
-    }
-
     // Emit cloned ensure bodies by walking up the ensure block stack.
     // If we have been passed a loop value, only emit bodies that are nested within that loop.
     private void emitEnsureBlocks(IRScope s, IRLoop loop) {
@@ -509,8 +491,8 @@ public Operand buildLambda(LambdaNode node, IRScope s) {
         // like the ensure block stack
         IRBuilder closureBuilder = newIRBuilder(manager);
 
-        // Receive self
-        closureBuilder.addInstr(closure, new ReceiveSelfInstr(closure.getSelf()));
+        // Prepare all implicit state (self, frame block, etc)
+        closureBuilder.prepareImplicitState(closure);
 
         // Set %current_scope = <current-scope>
         // Set %current_module = <current-module>
@@ -1124,7 +1106,7 @@ public Operand buildClass(ClassNode classNode, IRScope s) {
         IRClassBody body = new IRClassBody(manager, s, className, classNode.getPosition().getLine(), classNode.getScope());
         Variable classVar = addResultInstr(s, new DefineClassInstr(s.createTemporaryVariable(), body, container, superClass));
 
-        return buildModuleOrClassBody(s, classVar, body, classNode.getBodyNode(), classNode.getPosition().getLine());
+        return buildModuleOrClassBody(s, classVar, body, classNode.getBodyNode(), classNode.getPosition().getLine(), NullBlock.INSTANCE);
     }
 
     // class Foo; class << self; end; end
@@ -1135,7 +1117,8 @@ public Operand buildSClass(SClassNode sclassNode, IRScope s) {
         IRModuleBody body = new IRMetaClassBody(manager, s, manager.getMetaClassName(), sclassNode.getPosition().getLine(), sclassNode.getScope());
         Variable sClassVar = addResultInstr(s, new DefineMetaClassInstr(s.createTemporaryVariable(), receiver, body));
 
-        return buildModuleOrClassBody(s, sClassVar, body, sclassNode.getBodyNode(), sclassNode.getPosition().getLine());
+        // sclass bodies inherit the block of their containing method
+        return buildModuleOrClassBody(s, sClassVar, body, sclassNode.getBodyNode(), sclassNode.getPosition().getLine(), s.getYieldClosureVariable());
     }
 
     // @@c
@@ -1445,7 +1428,7 @@ public Operand run() {
             return addResultInstr(scope, new RuntimeHelperCall(scope.createTemporaryVariable(), IS_DEFINED_METHOD,
                     new Operand[] { scope.getSelf(), new StringLiteral(((VCallNode) node).getName()), Boolean.FALSE}));
         case YIELDNODE:
-            return buildDefinitionCheck(scope, new BlockGivenInstr(scope.createTemporaryVariable(), getImplicitBlockArg(scope)), "yield");
+            return buildDefinitionCheck(scope, new BlockGivenInstr(scope.createTemporaryVariable(), scope.getYieldClosureVariable()), "yield");
         case ZSUPERNODE:
             return addResultInstr(scope, new RuntimeHelperCall(scope.createTemporaryVariable(), IS_DEFINED_SUPER,
                     new Operand[] { scope.getSelf() } ));
@@ -1678,6 +1661,8 @@ protected IRMethod defineMethodInner(MethodDefNode defNode, IRMethod method, IRS
 
         addInstr(method, new ReceiveSelfInstr(method.getSelf()));
 
+        // Prepare all implicit state (self, frame block, etc)
+        prepareImplicitState(method);
 
         // These instructions need to be toward the top of the method because they may both be needed for
         // processing optional arguments as in def foo(a = Object).
@@ -1766,23 +1751,6 @@ public void receiveRequiredArg(Node node, IRScope s, int argIndex, boolean post,
         }
     }
 
-    private void receiveClosureArg(BlockArgNode blockVarNode, IRScope s) {
-        Variable blockVar = null;
-        if (blockVarNode != null) {
-            String blockArgName = blockVarNode.getName();
-            blockVar = s.getNewLocalVariable(blockArgName, 0);
-            if (s instanceof IRMethod) ((IRMethod)s).addArgDesc(IRMethodArgs.ArgType.block, blockArgName);
-            addInstr(s, new ReceiveClosureInstr(blockVar));
-        }
-
-        // In addition, store the block argument in an implicit block variable
-        if (s instanceof IRMethod) {
-            Variable implicitBlockArg = (Variable)getImplicitBlockArg(s);
-            if (blockVar == null) addInstr(s, new ReceiveClosureInstr(implicitBlockArg));
-            else addInstr(s, new CopyInstr(implicitBlockArg, blockVar));
-        }
-    }
-
     protected void receiveNonBlockArgs(final ArgsNode argsNode, IRScope s) {
         final int numPreReqd = argsNode.getPreCount();
         final int numPostReqd = argsNode.getPostCount();
@@ -1863,13 +1831,54 @@ protected void receiveNonBlockArgs(final ArgsNode argsNode, IRScope s) {
         }
     }
 
+    /**
+     * Reify the implicit incoming block into a full Proc, for use as "block arg", but only if
+     * a block arg is specified in this scope's arguments.
+     *
+     * @param argsNode the arguments containing the block arg, if any
+     * @param s the scope to which we are adding instructions
+     */
     protected void receiveBlockArg(final ArgsNode argsNode, IRScope s) {
-        // For methods, we always receive it (implicitly, if the block arg is not explicit)
-        // For closures, only if it is explicitly present
+        // reify to Proc if we have a block arg
         BlockArgNode blockArg = argsNode.getBlock();
-        if (s instanceof IRMethod || blockArg != null) receiveClosureArg(blockArg, s);
+        if (blockArg != null) {
+            String blockArgName = blockArg.getName();
+            Variable blockVar = s.getNewLocalVariable(blockArgName, 0);
+            if (s instanceof IRMethod) ((IRMethod) s).addArgDesc(IRMethodArgs.ArgType.block, blockArgName);
+            addInstr(s, new ReifyClosureInstr(s.getImplicitClosureVariable(), blockVar));
+        }
     }
 
+    /**
+     * Prepare implicit runtime state needed for typical methods to execute. This includes such things
+     * as the implicit self variable and any yieldable block available to this scope.
+     *
+     * @param s the scope to which we are adding instructions
+     */
+    private void prepareImplicitState(IRScope s) {
+        // Receive self
+        addInstr(s, new ReceiveSelfInstr(s.getSelf()));
+
+        // used for constructing block arg; if none, this will go away
+        addInstr(s, new LoadImplicitClosureInstr(s.getImplicitClosureVariable()));
+
+        // used for yields; metaclass body (sclass) inherits yield var from surrounding, and accesses it as implicit
+        if (s instanceof IRMethod || s instanceof IRMetaClassBody) {
+            addInstr(s, new LoadImplicitClosureInstr(s.getYieldClosureVariable()));
+        } else {
+            addInstr(s, new LoadFrameClosureInstr(s.getYieldClosureVariable()));
+        }
+    }
+
+    /**
+     * Process all arguments specified for this scope. This includes pre args (required args
+     * at the beginning of the argument list), opt args (arguments with a default value),
+     * a rest arg (catch-all for argument list overflow), post args (required arguments after
+     * a rest arg) and a block arg (to reify an incoming block into a Proc object.
+     *
+     * @param argsNode the args node containing the specification for the arguments
+     * @param s the scope to which we are adding instructions
+     */
     public void receiveArgs(final ArgsNode argsNode, IRScope s) {
         // 1.9 pre, opt, rest, post args
         receiveNonBlockArgs(argsNode, s);
@@ -2023,7 +2032,7 @@ private void handleBreakAndReturnsInLambdas(IRClosure s) {
         addInstr(s, new LabelInstr(rEndLabel));
     }
 
-    public void receiveMethodArgs(final ArgsNode argsNode, IRScope s) {
+    public void receiveMethodArgs(final ArgsNode argsNode, IRMethod s) {
         receiveArgs(argsNode, s);
     }
 
@@ -2348,8 +2357,8 @@ public Operand buildForIter(final ForNode forNode, IRScope s) {
         // like the ensure block stack
         IRBuilder forBuilder = newIRBuilder(manager);
 
-            // Receive self
-        forBuilder.addInstr(closure, new ReceiveSelfInstr(closure.getSelf()));
+        // Prepare all implicit state (self, frame block, etc)
+        forBuilder.prepareImplicitState(closure);
 
             // Build args
         Node varNode = forNode.getVarNode();
@@ -2499,8 +2508,15 @@ public Operand buildIter(final IterNode iterNode, IRScope s) {
         // like the ensure block stack
         IRBuilder closureBuilder = newIRBuilder(manager);
 
-        // Receive self
-        closureBuilder.addInstr(closure, new ReceiveSelfInstr(closure.getSelf()));
+        // Prepare all implicit state (self, frame block, etc)
+        closureBuilder.prepareImplicitState(closure);
+
+        // load block into temporary variable
+        if (s instanceof IRMethod) {
+            addInstr(s, new LoadImplicitClosureInstr(s.getYieldClosureVariable()));
+        } else {
+            addInstr(s, new LoadFrameClosureInstr(s.getYieldClosureVariable()));
+        }
 
         // Build args
         if (iterNode.getVarNode().getNodeType() != null) closureBuilder.receiveBlockArgs(iterNode, closure);
@@ -2635,7 +2651,7 @@ public Operand buildModule(ModuleNode moduleNode, IRScope s) {
         IRModuleBody body = new IRModuleBody(manager, s, moduleName, moduleNode.getPosition().getLine(), moduleNode.getScope());
         Variable moduleVar = addResultInstr(s, new DefineModuleInstr(s.createTemporaryVariable(), body, container));
 
-        return buildModuleOrClassBody(s, moduleVar, body, moduleNode.getBodyNode(), moduleNode.getPosition().getLine());
+        return buildModuleOrClassBody(s, moduleVar, body, moduleNode.getBodyNode(), moduleNode.getPosition().getLine(), NullBlock.INSTANCE);
     }
 
     public Operand buildMultipleAsgn(MultipleAsgnNode multipleAsgnNode, IRScope s) {
@@ -3232,6 +3248,8 @@ public IREvalScript buildEvalRoot(StaticScope staticScope, IRScope containingSco
         // Debug info: record line number
         addInstr(script, new LineNumberInstr(lineNumber));
 
+        prepareImplicitState(script);
+
         // Set %current_scope = <current-scope>
         // Set %current_module = <current-module>
         addInstr(script, new CopyInstr(script.getCurrentScopeVariable(), CURRENT_SCOPE[0]));
@@ -3249,7 +3267,10 @@ public IRScriptBody buildRoot(RootNode rootNode) {
 
         // Top-level script!
         IRScriptBody script = new IRScriptBody(manager, file, staticScope);
-        addInstr(script, new ReceiveSelfInstr(script.getSelf()));
+
+        // Prepare all implicit state (self, frame block, etc)
+        prepareImplicitState(script);
+
         // Set %current_scope = <current-scope>
         // Set %current_module = <current-module>
         addInstr(script, new CopyInstr(script.getCurrentScopeVariable(), CURRENT_SCOPE[0]));
@@ -3301,7 +3322,7 @@ public Operand buildSuper(SuperNode superNode, IRScope s) {
 
         Operand[] args = setupCallArgs(superNode.getArgsNode(), s);
         Operand block = setupCallClosure(superNode.getIterNode(), s);
-        if (block == null) block = getImplicitBlockArg(s);
+        if (block == null) block = s.getYieldClosureVariable();
         return buildSuperInstr(s, block, args);
     }
 
@@ -3418,7 +3439,7 @@ public Operand buildYield(YieldNode node, IRScope s) {
         }
 
         Variable ret = s.createTemporaryVariable();
-        addInstr(s, new YieldInstr(ret, getImplicitBlockArg(s), build(argNode, s), unwrap));
+        addInstr(s, new YieldInstr(ret, s.getYieldClosureVariable(), build(argNode, s), unwrap));
         return ret;
     }
 
@@ -3490,7 +3511,7 @@ public Operand buildZSuper(ZSuperNode zsuperNode, IRScope s) {
         if (s.isModuleBody()) return buildSuperInScriptBody(s);
 
         Operand block = setupCallClosure(zsuperNode.getIterNode(), s);
-        if (block == null) block = getImplicitBlockArg(s);
+        if (block == null) block = s.getYieldClosureVariable();
 
         // Enebo:ZSuper in for (or nested for) can be statically resolved like method but it needs to fixup depth.
         if (s instanceof IRMethod) {
@@ -3519,15 +3540,17 @@ private Operand[] adjustVariableDepth(Operand[] args, int depthFromSuper) {
         return newArgs;
     }
 
-    private Operand buildModuleOrClassBody(IRScope parent, Variable moduleVar, IRModuleBody body, Node bodyNode, int linenumber) {
-        Variable processBodyResult = addResultInstr(parent, new ProcessModuleBodyInstr(parent.createTemporaryVariable(), moduleVar));
+    private Operand buildModuleOrClassBody(IRScope parent, Variable moduleVar, IRModuleBody body, Node bodyNode, int linenumber, Operand block) {
+        Variable processBodyResult = addResultInstr(parent, new ProcessModuleBodyInstr(parent.createTemporaryVariable(), moduleVar, block));
         IRBuilder bodyBuilder = newIRBuilder(manager);
 
         if (RubyInstanceConfig.FULL_TRACE_ENABLED) {
             bodyBuilder.addInstr(body, new TraceInstr(RubyEvent.CLASS, null, body.getFileName(), linenumber));
         }
 
-        bodyBuilder.addInstr(body, new ReceiveSelfInstr(body.getSelf()));                            // %self
+        // Prepare all implicit state (self, frame block, etc)
+        bodyBuilder.prepareImplicitState(body);
+
         bodyBuilder.addInstr(body, new CopyInstr(body.getCurrentScopeVariable(), CURRENT_SCOPE[0])); // %scope
         bodyBuilder.addInstr(body, new CopyInstr(body.getCurrentModuleVariable(), SCOPE_MODULE[0])); // %module
         // Create a new nested builder to ensure this gets its own IR builder state

core/src/main/java/org/jruby/ir/IRFlags.java

@@ -44,7 +44,6 @@ public enum IRFlags {
     HAS_EXPLICIT_CALL_PROTOCOL,   // contains call protocol instrs => we don't need to manage bindings frame implicitly
     HAS_LOOPS,                    // has a loop
     HAS_NONLOCAL_RETURNS,         // has a non-local return
-    HAS_UNUSED_IMPLICIT_BLOCK_ARG,// Is %block implicit block arg unused?
     RECEIVES_CLOSURE_ARG,         // This scope (or parent receives a closure
     RECEIVES_KEYWORD_ARGS,        // receives keyword args
     REQUIRES_DYNSCOPE,            // does this scope require a dynamic scope?

core/src/main/java/org/jruby/ir/IRScope.java

@@ -133,6 +133,10 @@ public abstract class IRScope implements ParseResult {
 
     private IRManager manager;
 
+    private TemporaryVariable yieldClosureVariable;
+
+    private TemporaryVariable implicitClosureVariable;
+
     // Used by cloning code
     protected IRScope(IRScope s, IRScope lexicalParent) {
         this.lexicalParent = lexicalParent;
@@ -190,7 +194,6 @@ public IRScope(IRManager manager, IRScope lexicalParent, String name,
         flags.remove(HAS_EXPLICIT_CALL_PROTOCOL);
         flags.remove(HAS_LOOPS);
         flags.remove(HAS_NONLOCAL_RETURNS);
-        flags.remove(HAS_UNUSED_IMPLICIT_BLOCK_ARG);
         flags.remove(RECEIVES_KEYWORD_ARGS);
 
         // These flags are true by default!
@@ -896,6 +899,29 @@ public void setTemporaryVariableCount(int count) {
         temporaryVariableIndex = count + 1;
     }
 
+    /**
+     * Get the variable for accessing the "yieldable" closure in this scope.
+     */
+    public TemporaryVariable getYieldClosureVariable() {
+        if (yieldClosureVariable == null) {
+            return yieldClosureVariable = createTemporaryVariable();
+        }
+
+        return yieldClosureVariable;
+    }
+
+    /**
+     * Get the variable for accessing the implicit closure in this scope (the block
+     * passed along the JVM stack).
+     */
+    public TemporaryVariable getImplicitClosureVariable() {
+        if (implicitClosureVariable == null) {
+            return implicitClosureVariable = createTemporaryVariable();
+        }
+
+        return implicitClosureVariable;
+    }
+
     public TemporaryLocalVariable getNewUnboxedVariable(Class type) {
         TemporaryVariableType varType;
         if (type == Float.class) {

core/src/main/java/org/jruby/ir/IRVisitor.java

@@ -73,6 +73,8 @@ private void error(Object object) {
     public void LexicalSearchConstInstr(LexicalSearchConstInstr lexicalsearchconstinstr) { error(lexicalsearchconstinstr); }
     public void LineNumberInstr(LineNumberInstr linenumberinstr) { error(linenumberinstr); }
     public void LoadLocalVarInstr(LoadLocalVarInstr loadlocalvarinstr) { error(loadlocalvarinstr); }
+    public void LoadImplicitClosure(LoadImplicitClosureInstr loadimplicitclosureinstr) { error(loadimplicitclosureinstr); }
+    public void LoadFrameClosure(LoadFrameClosureInstr loadframeclosureinstr) { error(loadframeclosureinstr); }
     public void Match2Instr(Match2Instr match2instr) { error(match2instr); }
     public void Match3Instr(Match3Instr match3instr) { error(match3instr); }
     public void MatchInstr(MatchInstr matchinstr) { error(matchinstr); }
@@ -95,7 +97,7 @@ private void error(Object object) {
     public void PushFrameInstr(PushFrameInstr pushframeinstr) { error(pushframeinstr); }
     public void RaiseArgumentErrorInstr(RaiseArgumentErrorInstr raiseargumenterrorinstr) { error(raiseargumenterrorinstr); }
     public void RaiseRequiredKeywordArgumentErrorInstr(RaiseRequiredKeywordArgumentError instr) { error(instr); }
-    public void ReceiveClosureInstr(ReceiveClosureInstr receiveclosureinstr) { error(receiveclosureinstr); }
+    public void ReifyClosureInstr(ReifyClosureInstr reifyclosureinstr) { error(reifyclosureinstr); }
     public void ReceiveRubyExceptionInstr(ReceiveRubyExceptionInstr receiveexceptioninstr) { error(receiveexceptioninstr); }
     public void ReceiveJRubyExceptionInstr(ReceiveJRubyExceptionInstr receiveexceptioninstr) { error(receiveexceptioninstr); }
     public void ReceiveKeywordArgInstr(ReceiveKeywordArgInstr receiveKeywordArgInstr) { error(receiveKeywordArgInstr); }
@@ -163,6 +165,7 @@ private void error(Object object) {
     public void LocalVariable(LocalVariable localvariable) { error(localvariable); }
     public void Nil(Nil nil) { error(nil); }
     public void NthRef(NthRef nthref) { error(nthref); }
+    public void NullBlock(NullBlock nullblock) { error(nullblock); }
     public void ObjectClass(ObjectClass objectclass) { error(objectclass); }
     public void Rational(Rational rational) { error(rational); }
     public void Regexp(Regexp regexp) { error(regexp); }

core/src/main/java/org/jruby/ir/Operation.java

@@ -49,9 +49,13 @@ public enum Operation {
     RECV_KW_REST_ARG(OpFlags.f_is_arg_receive),
     RECV_REST_ARG(OpFlags.f_is_arg_receive),
     RECV_OPT_ARG(OpFlags.f_is_arg_receive),
-    RECV_CLOSURE(OpFlags.f_is_arg_receive),
     RECV_RUBY_EXC(OpFlags.f_is_arg_receive),
     RECV_JRUBY_EXC(OpFlags.f_is_arg_receive),
+    LOAD_IMPLICT_CLOSURE(OpFlags.f_is_arg_receive),
+    LOAD_FRAME_CLOSURE(OpFlags.f_is_arg_receive),
+
+    /** Instruction to reify an passed-in block to a Proc for def foo(&b) */
+    REIFY_CLOSURE(0),
 
     /* By default, call instructions cannot be deleted even if their results
      * aren't used by anyone unless we know more about what the call is,

core/src/main/java/org/jruby/ir/dataflow/analyses/LiveVariableNode.java

@@ -298,7 +298,6 @@ void markDeadInstructions() {
                     // System.out.println("YES!");
                     i.markDead();
                     it.remove();
-                    problem.getScope().getFlags().add(IRFlags.HAS_UNUSED_IMPLICIT_BLOCK_ARG);
                 } else {
                     // System.out.println("NO! has side effects! Op is: " + i.getOperation());
                 }