`_Py_CallBuiltinClass_StackRefSteal` does not declare `PY_VECTORCALL_ARGUMENTS_OFFSET`

[wjakob]

Bug report

Bug description:

Python's ceval.c contains the function

PyObject *
_Py_CallBuiltinClass_StackRefSteal(
    _PyStackRef callable,
    _PyStackRef *arguments,
    int total_args)
{
    PyObject *res;
    STACKREFS_TO_PYOBJECTS(arguments, total_args, args_o);
    if (CONVERSION_FAILED(args_o)) {
        res = NULL;
        goto cleanup;
    }
    PyTypeObject *tp = (PyTypeObject *)PyStackRef_AsPyObjectBorrow(callable);
    res = tp->tp_vectorcall((PyObject *)tp, args_o, total_args, NULL);       /* <----- relevant line */
   ....
}

The call to tp_vectorcall does not declare PY_VECTORCALL_ARGUMENTS_OFFSET despite STACKREFS_TO_PYOBJECTS (via _PyObjectArray_FromStackRefArray) always allocating an args_o that has an extra scratch space at position -1.

Strictly speaking, this isn't incorrect. But it can force the caller to make a redundant copy of the arguments, which is inefficient. Best practice to allocate the extra entry and then communicate its presence to the vector-callee.

This inefficiency is not restricted to Python-internal types. It also affects custom heap types that are themselves vector-callable. In this case, Python's adaptive optimizing interpreter replaces CALL_NON_PY_GENERAL with CALL_BUILTIN_CLASS that then goes through this code path.

cc @markshannon @vstinner

CPython versions tested on:

3.14

Operating systems tested on:

macOS

Linked PRs

• gh-143361: Pass PY_VECTORCALL_ARGUMENTS_OFFSET in _Py_CallBuiltinClass_StackRefSteal #143367

[Fidget-Spinner]

I don't think 3.14 is affected by this right? It should be only the main branch?

[a12k]

I don't think 3.14 is affected by this right? It should be only the main branch?

Yeah this function doesn't exist at all in 3.14 or earlier. Only in the main branch. I expect the change would be pretty trivial, changing the flags passed to vectorcall.

[a12k]

Sorry for the double comment, but was intrigued and taking a look at this with some findings. I was able to verify a fix with LLDB.

Before fix: LLDB shows nargsf = 1.

With fix: Adding total_args | PY_VECTORCALL_ARGUMENTS_OFFSET results in nargsf = 0x8000000000000001, which shows available offset to the callee and avoids redundant allocation/copy.

Also did a quick perf test with the fix locally with time taken for 5M allocations at 1.6193s pre-fix, and 1.5560s post-fix, about a 3.9% speedup, which seems pretty strong.

I have a patch ready for this and would be happy to submit a PR if this is a direction the core contribs want to take.

[Fidget-Spinner]

Yes please go ahead, I will merge this.

[a12k]

@Fidget-Spinner made the change in a PR above!

[wjakob]

@Fidget-Spinner @a12k

I realized I should not have looked at the master branch. That same inefficiency is also present in the latest CPython 3.14. In this case, it is in bytecodes.c

        op(_CALL_BUILTIN_CLASS, (callable, self_or_null, args[oparg] -- res)) {
            PyObject *callable_o = PyStackRef_AsPyObjectBorrow(callable);
            DEOPT_IF(!PyType_Check(callable_o));
            PyTypeObject *tp = (PyTypeObject *)callable_o;
            int total_args = oparg;
            _PyStackRef *arguments = args;
            if (!PyStackRef_IsNull(self_or_null)) {
                arguments--;
                total_args++;
            }
            DEOPT_IF(tp->tp_vectorcall == NULL);
            STAT_INC(CALL, hit);
            STACKREFS_TO_PYOBJECTS(arguments, total_args, args_o);
            if (CONVERSION_FAILED(args_o)) {
                DECREF_INPUTS();
                ERROR_IF(true);
            }
            PyObject *res_o = tp->tp_vectorcall((PyObject *)tp, args_o, total_args, NULL); /* <--- HERE */
            STACKREFS_TO_PYOBJECTS_CLEANUP(args_o);
            DECREF_INPUTS();
            ERROR_IF(res_o == NULL);
            res = PyStackRef_FromPyObjectSteal(res_o);
        }

[a12k]

Sure enough @wjakob, not the same function name but identical logic. @Fidget-Spinner would you recommend I add a patch for bytecodes.c to my PR, or create a second PR with the additional bytecodes fix?

[Fidget-Spinner]

@a12k we can only backport if it was a prior bug/regression. Eg if in 3.13 we set the flag and in 3.14 we didnt, then backport is fine. Otherwise we cant backport perf fixes to maintenance branches, or I would be backporting the JIT 😀

[Fidget-Spinner]

Does anyone know if we need to backport (ie, this was a regression compared to 3.13?) If not, I'm closing this issue as done.