Skip to content

Commit

Permalink
Clear reference to 'this' on all tail calls
Browse files Browse the repository at this point in the history 
Removes calls to emitClearLocals(), which is a no-op.

When the context is RETURN (indicating a tail call), and the operation
is an InvokeExpr, StaticMethodExpr, or InstanceMethodExpr, clear the
reference to 'this' which is in slot 0 of the locals.

Edge-case: Inside the body of a try block, we cannot clear 'this' at the tail
position as we might need to keep refs around for use in the catch or finally
clauses. Introduces another truthy dynamic binding var to track position being
inside a try block.

In a try block with no catch or finally, use enclosing context and return a
regular BodyExpr.

Adds two helpers to emitClearThis and inTailCall.

Adds test for the original reducer case and some try/catch cases.

Signed-off-by: Stuart Halloway <stu@cognitect.com>
  • Loading branch information
@ghadishayban @stuarthalloway
ghadishayban authored and stuarthalloway committed Jul 18, 2015
1 parent 6f4a49b commit 9b6c3a5
Show file tree
Hide file tree
Showing 3 changed files with 81 additions and 43 deletions.
102 changes: 59 additions & 43 deletions src/jvm/clojure/lang/Compiler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ public class Compiler implements Opcodes{
static final public Var METHOD = Var.create(null).setDynamic();

//null or not
static final public Var IN_TRY_BLOCK = Var.create(null).setDynamic();
static final public Var IN_CATCH_FINALLY = Var.create(null).setDynamic();

static final public Var NO_RECUR = Var.create(null).setDynamic();
Expand Down Expand Up @@ -371,6 +372,10 @@ static boolean isSpecial(Object sym){
return specials.containsKey(sym);
}

static boolean inTailCall(C context) {
return (context == C.RETURN) && (IN_TRY_BLOCK.deref() == null);
}

static Symbol resolveSymbol(Symbol sym){
//already qualified or classname?
if(sym.name.indexOf('.') > 0)
Expand Down Expand Up @@ -1002,12 +1007,13 @@ else if(instance != null && instance.hasJavaClass() && instance.getJavaClass() !
Symbol sym = (Symbol) RT.first(call);
Symbol tag = tagOf(form);
PersistentVector args = PersistentVector.EMPTY;
boolean tailPosition = inTailCall(context);
for(ISeq s = RT.next(call); s != null; s = s.next())
args = args.cons(analyze(context == C.EVAL ? context : C.EXPRESSION, s.first()));
if(c != null)
return new StaticMethodExpr(source, line, column, tag, c, munge(sym.name), args);
return new StaticMethodExpr(source, line, column, tag, c, munge(sym.name), args, tailPosition);
else
return new InstanceMethodExpr(source, line, column, tag, instance, munge(sym.name), args);
return new InstanceMethodExpr(source, line, column, tag, instance, munge(sym.name), args, tailPosition);
}
}
}
Expand Down Expand Up @@ -1444,13 +1450,15 @@ static class InstanceMethodExpr extends MethodExpr{
public final int line;
public final int column;
public final Symbol tag;
public final boolean tailPosition;
public final java.lang.reflect.Method method;

final static Method invokeInstanceMethodMethod =
Method.getMethod("Object invokeInstanceMethod(Object,String,Object[])");


public InstanceMethodExpr(String source, int line, int column, Symbol tag, Expr target, String methodName, IPersistentVector args)
public InstanceMethodExpr(String source, int line, int column, Symbol tag, Expr target,
String methodName, IPersistentVector args, boolean tailPosition)
{
this.source = source;
this.line = line;
Expand All @@ -1459,6 +1467,7 @@ public InstanceMethodExpr(String source, int line, int column, Symbol tag, Expr
this.methodName = methodName;
this.target = target;
this.tag = tag;
this.tailPosition = tailPosition;
if(target.hasJavaClass() && target.getJavaClass() != null)
{
List methods = Reflector.getMethods(target.getJavaClass(), args.count(), methodName, false);
Expand Down Expand Up @@ -1552,10 +1561,10 @@ public void emitUnboxed(C context, ObjExpr objx, GeneratorAdapter gen){
gen.checkCast(type);
MethodExpr.emitTypedArgs(objx, gen, method.getParameterTypes(), args);
gen.visitLineNumber(line, gen.mark());
if(context == C.RETURN)
if(tailPosition)
{
ObjMethod method = (ObjMethod) METHOD.deref();
method.emitClearLocals(gen);
method.emitClearThis(gen);
}
Method m = new Method(methodName, Type.getReturnType(method), Type.getArgumentTypes(method));
if(method.getDeclaringClass().isInterface())
Expand Down Expand Up @@ -1626,12 +1635,14 @@ static class StaticMethodExpr extends MethodExpr{
public final int column;
public final java.lang.reflect.Method method;
public final Symbol tag;
public final boolean tailPosition;
final static Method forNameMethod = Method.getMethod("Class classForName(String)");
final static Method invokeStaticMethodMethod =
Method.getMethod("Object invokeStaticMethod(Class,String,Object[])");
final static Keyword warnOnBoxedKeyword = Keyword.intern("warn-on-boxed");

public StaticMethodExpr(String source, int line, int column, Symbol tag, Class c, String methodName, IPersistentVector args)
public StaticMethodExpr(String source, int line, int column, Symbol tag, Class c,
String methodName, IPersistentVector args, boolean tailPosition)
{
this.c = c;
this.methodName = methodName;
Expand All @@ -1640,6 +1651,7 @@ public StaticMethodExpr(String source, int line, int column, Symbol tag, Class c
this.line = line;
this.column = column;
this.tag = tag;
this.tailPosition = tailPosition;

List methods = Reflector.getMethods(c, args.count(), methodName, true);
if(methods.isEmpty())
Expand Down Expand Up @@ -1778,10 +1790,10 @@ public void emit(C context, ObjExpr objx, GeneratorAdapter gen){
MethodExpr.emitTypedArgs(objx, gen, method.getParameterTypes(), args);
gen.visitLineNumber(line, gen.mark());
//Type type = Type.getObjectType(className.replace('.', '/'));
if(context == C.RETURN)
if(tailPosition)
{
ObjMethod method = (ObjMethod) METHOD.deref();
method.emitClearLocals(gen);
method.emitClearThis(gen);
}
Type type = Type.getType(c);
Method m = new Method(methodName, Type.getReturnType(method), Type.getArgumentTypes(method));
Expand Down Expand Up @@ -2265,13 +2277,14 @@ public Expr parse(C context, Object frm) {
}
else
{
if(bodyExpr == null)
try {
Var.pushThreadBindings(RT.map(NO_RECUR, true));
bodyExpr = (new BodyExpr.Parser()).parse(context, RT.seq(body));
} finally {
Var.popThreadBindings();
}
if(bodyExpr == null)
try {
Var.pushThreadBindings(RT.map(NO_RECUR, true, IN_TRY_BLOCK, RT.T));
bodyExpr = (new BodyExpr.Parser()).parse(context, RT.seq(body));
} finally {
Var.popThreadBindings();
}

if(Util.equals(op, CATCH))
{
Class c = HostExpr.maybeClass(RT.second(f), false);
Expand Down Expand Up @@ -2319,17 +2332,21 @@ public Expr parse(C context, Object frm) {
}
}
}
if(bodyExpr == null) {
try
{
Var.pushThreadBindings(RT.map(NO_RECUR, true));
bodyExpr = (new BodyExpr.Parser()).parse(C.EXPRESSION, RT.seq(body));
}
finally
{
Var.popThreadBindings();
}
}
if(bodyExpr == null)
{
// this codepath is hit when there is neither catch or finally, e.g. (try (expr))
// return a body expr directly
try
{
Var.pushThreadBindings(RT.map(NO_RECUR, true));
bodyExpr = (new BodyExpr.Parser()).parse(context, RT.seq(body));
}
finally
{
Var.popThreadBindings();
}
return bodyExpr;
}

return new TryExpr(bodyExpr, catches, finallyExpr, retLocal,
finallyLocal);
Expand Down Expand Up @@ -2577,23 +2594,13 @@ public void emit(C context, ObjExpr objx, GeneratorAdapter gen){
gen.newInstance(type);
gen.dup();
MethodExpr.emitTypedArgs(objx, gen, ctor.getParameterTypes(), args);
if(context == C.RETURN)
{
ObjMethod method = (ObjMethod) METHOD.deref();
method.emitClearLocals(gen);
}
gen.invokeConstructor(type, new Method("<init>", Type.getConstructorDescriptor(ctor)));
}
else
{
gen.push(destubClassName(c.getName()));
gen.invokeStatic(RT_TYPE, forNameMethod);
MethodExpr.emitArgsAsArray(args, objx, gen);
if(context == C.RETURN)
{
ObjMethod method = (ObjMethod) METHOD.deref();
method.emitClearLocals(gen);
}
gen.invokeStatic(REFLECTOR_TYPE, invokeConstructorMethod);
}
if(context == C.STATEMENT)
Expand Down Expand Up @@ -3570,6 +3577,7 @@ static class InvokeExpr implements Expr{
public final IPersistentVector args;
public final int line;
public final int column;
public final boolean tailPosition;
public final String source;
public boolean isProtocol = false;
public boolean isDirect = false;
Expand All @@ -3579,12 +3587,14 @@ static class InvokeExpr implements Expr{
static Keyword onKey = Keyword.intern("on");
static Keyword methodMapKey = Keyword.intern("method-map");

public InvokeExpr(String source, int line, int column, Symbol tag, Expr fexpr, IPersistentVector args) {
public InvokeExpr(String source, int line, int column, Symbol tag, Expr fexpr, IPersistentVector args, boolean tailPosition) {
this.source = source;
this.fexpr = fexpr;
this.args = args;
this.line = line;
this.column = column;
this.tailPosition = tailPosition;

if(fexpr instanceof VarExpr)
{
Var fvar = ((VarExpr)fexpr).var;
Expand Down Expand Up @@ -3736,10 +3746,10 @@ void emitArgsAndCall(int firstArgToEmit, C context, ObjExpr objx, GeneratorAdapt
}
gen.visitLineNumber(line, gen.mark());

if(context == C.RETURN)
if(tailPosition)
{
ObjMethod method = (ObjMethod) METHOD.deref();
method.emitClearLocals(gen);
method.emitClearThis(gen);
}

gen.invokeInterface(IFN_TYPE, new Method("invoke", OBJECT_TYPE, ARG_TYPES[Math.min(MAX_POSITIONAL_ARITY + 1,
Expand All @@ -3755,6 +3765,7 @@ public Class getJavaClass() {
}

static public Expr parse(C context, ISeq form) {
boolean tailPosition = inTailCall(context);
if(context != C.EVAL)
context = C.EXPRESSION;
Expr fexpr = analyze(context, form.first());
Expand Down Expand Up @@ -3817,7 +3828,7 @@ static public Expr parse(C context, ISeq form) {
// throw new IllegalArgumentException(
// String.format("No more than %d args supported", MAX_POSITIONAL_ARITY));

return new InvokeExpr((String) SOURCE.deref(), lineDeref(), columnDeref(), tagOf(form), fexpr, args);
return new InvokeExpr((String) SOURCE.deref(), lineDeref(), columnDeref(), tagOf(form), fexpr, args, tailPosition);
}
}

Expand Down Expand Up @@ -5758,6 +5769,11 @@ void emitClearLocalsOld(GeneratorAdapter gen){
}
}
}

void emitClearThis(GeneratorAdapter gen) {
gen.visitInsn(Opcodes.ACONST_NULL);
gen.visitVarInsn(Opcodes.ASTORE, 0);
}
}

public static class LocalBinding{
Expand Down Expand Up @@ -6183,14 +6199,14 @@ public Expr parse(C context, Object frm) {
{
if(recurMismatches != null && RT.booleanCast(recurMismatches.nth(i/2)))
{
init = new StaticMethodExpr("", 0, 0, null, RT.class, "box", RT.vector(init));
init = new StaticMethodExpr("", 0, 0, null, RT.class, "box", RT.vector(init), false);
if(RT.booleanCast(RT.WARN_ON_REFLECTION.deref()))
RT.errPrintWriter().println("Auto-boxing loop arg: " + sym);
}
else if(maybePrimitiveType(init) == int.class)
init = new StaticMethodExpr("", 0, 0, null, RT.class, "longCast", RT.vector(init));
init = new StaticMethodExpr("", 0, 0, null, RT.class, "longCast", RT.vector(init), false);
else if(maybePrimitiveType(init) == float.class)
init = new StaticMethodExpr("", 0, 0, null, RT.class, "doubleCast", RT.vector(init));
init = new StaticMethodExpr("", 0, 0, null, RT.class, "doubleCast", RT.vector(init), false);
}
//sequential enhancement of env (like Lisp let*)
try
Expand Down
18 changes: 18 additions & 0 deletions test/clojure/test_clojure/compilation.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -342,3 +342,21 @@
(deftest clj-1399
;; throws an exception on failure
(is (eval `(fn [] ~(CLJ1399. 1)))))

(deftest CLJ-1250-this-clearing
(let [closed-over-in-catch (let [x :foo]
(fn []
(try
(throw (Exception. "boom"))
(catch Exception e
x)))) ;; x should remain accessible to the fn

a (atom nil)
closed-over-in-finally (fn []
(try
:ret
(finally
(reset! a :run))))]
(is (= :foo (closed-over-in-catch)))
(is (= :ret (closed-over-in-finally)))
(is (= :run @a))))
4 changes: 4 additions & 0 deletions test/clojure/test_clojure/reducers.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,3 +89,7 @@
([ret k v] (when (= k k-fail)
(throw (IndexOutOfBoundsException.)))))
(zipmap (range test-map-count) (repeat :dummy)))))))

(deftest test-closed-over-clearing
;; this will throw OutOfMemory without proper reference clearing
(is (number? (reduce + 0 (r/map identity (range 1e8))))))

0 comments on commit 9b6c3a5

Please sign in to comment.