Improve rewritten passing of >3 args

We would incref them before putting them into the args array.
Now we keep some extra information during the rewriting process, which
lets rearrangeArguments say whether any of the values need to
be decreffed.  It's pretty rare that they need to so I think this
should be some nice savings (20% on a specifically-targeted microbenchmark,
no change on raytrace.py).

I'm a little bit worried though that since it is so rare to need to do the
decref, it would be easy to forget to check for it.

Well I think we already were forgetting it in some places... but at least now
it's an explicit arg you have to manage so maybe it's less likely to cause mistakes.

src/capi/typeobject.cpp

@@ -3348,7 +3348,7 @@ static Box* tppProxyToTpCall(Box* self, CallRewriteArgs* rewrite_args, ArgPassSp
     Box** oargs = NULL;
     try {
         rearrangeArguments(paramspec, NULL, "", NULL, rewrite_args, rewrite_success, argspec, arg1, arg2, arg3, args,
-                           oargs, keyword_names);
+                           oargs, keyword_names, NULL);
     } catch (ExcInfo e) {
         if (S == CAPI) {
             setCAPIException(e);

src/runtime/builtin_modules/builtins.cpp

@@ -551,7 +551,7 @@ Box* getattrFuncInternal(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args,
     static Box* defaults[] = { NULL };
     bool rewrite_success = false;
     rearrangeArguments(ParamReceiveSpec(3, 1, false, false), NULL, "getattr", defaults, rewrite_args, rewrite_success,
-                       argspec, arg1, arg2, arg3, args, NULL, keyword_names);
+                       argspec, arg1, arg2, arg3, args, NULL, keyword_names, NULL);
     if (!rewrite_success)
         rewrite_args = NULL;
 
@@ -685,7 +685,7 @@ Box* hasattrFuncInternal(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args,
                          Box* arg2, Box* arg3, Box** args, const std::vector<BoxedString*>* keyword_names) {
     bool rewrite_success = false;
     rearrangeArguments(ParamReceiveSpec(2, 0, false, false), NULL, "hasattr", NULL, rewrite_args, rewrite_success,
-                       argspec, arg1, arg2, arg3, args, NULL, keyword_names);
+                       argspec, arg1, arg2, arg3, args, NULL, keyword_names, NULL);
     if (!rewrite_success)
         rewrite_args = NULL;
 

src/runtime/capi.cpp

@@ -1802,7 +1802,7 @@ Box* BoxedCApiFunction::tppCall(Box* _self, CallRewriteArgs* rewrite_args, ArgPa
 
     bool rewrite_success = false;
     rearrangeArguments(paramspec, NULL, self->method_def->ml_name, defaults, rewrite_args, rewrite_success, argspec,
-                       arg1, arg2, arg3, args, oargs, keyword_names);
+                       arg1, arg2, arg3, args, oargs, keyword_names, NULL);
 
     AUTO_DECREF_ARGS(paramspec, arg1, arg2, arg3, oargs);
 

src/runtime/descr.cpp

@@ -308,16 +308,20 @@ Box* BoxedMethodDescriptor::tppCall(Box* _self, CallRewriteArgs* rewrite_args, A
         assert((paramspec.totalReceived() - 3) <= sizeof(oargs_array) / sizeof(oargs_array[0]));
         oargs = oargs_array;
     }
+    bool oargs_owned[1];
 
     bool rewrite_success = false;
     rearrangeArguments(paramspec, NULL, self->method->ml_name, defaults, rewrite_args, rewrite_success, argspec, arg1,
-                       arg2, arg3, args, oargs, keyword_names);
+                       arg2, arg3, args, oargs, keyword_names, oargs_owned);
 
     AUTO_DECREF_ARGS(paramspec, arg1, arg2, arg3, oargs);
 
     if (!rewrite_success)
         rewrite_args = NULL;
 
+    if (rewrite_args && oargs)
+        decrefOargs(rewrite_args->args, oargs_owned, 1);
+
     if (ml_flags & METH_CLASS) {
         rewrite_args = NULL;
         if (!PyType_Check(arg1))
@@ -629,7 +633,7 @@ Box* BoxedWrapperDescriptor::tppCall(Box* _self, CallRewriteArgs* rewrite_args,
 
     bool rewrite_success = false;
     rearrangeArguments(paramspec, NULL, self->wrapper->name.data(), NULL, rewrite_args, rewrite_success, argspec, arg1,
-                       arg2, arg3, args, oargs, keyword_names);
+                       arg2, arg3, args, oargs, keyword_names, NULL);
 
     AUTO_DECREF_ARGS(paramspec, arg1, arg2, arg3, oargs);
 

src/runtime/objmodel.cpp

@@ -3933,11 +3933,19 @@ public:
     }
 };
 
+void decrefOargs(RewriterVar* oargs, bool* oargs_owned, int num_oargs) {
+    for (int i = 0; i < num_oargs; i++) {
+        if (oargs_owned[i])
+            oargs->getAttr(i * sizeof(Box*))->setType(RefType::OWNED);
+    }
+}
+
 template <Rewritable rewritable, typename FuncNameCB>
 void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* param_names, FuncNameCB func_name_cb,
                                 Box** defaults, _CallRewriteArgsBase* rewrite_args, bool& rewrite_success,
                                 ArgPassSpec argspec, Box*& oarg1, Box*& oarg2, Box*& oarg3, Box** args, Box** oargs,
-                                const std::vector<BoxedString*>* keyword_names) {
+                                const std::vector<BoxedString*>* keyword_names,
+                                bool* oargs_owned) {
     if (rewritable == NOT_REWRITABLE) {
         assert(!rewrite_args);
         rewrite_args = NULL;
@@ -3957,6 +3965,11 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
     assert((oargs != NULL) == (num_output_args > 3));
     assert((defaults != NULL) == (paramspec.num_defaults != 0));
 
+    if (rewrite_args && oargs) {
+        assert(oargs_owned);
+        memset(oargs_owned, 0, (num_output_args - 3) * sizeof(bool));
+    }
+
     if (rewrite_args) {
         rewrite_success = false; // default case
     }
@@ -3974,16 +3987,6 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
                 Py_XINCREF(oargs[i]);
             }
         }
-        if (rewrite_args) {
-            if (num_output_args >= 3) {
-                for (int i = 0; i < num_output_args - 3; i++) {
-                    rewrite_args->args->getAttr(i * sizeof(Box*))
-                        ->setType(RefType::BORROWED)
-                        ->setNullable(argspec.has_kwargs)
-                        ->refConsumed();
-                }
-            }
-        }
     };
 
     // Super fast path:
@@ -4113,11 +4116,6 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
     for (int i = 0; i < positional_to_positional; i++) {
         getArg(i, oarg1, oarg2, oarg3, oargs) = incref(getArg(i, arg1, arg2, arg3, args));
     }
-    if (rewrite_args) {
-        for (int i = 3; i < positional_to_positional; i++) {
-            rewrite_args->args->getAttr((i - 3) * sizeof(Box*))->setType(RefType::BORROWED)->refConsumed();
-        }
-    }
 
     int varargs_to_positional = std::min((int)varargs_size, paramspec.num_args - positional_to_positional);
     for (int i = 0; i < varargs_to_positional; i++) {
@@ -4163,6 +4161,7 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
 
             RewriterVar* varargs_val;
             int varargs_size = unused_positional_rvars.size();
+            bool is_owned = false;
 
             if (varargs_size == 0) {
                 varargs_val
@@ -4182,6 +4181,7 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
                 };
                 varargs_val = rewrite_args->rewriter->call(false, create_ptrs[varargs_size], unused_positional_rvars)
                                   ->setType(RefType::OWNED);
+                is_owned = true;
             }
 
             if (varargs_val) {
@@ -4193,10 +4193,13 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
                     rewrite_args->arg3 = varargs_val;
                 if (varargs_idx >= 3) {
                     rewrite_args->args->setAttr((varargs_idx - 3) * sizeof(Box*), varargs_val);
+                    if (is_owned) {
+                        oargs_owned[varargs_idx - 3] = true;
                         varargs_val->refConsumed();
                     }
                 }
             }
+        }
 
         Box* ovarargs;
         if (argspec.num_args == 0 && paramspec.num_args == 0 && (!varargs || varargs->cls == tuple_cls)) {
@@ -4394,10 +4397,7 @@ void rearrangeArgumentsInternal(ParamReceiveSpec paramspec, const ParamNames* pa
                     = rewrite_args->rewriter->loadConst((intptr_t)default_obj, Location::forArg(2))->setType(RefType::BORROWED);
             else {
                 auto rvar = rewrite_args->rewriter->loadConst((intptr_t)default_obj);
-                rvar->setType(RefType::BORROWED);
                 rewrite_args->args->setAttr((arg_idx - 3) * sizeof(Box*), rvar);
-                if (default_obj)
-                    rvar->refConsumed();
             }
         }
 
@@ -4409,17 +4409,18 @@ template <Rewritable rewritable>
 void rearrangeArguments(ParamReceiveSpec paramspec, const ParamNames* param_names, const char* func_name,
                         Box** defaults, _CallRewriteArgsBase* rewrite_args, bool& rewrite_success, ArgPassSpec argspec,
                         Box*& oarg1, Box*& oarg2, Box*& oarg3, Box** args, Box** oargs,
-                        const std::vector<BoxedString*>* keyword_names) {
+                        const std::vector<BoxedString*>* keyword_names, bool* oargs_owned) {
     auto func = [func_name]() { return func_name; };
     return rearrangeArgumentsInternal<rewritable>(paramspec, param_names, func, defaults, rewrite_args, rewrite_success,
-                                                  argspec, oarg1, oarg2, oarg3, args, oargs, keyword_names);
+                                                  argspec, oarg1, oarg2, oarg3, args, oargs, keyword_names,
+                                                  oargs_owned);
 }
 template void rearrangeArguments<REWRITABLE>(ParamReceiveSpec, const ParamNames*, const char*, Box**,
                                              _CallRewriteArgsBase*, bool&, ArgPassSpec, Box*&, Box*&, Box*&, Box**,
-                                             Box**, const std::vector<BoxedString*>*);
+                                             Box**, const std::vector<BoxedString*>*, bool*);
 template void rearrangeArguments<NOT_REWRITABLE>(ParamReceiveSpec, const ParamNames*, const char*, Box**,
                                                  _CallRewriteArgsBase*, bool&, ArgPassSpec, Box*&, Box*&, Box*&, Box**,
-                                                 Box**, const std::vector<BoxedString*>*);
+                                                 Box**, const std::vector<BoxedString*>*, bool*);
 
 static StatCounter slowpath_callfunc("slowpath_callfunc");
 template <ExceptionStyle S, Rewritable rewritable>
@@ -4452,7 +4453,8 @@ Box* callFunc(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args, ArgPassSpe
         rewrite_args->rewriter->addDependenceOn(func->dependent_ics);
     }
 
-    Box** oargs;
+    Box** oargs = NULL;
+    bool* oargs_owned = NULL;
     bool rewrite_success = false;
 
     int num_output_args = paramspec.totalReceived();
@@ -4462,19 +4464,18 @@ Box* callFunc(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args, ArgPassSpe
         int size = (num_output_args - 3) * sizeof(Box*);
         oargs = (Box**)alloca(size);
 
+        oargs_owned = (bool*)alloca((num_output_args - 3) * sizeof(bool));
+
 #ifndef NDEBUG
         memset(&oargs[0], 0, size);
 #endif
-    } else {
-        // It won't get looked at, but the compiler wants this:
-        oargs = NULL;
     }
 
     try {
         auto func_name_cb = [md]() { return getFunctionName(md).data(); };
         rearrangeArgumentsInternal<rewritable>(paramspec, &md->param_names, func_name_cb,
                                                paramspec.num_defaults ? func->defaults->elts : NULL, rewrite_args,
-                                               rewrite_success, argspec, arg1, arg2, arg3, args, oargs, keyword_names);
+                                               rewrite_success, argspec, arg1, arg2, arg3, args, oargs, keyword_names, oargs_owned);
     } catch (ExcInfo e) {
         if (S == CAPI) {
             setCAPIException(e);
@@ -4580,11 +4581,8 @@ Box* callFunc(BoxedFunctionBase* func, CallRewriteArgs* rewrite_args, ArgPassSpe
                                         arg3, oargs);
     }
 
-    if (rewrite_args && num_output_args > 3) {
-        for (int i = 0; i < num_output_args - 3; i++) {
-            rewrite_args->args->getAttr(i * sizeof(Box*))->setType(RefType::OWNED)->setNullable(true);
-        }
-    }
+    if (rewrite_args && num_output_args > 3)
+        decrefOargs(rewrite_args->args, oargs_owned, num_output_args - 3);
 
     return res;
 }

src/runtime/rewrite_args.h

@@ -304,12 +304,14 @@ template <Rewritable rewritable = REWRITABLE>
 void rearrangeArguments(ParamReceiveSpec paramspec, const ParamNames* param_names, const char* func_name,
                         Box** defaults, _CallRewriteArgsBase* rewrite_args, bool& rewrite_success, ArgPassSpec argspec,
                         Box*& arg1, Box*& arg2, Box*& arg3, Box** args, Box** oargs,
-                        const std::vector<BoxedString*>* keyword_names);
+                        const std::vector<BoxedString*>* keyword_names, bool* oargs_owned);
 
 // new_args should be allocated by the caller if at least three args get passed in.
 // rewrite_args will get modified in place.
 ArgPassSpec bindObjIntoArgs(Box* bind_obj, RewriterVar* r_bind_obj, _CallRewriteArgsBase* rewrite_args,
                             ArgPassSpec argspec, Box*& arg1, Box*& arg2, Box*& arg3, Box** args, Box** new_args);
+
+void decrefOargs(RewriterVar* oargs, bool* oargs_owned, int oargs_size);
 } // namespace pyston
 
 #endif

src/runtime/types.cpp

@@ -761,10 +761,10 @@ static Box* typeCallInner(CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Bo
         static ParamNames param_names({ "", "string", "encoding", "errors" }, "", "");
         static Box* defaults[3] = { NULL, NULL, NULL };
         Box* oargs[1];
-
+        bool oargs_owned[1];
 
         rearrangeArguments(paramspec, &param_names, "unicode", defaults, rewrite_args, rewrite_success, argspec, arg1,
-                           arg2, arg3, args, oargs, keyword_names);
+                           arg2, arg3, args, oargs, keyword_names, oargs_owned);
         assert(arg1 == cls);
 
         AUTO_DECREF_ARGS(paramspec, arg1, arg2, arg3, oargs);
@@ -772,6 +772,9 @@ static Box* typeCallInner(CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Bo
         if (!rewrite_success)
             rewrite_args = NULL;
 
+        if (rewrite_args)
+            decrefOargs(rewrite_args->args, oargs_owned, 1);
+
         if (rewrite_args) {
             rewrite_args->out_rtn
                 = rewrite_args->rewriter->call(true, (void*)unicodeNewHelper, rewrite_args->arg1, rewrite_args->arg2,
@@ -794,7 +797,7 @@ static Box* typeCallInner(CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Bo
         bool rewrite_success = false;
         Box** oargs = NULL;
         rearrangeArguments(paramspec, NULL, "", NULL, rewrite_args, rewrite_success, argspec, arg1, arg2, arg3, args,
-                           oargs, keyword_names);
+                           oargs, keyword_names, NULL);
         assert(arg1 == cls);
 
         AUTO_DECREF(arg1);
@@ -1121,7 +1124,7 @@ static Box* typeCallInner(CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Bo
         bool rewrite_success = false;
         try {
             rearrangeArguments(ParamReceiveSpec(1, 0, true, true), NULL, "", NULL, rewrite_args, rewrite_success,
-                               argspec, made, arg2, arg3, args, NULL, keyword_names);
+                               argspec, made, arg2, arg3, args, NULL, keyword_names, NULL);
         } catch (ExcInfo e) {
             if (S == CAPI) {
                 setCAPIException(e);
@@ -1256,7 +1259,7 @@ static Box* typeCallInner(CallRewriteArgs* rewrite_args, ArgPassSpec argspec, Bo
             bool rewrite_success = false;
             try {
                 rearrangeArguments(ParamReceiveSpec(1, 0, true, true), NULL, "", NULL, rewrite_args, rewrite_success,
-                                   argspec, made, arg2, arg3, args, NULL, keyword_names);
+                                   argspec, made, arg2, arg3, args, NULL, keyword_names, NULL);
             } catch (ExcInfo e) {
                 Py_DECREF(made);
                 if (S == CAPI) {

test/tests/func_arg_passing.py

@@ -36,6 +36,18 @@ def f2(a, b=3, c=4, d=5, *args, **kw):
     print a, b, c, d, args, kw
 
 def g():
+    try:
+        print f2(1)
+    except Exception as e:
+        print e
+    try:
+        print f2(1, 2)
+    except Exception as e:
+        print e
+    try:
+        print f2(1, 2, 3)
+    except Exception as e:
+        print e
     print f2(1, 2, 3, 4)
     print f2(1, 2, 3, 4, k=1)
     print f2(1, 2, 3, 4, 5)

test/tests/list.py

@@ -236,3 +236,5 @@ try:
     print([1, 2] * d)
 except TypeError as e:
     print(type(e))
+
+print range(5).index(10, 100L, 200L)

test/tests/override_assertionerror.py

-# expected: reffail
 
 orig_ae = AssertionError
 class MyAssertionError(Exception):