Skip to content

Commit

Permalink
Modify yield logic to be aware of the on-stack block.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
headius committed Jan 9, 2015
1 parent e1ffae8 commit f17a736
Show file tree
Hide file tree
Showing 19 changed files with 370 additions and 95 deletions.
135 changes: 79 additions & 56 deletions core/src/main/java/org/jruby/ir/IRBuilder.java
Expand Up @@ -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) {
Expand Down Expand Up @@ -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>
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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() } ));
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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]));
Expand All @@ -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]));
Expand Down Expand Up @@ -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);
}

Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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
Expand Down
1 change: 0 additions & 1 deletion core/src/main/java/org/jruby/ir/IRFlags.java
Expand Up @@ -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?
Expand Down
28 changes: 27 additions & 1 deletion core/src/main/java/org/jruby/ir/IRScope.java
Expand Up @@ -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;
Expand Down Expand Up @@ -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!
Expand Down Expand Up @@ -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) {
Expand Down
5 changes: 4 additions & 1 deletion core/src/main/java/org/jruby/ir/IRVisitor.java
Expand Up @@ -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); }
Expand All @@ -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); }
Expand Down Expand Up @@ -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); }
Expand Down
6 changes: 5 additions & 1 deletion core/src/main/java/org/jruby/ir/Operation.java
Expand Up @@ -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,
Expand Down
Expand Up @@ -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());
}
Expand Down

0 comments on commit f17a736

Please sign in to comment.