Further streamlining of interpreter context

* Deleted old code in IRScope that is no longer relevant. * For closures, short-circuited prepareForInterpretation from base class to ensure that callers use the 1-arg version where it needs to be built. * Subsequent crashers exposed an unresolved FIXME in BuildLambdaInstr that had a WrappedIRClosure that needed cloning and build of an interpreter context as well.
jruby · Oct 15, 2014 · 1a1f927 · 1a1f927
1 parent c664f89
commit 1a1f927
Showing 10 changed files with 50 additions and 57 deletions.
diff --git a/core/src/main/java/org/jruby/internal/runtime/methods/InterpretedIRMethod.java b/core/src/main/java/org/jruby/internal/runtime/methods/InterpretedIRMethod.java
@@ -134,7 +134,7 @@ protected void pre(InterpreterContext ic, ThreadContext context, IRubyObject sel
     }
 
     public InterpreterContext ensureInstrsReady() {
-        InterpreterContext context = method.getInstrsForInterpretation();
+        InterpreterContext context = method.getInterpreterContext();
         if (context == null) {
             context =  method.prepareForInterpretation();
         }

diff --git a/core/src/main/java/org/jruby/ir/IRClosure.java b/core/src/main/java/org/jruby/ir/IRClosure.java
@@ -109,21 +109,27 @@ public IRClosure(IRManager manager, IRScope lexicalParent, int lineNumber, Stati
         this.nestingDepth++;
     }
 
-    public InterpreterContext getInterpreterContext(Operand self) {
-        initScope(false);
-
-        checkRelinearization();
-
+    public InterpreterContext prepareInterpreterContext(Operand self) {
         if (interpreterContext != null) return interpreterContext; // Already prepared
 
+        initScope(false);
+
         Instr[] linearizedInstrArray = prepareInstructions();
+
         interpreterContext = new ClosureInterpreterContext(getTemporaryVariablesCount(), getBooleanVariablesCount(),
                 getFixnumVariablesCount(), getFloatVariablesCount(),getFlags().clone(), linearizedInstrArray,
                 self, getStaticScope(), getBlockBody());
 
         return interpreterContext;
     }
 
+    @Override
+    public synchronized InterpreterContext prepareForInterpretation() {
+        // This should have already been prepared during preparation of parent scopes.
+        // If this is null, it would be a bug and let users throw a NPE.
+        return interpreterContext;
+    }
+
     public void setBeginEndBlock() {
         this.isBeginEndBlock = true;
     }

diff --git a/core/src/main/java/org/jruby/ir/IRScope.java b/core/src/main/java/org/jruby/ir/IRScope.java
@@ -466,18 +466,6 @@ public CFG getCFG() {
         return cfg;
     }
 
-    private synchronized InterpreterContext prepareInterpreterContext() {
-        checkRelinearization();
-
-        if (interpreterContext != null) return interpreterContext; // Already prepared
-
-        Instr[] linearizedInstrArray = prepareInstructions();
-        interpreterContext = new InterpreterContext(getTemporaryVariablesCount(), getBooleanVariablesCount(),
-                getFixnumVariablesCount(), getFloatVariablesCount(),getFlags().clone(), linearizedInstrArray);
-
-        return interpreterContext;
-    }
-
     protected Instr[] prepareInstructions() {
         setupLinearization();
 
@@ -609,9 +597,6 @@ private void optimizeSimpleScopes() {
     }
 
     protected void initScope(boolean jitMode) {
-        // Reset linearization, if any exists
-        resetLinearizationData();
-
         // Build CFG and run compiler passes, if necessary
         if (getCFG() == null) {
             buildCFG();
@@ -631,25 +616,31 @@ protected void initScope(boolean jitMode) {
 
     /** Run any necessary passes to get the IR ready for interpretation */
     public synchronized InterpreterContext prepareForInterpretation() {
-        initScope(false);
+        if (interpreterContext != null) return interpreterContext; // Already prepared
 
-        checkRelinearization();
+        initScope(false);
 
         // System.out.println("-- passes run for: " + this + " = " + java.util.Arrays.toString(executedPasses.toArray()));
 
         // Linearize CFG, etc.
-        return prepareInterpreterContext();
+        Instr[] linearizedInstrArray = prepareInstructions();
+
+        interpreterContext = new InterpreterContext(getTemporaryVariablesCount(), getBooleanVariablesCount(),
+                getFixnumVariablesCount(), getFloatVariablesCount(),getFlags().clone(), linearizedInstrArray);
+
+        return interpreterContext;
     }
 
     /* SSS FIXME: Do we need to synchronize on this?  Cache this info in a scope field? */
     /** Run any necessary passes to get the IR ready for compilation */
     public synchronized List<BasicBlock> prepareForCompilation() {
+        // Reset linearization, if any exists
+        resetLinearizationData();
+
         initScope(true);
 
         runCompilerPasses(getManager().getJITPasses(this));
 
-        checkRelinearization();
-
         prepareInstructions();
 
         return buildLinearization();
@@ -1047,7 +1038,7 @@ public List<Instr> getInstrs() {
         return instrList;
     }
 
-    public InterpreterContext getInstrsForInterpretation() {
+    public InterpreterContext getInterpreterContext() {
         return interpreterContext;
     }
 
@@ -1056,13 +1047,7 @@ public void resetLinearizationData() {
         relinearizeCFG = false;
     }
 
-    public void checkRelinearization() {
-        if (relinearizeCFG) resetLinearizationData();
-    }
-
     public List<BasicBlock> buildLinearization() {
-        checkRelinearization();
-
         if (linearizedBBList != null) return linearizedBBList; // Already linearized
 
         linearizedBBList = CFGLinearizer.linearize(cfg);
@@ -1090,8 +1075,8 @@ public CFG cfg() {
     }
 
     public void resetState() {
-        relinearizeCFG = true;
         interpreterContext = null;
+        resetLinearizationData();
         cfg.resetState();
 
         // reset flags

diff --git a/core/src/main/java/org/jruby/ir/instructions/BuildLambdaInstr.java b/core/src/main/java/org/jruby/ir/instructions/BuildLambdaInstr.java
@@ -21,9 +21,9 @@ public class BuildLambdaInstr extends Instr implements ResultInstr, FixedArityIn
     /** The position for the block */
     private final ISourcePosition position;
     private Variable result;
-    private WrappedIRClosure lambdaBody;
+    private Operand lambdaBody;
 
-    public BuildLambdaInstr(Variable lambda, WrappedIRClosure lambdaBody, ISourcePosition position) {
+    public BuildLambdaInstr(Variable lambda, Operand lambdaBody, ISourcePosition position) {
         super(Operation.LAMBDA);
 
         this.result = lambda;
@@ -32,8 +32,10 @@ public BuildLambdaInstr(Variable lambda, WrappedIRClosure lambdaBody, ISourcePos
     }
 
     public String getLambdaBodyName() {
-        return getLambdaBody().getClosure().getName();
+        // SSS FIXME: this requires a fix 
+        return ""; // getLambdaBody().getClosure().getName();
     }
+
     @Override
     public Operand[] getOperands() {
         return new Operand[] { lambdaBody, new StringLiteral(position.getFile()), new Fixnum(position.getLine()) };
@@ -61,16 +63,15 @@ public boolean computeScopeFlags(IRScope scope) {
 
     @Override
     public Instr clone(CloneInfo ii) {
-        // SSS FIXME: This is buggy. The lambda body might have to be cloned depending on cloning context.
-        return new BuildLambdaInstr(ii.getRenamedVariable(getResult()), getLambdaBody(), position);
+        return new BuildLambdaInstr(ii.getRenamedVariable(getResult()), getLambdaBody().cloneForInlining(ii), position);
     }
 
     @Override
     public void simplifyOperands(Map<Operand, Operand> valueMap, boolean force) {
-        lambdaBody = (WrappedIRClosure) lambdaBody.getSimplifiedOperand(valueMap, force);
+        lambdaBody = lambdaBody.getSimplifiedOperand(valueMap, force);
     }
 
-    public WrappedIRClosure getLambdaBody() {
+    public Operand getLambdaBody() {
         return lambdaBody;
     }
 
@@ -83,7 +84,7 @@ public Object interpret(ThreadContext context, StaticScope currScope, DynamicSco
         // SSS FIXME: Copied this from ast/LambdaNode ... Is this required here as well?
         //
         // JRUBY-5686: do this before executing so first time sets cref module
-        getLambdaBody().getClosure().getStaticScope().determineModule();
+        ((ClosureInterpreterContext)getLambdaBody()).getStaticScope().determineModule();
 
         // CON: This must not be happening, because nil would never cast to Block
 //        IRClosure body = getLambdaBody().getClosure();

diff --git a/core/src/main/java/org/jruby/ir/interpreter/Interpreter.java b/core/src/main/java/org/jruby/ir/interpreter/Interpreter.java
@@ -78,7 +78,10 @@ public static IRubyObject interpretCommonEval(Ruby runtime, String file, int lin
         StaticScope ss = rootNode.getStaticScope();
         IRScope containingIRScope = getEvalContainerScope(ss, evalType);
         IREvalScript evalScript = IRBuilder.createIRBuilder(runtime, runtime.getIRManager()).buildEvalRoot(ss, containingIRScope, file, lineNumber, rootNode, evalType);
-        evalScript.prepareForInterpretation();
+        // ClosureInterpreterContext never retrieved as an operand in this context.
+        // So, self operand is not required here.
+        // Passing null to force early crasher if ever used differently.
+        evalScript.prepareInterpreterContext(null);
         ThreadContext context = runtime.getCurrentContext();
 
         IRubyObject rv = null;
@@ -121,7 +124,7 @@ public static void runBeginEndBlocks(List<IRClosure> beBlocks, ThreadContext con
 
         for (IRClosure b: beBlocks) {
             // SSS FIXME: Should I piggyback on WrappedIRClosure.retrieve or just copy that code here?
-            b.prepareForInterpretation();
+            b.prepareInterpreterContext(b.getSelf());
             Block blk = (Block)(new WrappedIRClosure(b.getSelf(), b)).retrieve(context, self, currScope, context.getCurrentScope(), temp);
             blk.yield(context, null);
         }
@@ -525,7 +528,7 @@ private static DynamicScope getNewDynScope(ThreadContext context, IRScope scope,
 
     private static IRubyObject interpret(ThreadContext context, IRubyObject self,
             IRScope scope, Visibility visibility, RubyModule implClass, String name, IRubyObject[] args, Block block, Block.Type blockType) {
-        InterpreterContext interpreterContext = scope.getInstrsForInterpretation();
+        InterpreterContext interpreterContext = scope.getInterpreterContext();
         Instr[] instrs = interpreterContext.getInstructions();
         int      numTempVars    = interpreterContext.getTemporaryVariablecount();
         Object[] temp           = numTempVars > 0 ? new Object[numTempVars] : null;

diff --git a/core/src/main/java/org/jruby/ir/interpreter/Profiler.java b/core/src/main/java/org/jruby/ir/interpreter/Profiler.java
@@ -162,7 +162,7 @@ public int compare(IRCallSite a, IRCallSite b) {
 
             IRScope tgtMethod = ircs.tgtM.getIRMethod();
 
-            Instr[] instrs = tgtMethod.getInstrsForInterpretation().getInstructions();
+            Instr[] instrs = tgtMethod.getInterpreterContext().getInstructions();
             // Dont inline large methods -- 500 is arbitrary
             // Can be null if a previously inlined method hasn't been rebuilt
             if ((instrs == null) || instrs.length > 500) {
@@ -228,8 +228,8 @@ public int compare(IRScope a, IRScope b) {
                 if (bden == 0) bden = 1;
 
                 // Use estimated instr count to order scopes -- rather than raw thread-poll count
-                float aCount = scopeThreadPollCounts.get(a).count * (1.0f * a.getInstrsForInterpretation().getInstructions().length/aden);
-                float bCount = scopeThreadPollCounts.get(b).count * (1.0f * b.getInstrsForInterpretation().getInstructions().length/bden);
+                float aCount = scopeThreadPollCounts.get(a).count * (1.0f * a.getInterpreterContext().getInstructions().length/aden);
+                float bCount = scopeThreadPollCounts.get(b).count * (1.0f * b.getInterpreterContext().getInstructions().length/bden);
                 if (aCount == bCount) return 0;
                 return (aCount < bCount) ? 1 : -1;
             }

diff --git a/core/src/main/java/org/jruby/ir/operands/ClosureInterpreterContext.java b/core/src/main/java/org/jruby/ir/operands/ClosureInterpreterContext.java
@@ -32,6 +32,8 @@ public ClosureInterpreterContext(int temporaryVariablecount, int temporaryBoolea
         this.body = body;
     }
 
+    public StaticScope getStaticScope() { return staticScope; }
+
     @Override
     public Object retrieve(ThreadContext context, IRubyObject self, StaticScope currScope, DynamicScope currDynScope, Object[] temp) {
         staticScope.determineModule();

diff --git a/core/src/main/java/org/jruby/ir/operands/WrappedIRClosure.java b/core/src/main/java/org/jruby/ir/operands/WrappedIRClosure.java
@@ -61,9 +61,9 @@ public Operand cloneForInlining(CloneInfo info) {
                 return new WrappedIRClosure(info.getRenamedVariable(self), closure.cloneForInlining(info));
             }
 
-            // We are saving instructions so that if JIT hits IRClosure it will not concurrently
+            // We are saving instructions so that if JIT hits IRClosure, it will not concurrently
             // modify the same object.
-            return closure.getInterpreterContext(self);
+            return closure.prepareInterpreterContext(self);
         }
 
         return new WrappedIRClosure(info.getRenamedVariable(self), closure.cloneForInlining(info));

diff --git a/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java b/core/src/main/java/org/jruby/ir/targets/JVMVisitor.java
@@ -1931,7 +1931,7 @@ public void BuildLambdaInstr(BuildLambdaInstr buildlambdainstr) {
 
         jvmMethod().loadRuntime();
 
-        IRClosure body = buildlambdainstr.getLambdaBody().getClosure();
+        IRClosure body = ((WrappedIRClosure)buildlambdainstr.getLambdaBody()).getClosure();
         if (body == null) {
             jvmMethod().pushNil();
         } else {

diff --git a/core/src/main/java/org/jruby/runtime/InterpretedIRBlockBody.java b/core/src/main/java/org/jruby/runtime/InterpretedIRBlockBody.java
@@ -27,19 +27,15 @@ public InterpretedIRBlockBody(IRClosure closure, Arity arity, int argumentType)
     }
 
     public InterpreterContext ensureInstrsReady() {
-        InterpreterContext context = closure.getInstrsForInterpretation();
-        if (context == null) {
-            context = closure.prepareForInterpretation();
-        }
-
         if (IRRuntimeHelpers.isDebug() && !displayedCFG) {
             LOG.info("Executing '" + closure + "' (pushScope=" + pushScope + ", reuseParentScope=" + reuseParentScope);
             CFG cfg = closure.getCFG();
             LOG.info("Graph:\n" + cfg.toStringGraph());
             LOG.info("CFG:\n" + cfg.toStringInstrs());
             displayedCFG = true;
         }
-        return context;
+        // Always prepared in the context of parent scope -- so a null value here is a bug.
+        return closure.getInterpreterContext();
     }
 
     protected IRubyObject commonYieldPath(ThreadContext context, IRubyObject[] args, IRubyObject self, Binding binding, Type type, Block block) {