Basic auto-refcounter for the rewriter

src/asm_writing/rewriter.h

@@ -201,7 +201,12 @@ class RewriterVar;
 class RewriterAction;
 
 enum class RefType {
-    UNKNOWN,
+    UNKNOWN
+#ifndef NDEBUG
+        // Set this to non-zero to make it possible for the debugger to
+        = 0
+#endif
+        ,
     OWNED,
     BORROWED,
 };
@@ -209,10 +214,13 @@ enum class RefType {
 // This might make more sense as an inner class of Rewriter, but
 // you can't forward-declare that :/
 class RewriterVar {
-#ifndef NDEBUG
-private:
-    int skip_assert_last_action = 0;
-#endif
+    // Fields for automatic refcounting:
+    int num_refs_consumed = 0; // The number of "refConsumed()" calls on this RewriterVar
+    int last_refconsumed_numuses = 0; // The number of uses in the `uses` array when the last refConsumed() call was made.
+    RefType reftype = RefType::UNKNOWN;
+    // Helper function: whether there is a ref that got consumed but came from the consumption of the
+    // initial (owned) reference.
+    bool refHandedOff();
 
 public:
     typedef llvm::SmallVector<RewriterVar*, 8> SmallVector;
@@ -228,109 +236,31 @@ public:
     RewriterVar* cmp(AST_TYPE::AST_TYPE cmp_type, RewriterVar* other, Location loc = Location::any());
     RewriterVar* toBool(Location loc = Location::any());
 
-    RewriterVar* setType(RefType type) {
-        assert(this->reftype == RefType::UNKNOWN);
-        assert(type != RefType::UNKNOWN);
-        this->reftype = type;
-        this->vrefcount = 1;
-        return this;
-    }
+    RewriterVar* setType(RefType type);
 
-    // Due to optimizations, there are some functions that sometimes return a new RewriterVar and
-    // sometimes return an existing one (specifically, loadConst() does this).
-    // asBorrowed() is meant for that kind of case.
-    // I think its presence indicates something fishy about the API though.
+    // XXX convert callers back to setType
     RewriterVar* asBorrowed() {
-        if (this->reftype == RefType::UNKNOWN)
-            return setType(RefType::BORROWED);
-
-        assert(this->reftype == RefType::BORROWED);
-
-        // Special case: the rewriter can "resurrect" constants, and produce an existing
-        // RewriterVar even though at the bjit / IC level it's a new variable.
-        if (this->vrefcount == 0) {
-#ifndef NDEBUG
-            skip_assert_last_action++;
-#endif
-            vrefcount++;
-            return this;
-        } else {
-            return this->incvref();
-        }
+        return setType(RefType::BORROWED);
     }
 
     RewriterVar* incvref() {
-        assert(reftype == RefType::OWNED || reftype == RefType::BORROWED);
-        //assert(vrefcount > 0 || (reftype == RefType::BORROWED && vrefcount >= 0));
-        assert(vrefcount > 0);
-        vrefcount++;
-        return this;
+        assert(0 && "don't call incvref anymore");
     }
 
-#ifndef NDEBUG
-    // Assert that there are exactly num_left actions get added after this call.
-    // Meant for debugging.  It's a way to cross between the two rewriter phases.
-    void addAssertLastActionAction(int num_left=0);
-#endif
-
     RewriterVar* decvref() {
-        assert(reftype == RefType::OWNED || reftype == RefType::BORROWED);
-        assert(vrefcount > 0);
-        vrefcount--;
-
-        if (vrefcount == 0) {
-
-            if (reftype == RefType::OWNED)
-                decref();
-
-            // Assert that there are no more uses of the this at the machine-code level.
-            // This is kind of weird to cross this abstraction boundary like this, but
-            // I think it's a good safety net.
-#ifndef NDEBUG
-            addAssertLastActionAction();
-#endif
-        }
-        return this;
+        assert(0 && "don't call decvref anymore");
     }
 
-    // Steal a ref (not a vref) from this object.
-    // If there aren't any owned refs available, one will be created.
-    // Doesn't change the vrefcount, but the vref that stealRef acts on
-    // becomes a borrowed vref.  One can not use this RewriterVar after stealRef
-    // except for the operation that will actually steal the ref.
-    // If that is necessary, an extra vref should be taken out beforehand.
-    //
-    // Typical usage should look like:
-    //   var->stealRef();
-    //   functionThatStealsRef(var);
-    //   var->decvref();
-    //
-    // If you need to use var after functionThatStealsRef, the whole snippet
-    // needs to be surrounded in an incvref/decvref.
-    //
-    // TODO: I still think I can find a better API for this.  Maybe something like
-    // var->materializeRefFor([&](){ functionThatStealsRef(var); });
-    //
-    RewriterVar* stealRef() {
-        assert(reftype == RefType::OWNED || reftype == RefType::BORROWED);
-        assert(vrefcount > 0);
-
-        if (reftype == RefType::OWNED && vrefcount == 1) {
-            // handoff, do nothing;
-            reftype = RefType::BORROWED;
-        } else {
-            incref();
-        }
+    // Call this to let the automatic refcount machinery know that a refcount
+    // got "consumed", ie passed off.  Such as to a function that steals a reference,
+    // or when stored into a memory location that is an owned reference, etc.
+    // This should get called *after* the ref got consumed, ie something like
+    //   r_array->setAttr(0, r_val);
+    //   r_val->refConsumed()
+    void refConsumed();
 
-        if (vrefcount == 1) {
-            // See the comment in decvref.
-            // This is similar, except that we want there to be exactly one more use of this variable:
-            // whatever consumes the vref.  We want that to happen after the materializeVref call.
-#ifndef NDEBUG
-            addAssertLastActionAction(1);
-#endif
-        }
-        return this;
+    RewriterVar* stealRef() {
+        assert(0 && "don't call stealref anymore");
     }
 
 
@@ -360,7 +290,12 @@ private:
     llvm::SmallVector<int, 32> uses;
     int next_use;
     void bumpUse();
+    // Call this on the result at the end of the action in which it's created
+    // TODO we should have a better way of dealing with variables that have 0 uses
     void releaseIfNoUses();
+    // Helper funciton to release all the resources that this var is using.
+    // Don't call it directly: call bumpUse and releaseIfNoUses instead.
+    void _release();
     bool isDoneUsing() { return next_use == uses.size(); }
     bool hasScratchAllocation() const { return scratch_allocation.second > 0; }
     void resetHasScratchAllocation() { scratch_allocation = std::make_pair(0, 0); }
@@ -369,9 +304,6 @@ private:
     bool is_arg;
     bool is_constant;
 
-    RefType reftype = RefType::UNKNOWN;
-    int vrefcount;
-
     uint64_t constant_value;
     Location arg_loc;
     std::pair<int /*offset*/, int /*size*/> scratch_allocation;
@@ -397,12 +329,7 @@ private:
     RewriterVar& operator=(const RewriterVar&) = delete;
 
 public:
-    RewriterVar(Rewriter* rewriter)
-        : rewriter(rewriter),
-          next_use(0),
-          is_arg(false),
-          is_constant(false),
-          vrefcount(0) {
+    RewriterVar(Rewriter* rewriter) : rewriter(rewriter), next_use(0), is_arg(false), is_constant(false) {
         assert(rewriter);
     }
 
@@ -410,7 +337,7 @@ public:
     // XXX: for testing, reset these on deallocation so that we will see the next time they get set.
     ~RewriterVar() {
         reftype = (RefType)-1;
-        vrefcount = -11;
+        num_refs_consumed = -11;
     }
 #endif
 

src/codegen/ast_interpreter.cpp

@@ -418,8 +418,6 @@ Box* ASTInterpreter::executeInner(ASTInterpreter& interpreter, CFGBlock* start_b
                 interpreter.jit->emitSetCurrentInst(s);
             if (v.o) {
                 Py_DECREF(v.o);
-                if (v.var)
-                    v.var->decvref();
             }
             v = interpreter.visit_stmt(s);
         }
@@ -509,7 +507,6 @@ void ASTInterpreter::doStore(AST_expr* node, STOLEN(Value) value) {
         Value o = visit_expr(attr->value);
         if (jit) {
             jit->emitSetAttr(node, o, attr->attr.getBox(), value);
-            o.var->decvref();
         }
         pyston::setattr(o.o, attr->attr.getBox(), value.o);
         Py_DECREF(o.o);
@@ -578,10 +575,6 @@ Value ASTInterpreter::visit_binop(AST_BinOp* node) {
     AUTO_DECREF(left.o);
     AUTO_DECREF(right.o);
     Value r = doBinOp(node, left, right, node->op_type, BinExpType::BinOp);
-    if (jit) {
-        left.var->decvref();
-        right.var->decvref();
-    }
     return r;
 }
 
@@ -869,10 +862,6 @@ Value ASTInterpreter::visit_augBinOp(AST_AugBinOp* node) {
     Py_DECREF(left.o);
     Py_DECREF(right.o);
 
-    if (jit) {
-        left.var->decvref();
-        right.var->decvref();
-    }
     return r;
 }
 
@@ -883,8 +872,6 @@ Value ASTInterpreter::visit_langPrimitive(AST_LangPrimitive* node) {
         Value val = visit_expr(node->args[0]);
         v = Value(getPystonIter(val.o), jit ? jit->emitGetPystonIter(val) : NULL);
         Py_DECREF(val.o);
-        if (jit)
-            val.var->decvref();
     } else if (node->opcode == AST_LangPrimitive::IMPORT_FROM) {
         assert(node->args.size() == 2);
         assert(node->args[0]->type == AST_TYPE::Name);
@@ -946,8 +933,6 @@ Value ASTInterpreter::visit_langPrimitive(AST_LangPrimitive* node) {
         Value obj = visit_expr(node->args[0]);
         v = Value(boxBool(nonzero(obj.o)), jit ? jit->emitNonzero(obj) : NULL);
         Py_DECREF(obj.o);
-        if (jit)
-            obj.var->decvref();
     } else if (node->opcode == AST_LangPrimitive::SET_EXC_INFO) {
         assert(node->args.size() == 3);
 
@@ -976,8 +961,6 @@ Value ASTInterpreter::visit_langPrimitive(AST_LangPrimitive* node) {
         Value obj = visit_expr(node->args[0]);
         v = Value(boxBool(hasnext(obj.o)), jit ? jit->emitHasnext(obj) : NULL);
         Py_DECREF(obj.o);
-        if (jit)
-            obj.var->decvref();
     } else if (node->opcode == AST_LangPrimitive::PRINT_EXPR) {
         abortJITing();
         Value obj = visit_expr(node->args[0]);
@@ -1157,8 +1140,6 @@ Value ASTInterpreter::visit_makeFunction(AST_MakeFunction* mkfn) {
         if (jit) {
             auto prev_func_var = func.var;
             func.var = jit->emitRuntimeCall(NULL, decorators[i], ArgPassSpec(1), { func }, NULL);
-            decorators[i].var->decvref();
-            prev_func_var->decvref();
         }
     }
     return func;
@@ -1333,12 +1314,6 @@ Value ASTInterpreter::visit_print(AST_Print* node) {
     else
         printHelper(getSysStdout(), autoXDecref(var.o), node->nl);
 
-    if (jit) {
-        if (node->dest)
-            dest.var->decvref();
-        var.var->decvref();
-    }
-
     return Value();
 }
 
@@ -1362,10 +1337,6 @@ Value ASTInterpreter::visit_compare(AST_Compare* node) {
     Value r = doBinOp(node, left, right, node->ops[0], BinExpType::Compare);
     Py_DECREF(left.o);
     Py_DECREF(right.o);
-    if (jit) {
-        left.var->decvref();
-        right.var->decvref();
-    }
     return r;
 }
 
@@ -1499,12 +1470,6 @@ Value ASTInterpreter::visit_call(AST_Call* node) {
     for (auto e : args)
         Py_DECREF(e);
 
-    if (jit) {
-        func.var->decvref();
-        for (auto e : args_vars)
-            e->decvref();
-    }
-
     return v;
 }
 
@@ -1693,8 +1658,6 @@ Value ASTInterpreter::visit_attribute(AST_Attribute* node) {
     Value v = visit_expr(node->value);
     Value r(pyston::getattr(v.o, node->attr.getBox()), jit ? jit->emitGetAttr(v, node->attr.getBox(), node) : NULL);
     Py_DECREF(v.o);
-    if (jit)
-        v.var->decvref();
     return r;
 }
 }

src/codegen/baseline_jit.cpp

@@ -278,9 +278,6 @@ RewriterVar* JitFragmentWriter::emitCreateTuple(const llvm::ArrayRef<RewriterVar
     else
         r = call(false, (void*)createTupleHelper, imm(num), allocArgs(values));
 
-    for (auto v : values)
-        v->decvref();
-
     return r;
 }
 
@@ -305,7 +302,6 @@ RewriterVar* JitFragmentWriter::emitGetBlockLocal(InternedString s, int vreg) {
     auto it = local_syms.find(s);
     if (it == local_syms.end())
         return emitGetLocal(s, vreg);
-    it->second->incvref();
     return it->second;
 }
 
@@ -378,7 +374,7 @@ RewriterVar* JitFragmentWriter::emitLandingpad() {
 
 RewriterVar* JitFragmentWriter::emitNonzero(RewriterVar* v) {
     // nonzeroHelper returns bool
-    return call(false, (void*)nonzeroHelper, v);
+    return call(false, (void*)nonzeroHelper, v)->setType(RefType::OWNED);
 }
 
 RewriterVar* JitFragmentWriter::emitNotNonzero(RewriterVar* v) {
@@ -498,11 +494,13 @@ void JitFragmentWriter::emitOSRPoint(AST_Jump* node) {
 }
 
 void JitFragmentWriter::emitPrint(RewriterVar* dest, RewriterVar* var, bool nl) {
+    if (LOG_BJIT_ASSEMBLY) comment("BJIT: emitPrint() start");
     if (!dest)
         dest = call(false, (void*)getSysStdout);
     if (!var)
         var = imm(0ul);
     call(false, (void*)printHelper, dest, var, imm(nl));
+    if (LOG_BJIT_ASSEMBLY) comment("BJIT: emitPrint() end");
 }
 
 void JitFragmentWriter::emitRaise0() {
@@ -514,38 +512,23 @@ void JitFragmentWriter::emitRaise3(RewriterVar* arg0, RewriterVar* arg1, Rewrite
 }
 
 void JitFragmentWriter::emitEndBlock() {
-    for (auto v : local_syms) {
-        if (v.second) {
-            if (LOG_BJIT_ASSEMBLY) {
-                // XXX silly but we need to keep this alive
-                std::string s = std::string("BJIT: decvref(") + v.first.c_str() + ")";
-                comment(*new std::string(s));
-            }
-            v.second->decvref(); // xdecref?
-        }
-    }
+    // XXX remove
 }
 
 void JitFragmentWriter::emitReturn(RewriterVar* v) {
-    v->stealRef();
     addAction([=]() { _emitReturn(v); }, { v }, ActionType::NORMAL);
-    v->decvref();
+    v->refConsumed();
 }
 
 void JitFragmentWriter::emitSetAttr(AST_expr* node, RewriterVar* obj, BoxedString* s, STOLEN(RewriterVar*) attr) {
     attr->stealRef();
     emitPPCall((void*)setattr, { obj, imm(s), attr }, 2, 512, node);
-    attr->decvref();
 }
 
 void JitFragmentWriter::emitSetBlockLocal(InternedString s, STOLEN(RewriterVar*) v) {
     if (LOG_BJIT_ASSEMBLY) comment("BJIT: emitSetBlockLocal() start");
     RewriterVar* prev = local_syms[s];
     local_syms[s] = v;
-    if (prev) {
-        // TODO: xdecref?
-        prev->decvref();
-    }
     if (LOG_BJIT_ASSEMBLY) comment("BJIT: emitSetBlockLocal() end");
 }
 
@@ -558,9 +541,8 @@ void JitFragmentWriter::emitSetExcInfo(RewriterVar* type, RewriterVar* value, Re
 }
 
 void JitFragmentWriter::emitSetGlobal(Box* global, BoxedString* s, STOLEN(RewriterVar*) v) {
-    v->stealRef();
     emitPPCall((void*)setGlobal, { imm(global), imm(s), v }, 2, 512);
-    v->decvref();
+    v->refConsumed();
 }
 
 void JitFragmentWriter::emitSetItem(RewriterVar* target, RewriterVar* slice, RewriterVar* value) {
@@ -585,9 +567,8 @@ void JitFragmentWriter::emitSetLocal(InternedString s, int vreg, bool set_closur
              v);
     } else {
         RewriterVar* prev = vregs_array->getAttr(8 * vreg);
-        v->stealRef();
         vregs_array->setAttr(8 * vreg, v);
-        v->decvref();
+        v->refConsumed();
 
         // TODO With definedness analysis, we could know whether we can skip this check (definitely defined)
         // or not even load the previous value (definitely undefined).
@@ -973,10 +954,6 @@ void JitFragmentWriter::_emitPPCall(RewriterVar* result, void* func_addr, llvm::
         pp_scratch_location -= 8;
     }
 
-    for (RewriterVar* arg : args) {
-        arg->bumpUse();
-    }
-
     assertConsistent();
 
     StackInfo stack_info(pp_scratch_size, pp_scratch_location);
@@ -987,6 +964,17 @@ void JitFragmentWriter::_emitPPCall(RewriterVar* result, void* func_addr, llvm::
     assertConsistent();
 
     result->releaseIfNoUses();
+
+    // TODO: it would be nice to be able to bumpUse on these earlier so that we could potentially avoid spilling
+    // the args across the call if we don't need to.
+    // This had to get moved to the very end of this function due to the fact that bumpUse can cause refcounting
+    // operations to happen.
+    // I'm not sure though that just moving this earlier is good enough though -- we also might need to teach setupCall
+    // that the args might not be needed afterwards?
+    // Anyway this feels like micro-optimizations at the moment and we can figure it out later.
+    for (RewriterVar* arg : args) {
+        arg->bumpUse();
+    }
 }
 
 void JitFragmentWriter::_emitRecordType(RewriterVar* type_recorder_var, RewriterVar* obj_cls_var) {
@@ -1025,6 +1013,20 @@ void JitFragmentWriter::_emitSideExit(STOLEN(RewriterVar*) var, RewriterVar* val
     assert(next_block_var->is_constant);
     uint64_t val = val_constant->constant_value;
 
+    assert(val == (uint64_t)True || val == (uint64_t)False);
+
+    // HAXX ahead:
+    // Override the automatic refcounting system, to force a decref to happen before the jump.
+    // Really, we should probably do a decref on either side post-jump.
+    // But the automatic refcounter doesn't support that, and since the value is either True or False,
+    // we can get away with doing the decref early.
+    // TODO: better solution is to just make NONZERO return a borrowed ref, so we don't have to decref at all.
+    _decref(var);
+    // Hax: override the automatic refcount system
+    assert(var->reftype == RefType::OWNED);
+    assert(var->num_refs_consumed == 0);
+    var->reftype = RefType::BORROWED;
+
     assembler::Register var_reg = var->getInReg();
     if (isLargeConstant(val)) {
         assembler::Register reg = val_constant->getInReg(Location::any(), true, /* otherThan */ var_reg);
@@ -1034,10 +1036,7 @@ void JitFragmentWriter::_emitSideExit(STOLEN(RewriterVar*) var, RewriterVar* val
     }
 
     {
-        // TODO: Figure out if we need a large/small forward based on the number of local syms we will have to decref?
-        assembler::LargeForwardJump jne(*assembler, assembler::COND_EQUAL);
-
-        _decref(var);
+        assembler::ForwardJump jne(*assembler, assembler::COND_EQUAL);
 
         ExitInfo exit_info;
         _emitJump(next_block, next_block_var, exit_info);
@@ -1049,11 +1048,6 @@ void JitFragmentWriter::_emitSideExit(STOLEN(RewriterVar*) var, RewriterVar* val
         }
     }
 
-    if (LOG_BJIT_ASSEMBLY) assembler->comment("BJIT: decreffing emitSideExit var");
-
-    _decref(var);
-    if (LOG_BJIT_ASSEMBLY) assembler->comment("BJIT: decreffing emitSideExit var end");
-
     var->bumpUse();
     val_constant->bumpUse();
 

src/core/options.cpp

@@ -24,8 +24,8 @@ int PYSTON_VERSION_MICRO = 0;
 
 int MAX_OPT_ITERATIONS = 1;
 
-bool LOG_IC_ASSEMBLY = false;
-bool LOG_BJIT_ASSEMBLY = false;
+bool LOG_IC_ASSEMBLY = 0;
+bool LOG_BJIT_ASSEMBLY = 0;
 
 bool CONTINUE_AFTER_FATAL = false;
 bool FORCE_INTERPRETER = true;
@@ -49,7 +49,7 @@ bool FORCE_LLVM_CAPI_CALLS = false;
 bool FORCE_LLVM_CAPI_THROWS = false;
 
 int OSR_THRESHOLD_INTERPRETER = 5; // XXX
-int REOPT_THRESHOLD_INTERPRETER = 5; // XXX
+int REOPT_THRESHOLD_INTERPRETER = 1; // XXX
 int OSR_THRESHOLD_BASELINE = 2500;
 int REOPT_THRESHOLD_BASELINE = 1500;
 int OSR_THRESHOLD_T2 = 10000;

src/runtime/objmodel.cpp

@@ -1023,15 +1023,13 @@ void Box::setattr(BoxedString* attr, Box* val, SetattrRewriteArgs* rewrite_args)
                     RewriterVar* r_hattrs
                         = rewrite_args->obj->getAttr(cls->attrs_offset + offsetof(HCAttrs, attr_list), Location::any());
 
+                    // Don't need to do anything: just getting it and setting it to OWNED
+                    // will tell the auto-refcount system to decref it.
                     r_hattrs->getAttr(offset * sizeof(Box*) + offsetof(HCAttrs::AttrList, attrs))
-                        ->setType(RefType::OWNED)
-                        ->decvref();
+                        ->setType(RefType::OWNED);
                     // TODO make this stealing
-                    rewrite_args->attrval->incvref();
-                    rewrite_args->attrval->stealRef();
                     r_hattrs->setAttr(offset * sizeof(Box*) + offsetof(HCAttrs::AttrList, attrs),
                                       rewrite_args->attrval);
-                    rewrite_args->attrval->decvref();
 
                     rewrite_args->out_success = true;
                 }
@@ -1389,8 +1387,6 @@ Box* nondataDescriptorInstanceSpecialCases(GetattrRewriteArgs* rewrite_args, Box
         } else {
             *bind_obj_out = incref(im_self);
             if (rewrite_args) {
-                r_im_func->incvref();
-                r_im_self->incvref();
                 rewrite_args->setReturn(r_im_func, ReturnConvention::HAS_RETURN);
                 *r_bind_obj_out = r_im_self;
             }
@@ -1951,7 +1947,7 @@ Box* getattrInternalGeneric(Box* obj, BoxedString* attr, GetattrRewriteArgs* rew
     // Look up the class attribute (called `descr` here because it might
     // be a descriptor).
     Box* descr = NULL;
-    AutoDecvref r_descr = NULL;
+    RewriterVar* r_descr = NULL;
     if (rewrite_args) {
         RewriterVar* r_obj_cls = rewrite_args->obj->getAttr(offsetof(Box, cls), Location::any());
         GetattrRewriteArgs grewrite_args(rewrite_args->rewriter, r_obj_cls, rewrite_args->destination);
@@ -2546,13 +2542,11 @@ void setattrGeneric(Box* obj, BoxedString* attr, STOLEN(Box*) val, SetattrRewrit
             raiseAttributeError(obj, attr->s());
         }
 
-        if (rewrite_args)
-            rewrite_args->attrval->stealRef();
-        obj->setattr(attr, val, rewrite_args);
         // TODO: make setattr() stealing
-        Py_DECREF(val);
+        obj->setattr(attr, val, rewrite_args);
         if (rewrite_args)
-            rewrite_args->attrval->decvref();
+            rewrite_args->attrval->refConsumed();
+        Py_DECREF(val);
     }
 
     // TODO this should be in type_setattro
@@ -2661,12 +2655,8 @@ extern "C" void setattr(Box* obj, BoxedString* attr, STOLEN(Box*) attr_val) {
             // rewriter->trap();
             SetattrRewriteArgs rewrite_args(rewriter.get(), rewriter->getArg(0), rewriter->getArg(2));
             setattrGeneric<REWRITABLE>(obj, attr, attr_val, &rewrite_args);
-            if (rewrite_args.out_success) {
-                rewriter->getArg(0)->decvref();
-                rewriter->getArg(1)->decvref();
-                // arg 2 vref got consumed by setattrGeneric
+            if (rewrite_args.out_success)
                 rewriter->commit();
-            }
         } else {
             setattrGeneric<NOT_REWRITABLE>(obj, attr, attr_val, NULL);
         }
@@ -3245,10 +3235,8 @@ Box* callattrInternal(Box* obj, BoxedString* attr, LookupScope scope, CallattrRe
         else
             arg_array->setAttr(8, rewrite_args->rewriter->loadConst(0));
 
-        auto r_rtn = rewrite_args->rewriter->call(true, (void*)Helper::call, arg_vec);
+        auto r_rtn = rewrite_args->rewriter->call(true, (void*)Helper::call, arg_vec)->setType(RefType::OWNED);
         rewrite_args->setReturn(r_rtn, S == CXX ? ReturnConvention::HAS_RETURN : ReturnConvention::CAPI_RETURN);
-        if (r_bind_obj)
-            r_bind_obj->decvref();
 
         void* _args[2] = { args, const_cast<std::vector<BoxedString*>*>(keyword_names) };
         Box* rtn = Helper::call(val, argspec, arg1, arg2, arg3, _args);
@@ -3267,11 +3255,6 @@ Box* callattrInternal(Box* obj, BoxedString* attr, LookupScope scope, CallattrRe
     } else {
         r = runtimeCallInternal<S, NOT_REWRITABLE>(val, NULL, argspec, arg1, arg2, arg3, args, keyword_names);
     }
-    if (rewrite_args) {
-        if (r_bind_obj)
-            r_bind_obj->decvref();
-        r_val->decvref();
-    }
     return r;
 }
 
@@ -3584,12 +3567,6 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
         if (rewrite_args) {
             // TODO should probably rethink the whole vrefcounting thing here.
             // which ones actually need new vrefs?
-            if (num_output_args >= 1)
-                rewrite_args->arg1->incvref();
-            if (num_output_args >= 2)
-                rewrite_args->arg2->incvref();
-            if (num_output_args >= 3)
-                rewrite_args->arg3->incvref();
             if (num_output_args >= 3) {
                 RELEASE_ASSERT(0, "not sure what to do here\n");
                 //for (int i = 0; i < num_output_args - 3; i++) {
@@ -4177,9 +4154,6 @@ Box* callFunc(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args, ArgPassSpe
     }
     if (rewrite_args) {
         RELEASE_ASSERT(num_output_args <= 3, "figure out vrefs for arg array");
-        for (int i = 0; i < num_output_args; i++) {
-            getArg(i, rewrite_args)->decvref();
-        }
     }
 
     return res;