Skip to content
Permalink

Comparing changes

Choose two branches to see what’s changed or to start a new pull request. If you need to, you can also or learn more about diff comparisons.

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also . Learn more about diff comparisons here.
base repository: jruby/jruby
Failed to load repositories. Confirm that selected base ref is valid, then try again.
Loading
base: 668efad99bb4^
Choose a base ref
...
head repository: jruby/jruby
Failed to load repositories. Confirm that selected head ref is valid, then try again.
Loading
compare: ba060a75dcc3
Choose a head ref
  • 2 commits
  • 6 files changed
  • 1 contributor

Commits on Oct 30, 2014

  1. Record producer info in TempVars as part of OptTempVarsPass

    @subbuss
    subbuss committed Oct 30, 2014
    Copy the full SHA
    668efad View commit details

  2. First hacky attempt at chaining instructions to reduce interp overheads 

    * Instrs are chained via producer field in TemporaryLocalVariable
  and instrs further down in the expr tree are removed from the
  interp instr stream and are interped when their users require the
  values computed by them.

* Doesn't run on all code yet. But, pushing to branch so we can
  collaborate and experiment with it rapidly.

* ~15% user time improvement for total runtime on a set of benches.
  ~7-8% wallclock time improvement.
    @subbuss
    subbuss committed Oct 30, 2014
    Copy the full SHA
    ba060a7 View commit details
68 changes: 54 additions & 14 deletions core/src/main/java/org/jruby/ir/IRScope.java
View file
Original file line number Diff line number Diff line change
@@ -475,6 +475,7 @@ protected Instr[] prepareInstructions() {

// Pass 1. Set up IPCs for labels and instructions and build linear instr list
List<Instr> newInstrs = new ArrayList<>();
Map<BasicBlock, List<Instr>> clonedInstrMap = new HashMap<>();
int ipc = 0;
for (BasicBlock b: linearizedBBList) {
// All same-named labels must be same Java instance for this to work or we would need
@@ -485,19 +486,62 @@ protected Instr[] prepareInstructions() {

List<Instr> bbInstrs = b.getInstrs();
int bbInstrsLength = bbInstrs.size();
List<Instr> clonedInstrs = new ArrayList<>();
clonedInstrMap.put(b, clonedInstrs);
Map<TemporaryLocalVariable, Instr> producers = new HashMap<>();
boolean stopChaining = false;
for (int i = 0; i < bbInstrsLength; i++) {
Instr instr = bbInstrs.get(i);
if (!(instr instanceof ReceiveSelfInstr)) {
Instr newInstr = instr.clone(cloneInfo);

if (newInstr instanceof Specializeable) {
newInstr = ((Specializeable) newInstr).specializeForInterpretation();
// Skip
if (instr instanceof ReceiveSelfInstr) continue;

// We shouldn't pop before all instrs. have executed.
if (instr instanceof PopBindingInstr || instr instanceof PopFrameInstr) stopChaining = true;

Instr newInstr = instr.clone(cloneInfo);
if (newInstr instanceof Specializeable) {
newInstr = ((Specializeable) newInstr).specializeForInterpretation();
}

if (!stopChaining) {
// process uses before defs so you don't introduce cycles in scenarios like this:
// %v = .. %v ...
for (Variable v: newInstr.getUsedVariables()) {
if (v instanceof TemporaryLocalVariable) {
TemporaryLocalVariable tlv = (TemporaryLocalVariable)v;
tlv.producer = producers.get(tlv);
// If producer-consumer are within a basicblock boundary, use it!
if (tlv.producer != null) {
clonedInstrs.remove(tlv.producer);
producers.remove(tlv);
}
}
}

newInstr.setIPC(ipc);
newInstrs.add(newInstr);
ipc++;
// Update producer info
if (newInstr instanceof ResultInstr) {
Variable oldRes = ((ResultInstr)instr).getResult();

if (oldRes instanceof TemporaryLocalVariable && ((TemporaryLocalVariable)oldRes).producer != null) {
TemporaryLocalVariable newRes = (TemporaryLocalVariable)((ResultInstr)newInstr).getResult();
Operation op = newInstr.getOperation();
// Limited set of instrs for which we support chaining
if (op != Operation.RUNTIME_HELPER && (op.opClass == OpClass.CALL_OP || op.opClass == OpClass.OTHER_OP)) {
producers.put(newRes, newInstr);
}
}
}
}

clonedInstrs.add(newInstr);
}

// See which instrs have chained and should be skipped
for (Instr instr: clonedInstrs) {
newInstrs.add(instr);
instr.setIPC(ipc);
ipc++;
}
}

@@ -513,13 +557,9 @@ protected Instr[] prepareInstructions() {
for (BasicBlock b : linearizedBBList) {
BasicBlock rescuerBB = cfg().getRescuerBBFor(b);
int rescuerPC = rescuerBB == null ? -1 : rescuerBB.getLabel().getTargetPC();
for (Instr instr : b.getInstrs()) {
// FIXME: If we did not omit instrs from previous pass we could end up just doing a
// a size and for loop this n times instead of walking an examining each instr
if (!(instr instanceof ReceiveSelfInstr)) {
linearizedInstrArray[ipc].setRPC(rescuerPC);
ipc++;
}
for (Instr instr : clonedInstrMap.get(b)) {
linearizedInstrArray[ipc].setRPC(rescuerPC);
ipc++;
}
}

2 changes: 1 addition & 1 deletion core/src/main/java/org/jruby/ir/instructions/Instr.java
View file
Original file line number Diff line number Diff line change
@@ -114,7 +114,7 @@ public boolean canBeDeleted(IRScope s) {
// If this scope (or any nested scope) has a ZSuperInstr, then the arguments of this
// scope could be used by any of those ZSuper instructions. If so, we cannot delete
// the argument receive.
// SSS FIXME: This check may be redundant. ZSuper now explicits lists
// SSS FIXME: This check may be redundant. ZSuper now explicitly lists
// all arguments that it may use.
return false;
} else {
66 changes: 64 additions & 2 deletions core/src/main/java/org/jruby/ir/interpreter/Interpreter.java
View file
Original file line number Diff line number Diff line change
@@ -211,8 +211,12 @@ private static Object retrieveOp(Operand r, ThreadContext context, IRubyObject s
if (r instanceof Self) {
return self;
} else if (r instanceof TemporaryLocalVariable) {
res = temp[((TemporaryLocalVariable)r).offset];
return res == null ? context.nil : res;
TemporaryLocalVariable tlv = (TemporaryLocalVariable)r;
if (tlv.producer != null) {
return getInstrResult(context, self, currScope, currDynScope, temp, (ResultInstr)tlv.producer);
}
Object o = temp[tlv.offset];
return o == null ? context.nil : o;
} else if (r instanceof LocalVariable) {
LocalVariable lv = (LocalVariable)r;
res = currDynScope.getValue(lv.getLocation(), lv.getScopeDepth());
@@ -561,6 +565,64 @@ private static void processOtherOp(ThreadContext context, Instr instr, Operation
}
}

public static Object getInstrResult(ThreadContext context, IRubyObject self, StaticScope currScope, DynamicScope currDynScope, Object[] temp, ResultInstr instr) {
if (IRRuntimeHelpers.isDebug()) {
LOG.info(" I: {}", instr);
}
Object result;
Operation op = ((Instr)instr).getOperation();
switch(op) {
case CALL_1F: {
OneFixnumArgNoBlockCallInstr call = (OneFixnumArgNoBlockCallInstr)instr;
IRubyObject r = (IRubyObject)retrieveOp(call.getReceiver(), context, self, currDynScope, currScope, temp);
return call.getCallSite().call(context, self, r, call.getFixnumArg());
}
case CALL_1O: {
OneOperandArgNoBlockCallInstr call = (OneOperandArgNoBlockCallInstr)instr;
IRubyObject r = (IRubyObject)retrieveOp(call.getReceiver(), context, self, currDynScope, currScope, temp);
IRubyObject o = (IRubyObject)call.getArg1().retrieve(context, self, currScope, currDynScope, temp);
return call.getCallSite().call(context, self, r, o);
}
case CALL_1OB: {
OneOperandArgBlockCallInstr call = (OneOperandArgBlockCallInstr)instr;
IRubyObject r = (IRubyObject)retrieveOp(call.getReceiver(), context, self, currDynScope, currScope, temp);
IRubyObject o = (IRubyObject)call.getArg1().retrieve(context, self, currScope, currDynScope, temp);
Block preparedBlock = call.prepareBlock(context, self, currScope, currDynScope, temp);
return call.getCallSite().call(context, self, r, o, preparedBlock);
}
case CALL_0O: {
ZeroOperandArgNoBlockCallInstr call = (ZeroOperandArgNoBlockCallInstr)instr;
IRubyObject r = (IRubyObject)retrieveOp(call.getReceiver(), context, self, currDynScope, currScope, temp);
return call.getCallSite().call(context, self, r);
}
case COPY: {
CopyInstr c = (CopyInstr)instr;
Operand src = c.getSource();
return retrieveOp(src, context, self, currDynScope, currScope, temp);
}
case GET_FIELD: {
GetFieldInstr gfi = (GetFieldInstr)instr;
IRubyObject object = (IRubyObject)gfi.getSource().retrieve(context, self, currScope, currDynScope, temp);
VariableAccessor a = gfi.getAccessor(object);
result = a == null ? null : (IRubyObject)a.get(object);
if (result == null) {
if (context.runtime.isVerbose()) {
context.runtime.getWarnings().warning(ID.IVAR_NOT_INITIALIZED, "instance variable " + gfi.getRef() + " not initialized");
}
result = context.nil;
}
return result;
}
case SEARCH_CONST: {
SearchConstInstr sci = (SearchConstInstr)instr;
ConstantCache cache = sci.getConstantCache();
return ConstantCache.isCached(cache) ? cache. value : sci.cache(context, currScope, currDynScope, self, temp);
}
default:
return ((Instr)instr).interpret(context, currScope, currDynScope, self, temp);
}
}

private static IRubyObject interpret(ThreadContext context, IRubyObject self,
InterpreterContext interpreterContext, Visibility visibility, RubyModule implClass,
String name, IRubyObject[] args, Block block, Block.Type blockType) {
14 changes: 13 additions & 1 deletion core/src/main/java/org/jruby/ir/operands/TemporaryLocalVariable.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package org.jruby.ir.operands;

import org.jruby.ir.IRVisitor;
import org.jruby.ir.instructions.Instr;
import org.jruby.ir.instructions.ResultInstr;
import org.jruby.ir.interpreter.Interpreter;
import org.jruby.ir.transformations.inlining.SimpleCloneInfo;
import org.jruby.parser.StaticScope;
import org.jruby.runtime.DynamicScope;
@@ -14,6 +17,7 @@
public class TemporaryLocalVariable extends TemporaryVariable {
public static final String PREFIX = "%v_";
public final int offset;
public Instr producer;

public TemporaryLocalVariable(String name, int offset) {
super(name);
@@ -47,7 +51,7 @@ public String getPrefix() {

@Override
public Variable clone(SimpleCloneInfo ii) {
return new TemporaryLocalVariable(offset);
return new TemporaryLocalVariable(getName(), offset);
}

@Override
@@ -64,10 +68,18 @@ public Object retrieve(ThreadContext context, IRubyObject self, StaticScope curr
// both here and in DynamicScope var lookups. To be done later.
//
// I dont like this at all. This feels ugly!
if (producer != null) {
return Interpreter.getInstrResult(context, self, currScope, currDynScope, temp, (ResultInstr)producer);
}
Object o = temp[offset];
return o == null ? context.nil : o;
}

@Override
public String toString() {
return getName() + (producer == null ? "-unrecorded-" : "=["+ producer.getOperation() + "]" );
}

@Override
public void visit(IRVisitor visitor) {
visitor.TemporaryLocalVariable(this);
59 changes: 49 additions & 10 deletions core/src/main/java/org/jruby/ir/passes/OptimizeTempVarsPass.java
View file
Original file line number Diff line number Diff line change
@@ -4,6 +4,7 @@
import org.jruby.ir.IRScope;
import org.jruby.ir.instructions.*;
import org.jruby.ir.operands.Operand;
import org.jruby.ir.operands.TemporaryLocalVariable;
import org.jruby.ir.operands.TemporaryVariable;
import org.jruby.ir.operands.Variable;

@@ -33,15 +34,33 @@ public boolean invalidate(IRScope s) {
return false;
}

private static void allocVar(Operand oldVar, IRScope s, List<TemporaryVariable> freeVarsList, Map<Operand, Operand> newVarMap) {
private static TemporaryLocalVariable allocVar(Operand oldVar, IRScope s, List<TemporaryLocalVariable> freeVarsList, Map<Operand, Operand> newVarMap, boolean isResult, Instr producer) {
// If we dont have a var mapping, get a new var -- try the free list first
// and if none available, allocate a fresh one
if (newVarMap.get(oldVar) == null) {
newVarMap.put(oldVar, freeVarsList.isEmpty() ? s.createTemporaryVariable() : freeVarsList.remove(0));
TemporaryLocalVariable newVar = (TemporaryLocalVariable)newVarMap.get(oldVar);
if (newVar == null) {
if (freeVarsList.isEmpty()) {
newVar = s.createTemporaryVariable();
} else {
newVar = freeVarsList.remove(0);
if (isResult) {
newVar = (TemporaryLocalVariable)newVar.clone(null);
}
}
newVarMap.put(oldVar, newVar);
} else if (isResult) {
newVar = (TemporaryLocalVariable)newVar.clone(null);
newVarMap.put(oldVar, newVar);
}

if (producer != null) {
newVar.producer = producer;
}

return newVar;
}

private static void freeVar(TemporaryVariable newVar, List<TemporaryVariable> freeVarsList) {
private static void freeVar(TemporaryLocalVariable newVar, List<TemporaryLocalVariable> freeVarsList) {
// Put the new var onto the free list (but only if it is not already there).
if (!freeVarsList.contains(newVar)) freeVarsList.add(0, newVar);
}
@@ -129,10 +148,18 @@ else if ((uses.size() == 1) && (defs != null) && (defs.size() == 1) && (i instan
i.markDead();
instrs.remove();

// Clear uses/defs for old-var
tmpVarUses.remove(v);
tmpVarDefs.remove(v);

// Fix up use
Map<Operand, Operand> copyMap = new HashMap<Operand, Operand>();
copyMap.put(v, src);
soleUse.simplifyOperands(copyMap, true);
if (src instanceof TemporaryLocalVariable) {
List<Instr> srcUses = tmpVarUses.get(src);
srcUses.add(soleUse);
}
}
}
}
@@ -156,10 +183,18 @@ else if (i instanceof CopyInstr) {
if ((uses.size() == 1) && (defs.size() == 1)) {
Instr soleDef = defs.get(0);
if (!soleDef.isDead()) {
Variable ciRes = ci.getResult();
// Fix up def
((ResultInstr)soleDef).updateResult(ci.getResult());
((ResultInstr)soleDef).updateResult(ciRes);
ci.markDead();
instrs.remove();

// Update defs for ciRes if it is a tmp-var
if (ciRes instanceof TemporaryVariable) {
List<Instr> ciDefs = tmpVarDefs.get(ciRes);
ciDefs.remove(ci);
ciDefs.add(soleDef);
}
}
}
}
@@ -219,7 +254,7 @@ else if (i instanceof CopyInstr) {
// Replace all single use operands with constants they were assigned to.
// Using operand -> operand signature because simplifyOperands works on operands
Map<Operand, Operand> newVarMap = new HashMap<Operand, Operand>();
List<TemporaryVariable> freeVarsList = new ArrayList<TemporaryVariable>();
List<TemporaryLocalVariable> freeVarsList = new ArrayList<TemporaryLocalVariable>();
iCount = -1;
s.resetTemporaryVariables();

@@ -230,20 +265,24 @@ else if (i instanceof CopyInstr) {
Variable result = null;
if (i instanceof ResultInstr) {
result = ((ResultInstr)i).getResult();
if (result instanceof TemporaryVariable) allocVar(result, s, freeVarsList, newVarMap);
if (result instanceof TemporaryVariable) {
List<Instr> uses = tmpVarUses.get((TemporaryVariable)result);
List<Instr> defs = tmpVarDefs.get((TemporaryVariable)result);
allocVar(result, s, freeVarsList, newVarMap, true, (uses != null && (uses.size() == 1) && defs != null && (defs.size() == 1)) ? i : null);
}
}
for (Variable v: i.getUsedVariables()) {
if (v instanceof TemporaryVariable) allocVar(v, s, freeVarsList, newVarMap);
if (v instanceof TemporaryVariable) allocVar(v, s, freeVarsList, newVarMap, false, null);
}

// Free dead vars
if ((result instanceof TemporaryVariable) && lastVarUseOrDef.get((TemporaryVariable)result) == iCount) {
freeVar((TemporaryVariable)newVarMap.get(result), freeVarsList);
freeVar((TemporaryLocalVariable)newVarMap.get(result), freeVarsList);
}
for (Variable v: i.getUsedVariables()) {
if (v instanceof TemporaryVariable) {
TemporaryVariable tv = (TemporaryVariable)v;
if (lastVarUseOrDef.get(tv) == iCount) freeVar((TemporaryVariable)newVarMap.get(tv), freeVarsList);
if (lastVarUseOrDef.get(tv) == iCount) freeVar((TemporaryLocalVariable)newVarMap.get(tv), freeVarsList);
}
}

5 changes: 5 additions & 0 deletions core/src/main/java/org/jruby/ir/transformations/inlining/SimpleCloneInfo.java
View file
Original file line number Diff line number Diff line change
@@ -3,6 +3,7 @@
import org.jruby.ir.IRScope;
import org.jruby.ir.operands.Label;
import org.jruby.ir.operands.LocalVariable;
import org.jruby.ir.operands.TemporaryLocalVariable;
import org.jruby.ir.operands.Variable;

/**
@@ -22,6 +23,10 @@ public boolean isEnsureBlockCloneMode() {
}

public Variable getRenamedVariable(Variable variable) {
if (variable instanceof TemporaryLocalVariable) {
return variable.clone(this);
}

Variable renamed = super.getRenamedVariable(variable);

// FIXME: I don't understand how this case can possibly exist. If it does a qualitative comment should be added here.