Slow functions calls for C-coded functions, accepting positional-or-kwarg argument vs positional-only

[skirpichev]

Bug report

Bug description:

I don't see measurable difference on pure-Python level, e.g. for

# a.py
import pyperf

def f1(x):
    return 1

def f2(x, /):
    return 1

runner = pyperf.Runner()
runner.bench_func("f1(1)", f1, 1)
runner.bench_func("f2(1)", f2, 1)

I got on 3.13:

$ python a.py -q
f1(1): Mean +- std dev: 233 ns +- 2 ns
f2(1): Mean +- std dev: 232 ns +- 2 ns

But for functions, implemented in C, using the Argument Clinic - the difference is bigger:

$ python bench.py -q -o patch.json
isfinite(1.0): Mean +- std dev: 152 ns +- 1 ns
$ git checkout master && make -s
Checked 114 modules (35 built-in, 78 shared, 1 n/a on linux-x86_64, 0 disabled, 0 missing, 0 failed on import)
$ python bench.py -q -o ref.json
isfinite(1.0): Mean +- std dev: 136 ns +- 1 ns

Benchmark	ref	patch
isfinite(1.0)	136 ns	152 ns: 1.12x slower

(In the second version - the math.isfinite() accept also keyword argument x)

a patch

(run ./python Tools/clinic/clinic.py Modules/mathmodule.c !)

diff --git a/Modules/mathmodule.c b/Modules/mathmodule.c
index 7c2a421dd6..e33281ac8b 100644
--- a/Modules/mathmodule.c
+++ b/Modules/mathmodule.c
@@ -3156,14 +3156,13 @@ math_radians_impl(PyObject *module, double x)
 math.isfinite
 
     x: double
-    /
 
 Return True if x is neither an infinity nor a NaN, and False otherwise.
 [clinic start generated code]*/
 
 static PyObject *
 math_isfinite_impl(PyObject *module, double x)
-/*[clinic end generated code: output=8ba1f396440c9901 input=46967d254812e54a]*/
+/*[clinic end generated code: output=8ba1f396440c9901 input=a47ad6e72998b81d]*/
 {
     return PyBool_FromLong((long)isfinite(x));
 }

This performance penalty affects API decisions (e.g. #131886) and that looks as a bug.

CPython versions tested on:

CPython main branch

Operating systems tested on:

Linux

Linked PRs

• gh-136681: make argsbuf static to speedup processing positional arguments with AC #136732

[serhiy-storchaka]

Look at the generated code. _PyArg_UnpackKeywords has a fast path. I would check if kwnames is NULL here.

[skirpichev]

Look at the generated code. _PyArg_UnpackKeywords has a fast path. I would check if kwnames is NULL here.

No. It's already checked. Twice. Both in _PyArg_UnpackKeywords() function and in the _PyArg_UnpackKeywords() macro. And this path - hits.

The problem is code for initialization of _kwtuple in clinic's generated output:

    #if defined(Py_BUILD_CORE) && !defined(Py_BUILD_CORE_MODULE)

    #define NUM_KEYWORDS 1
    static struct {
        PyGC_Head _this_is_not_used;
        PyObject_VAR_HEAD
        Py_hash_t ob_hash;
        PyObject *ob_item[NUM_KEYWORDS];
    } _kwtuple = {
        .ob_base = PyVarObject_HEAD_INIT(&PyTuple_Type, NUM_KEYWORDS)
        .ob_hash = -1,
        .ob_item = { _Py_LATIN1_CHR('x'), },
    };
    #undef NUM_KEYWORDS
    #define KWTUPLE (&_kwtuple.ob_base.ob_base)

    #else  // !Py_BUILD_CORE
    #  define KWTUPLE NULL
    #endif  // !Py_BUILD_CORE

    static const char * const _keywords[] = {"x", NULL};
    static _PyArg_Parser _parser = {
        .keywords = _keywords,
        .fname = "isfinite",
        .kwtuple = KWTUPLE,
    };
    #undef KWTUPLE
    PyObject *argsbuf[1];
    double x;

    args = _PyArg_UnpackKeywords(args, nargs, NULL, kwnames, &_parser,
            /*minpos*/ 1, /*maxpos*/ 1, /*minkw*/ 0, /*varpos*/ 0, argsbuf);

If I move that to if (kwnames) { ... } block - speed loss is lost.

[serhiy-storchaka]

Static initializer should only be executed once.

[skirpichev]

Hmm, with following in the clinic output:

    if (kwnames) {
        PyObject *argsbuf[1];

        args = _PyArg_UnpackKeywords(args, nargs, NULL, kwnames, &_parser,
                   /*minpos*/ 1, /*maxpos*/ 1, /*minkw*/ 0, /*varpos*/ 0, argsbuf);
    }

I got same numbers as in the master (i.e. with positional-only isfinite) for a simple benchmark:

# a.py
import pyperf
from math import isfinite

runner = pyperf.Runner()
runner.bench_func('isfinite(1.0)', isfinite, 1.0)

(CPython built with default configure flags.)

patch for the AC

diff --git a/Tools/clinic/libclinic/parse_args.py b/Tools/clinic/libclinic/parse_args.py
index 0e15d2f163..bb25576481 100644
--- a/Tools/clinic/libclinic/parse_args.py
+++ b/Tools/clinic/libclinic/parse_args.py
@@ -622,7 +622,6 @@ def parse_general(self, clang: CLanguage) -> None:
                 self.flags = "METH_FASTCALL|METH_KEYWORDS"
                 self.parser_prototype = PARSER_PROTOTYPE_FASTCALL_KEYWORDS
                 self.declarations = declare_parser(self.func, codegen=self.codegen)
-                self.declarations += "\nPyObject *argsbuf[%s];" % (len(self.converters) or 1)
                 if self.varpos:
                     self.declarations += "\nPyObject * const *fastargs;"
                     argsname = 'fastargs'
@@ -640,15 +639,18 @@ def parse_general(self, clang: CLanguage) -> None:
                 argsname = 'fastargs'
                 argname_fmt = 'fastargs[%d]'
                 self.declarations = declare_parser(self.func, codegen=self.codegen)
-                self.declarations += "\nPyObject *argsbuf[%s];" % (len(self.converters) or 1)
                 self.declarations += "\nPyObject * const *fastargs;"
                 self.declarations += "\nPy_ssize_t nargs = PyTuple_GET_SIZE(args);"
                 if has_optional_kw:
                     self.declarations += "\nPy_ssize_t noptargs = %s + (kwargs ? PyDict_GET_SIZE(kwargs) : 0) - %d;" % (nargs, self.min_pos + self.min_kw_only)
                 unpack_args = '_PyTuple_CAST(args)->ob_item, nargs, kwargs, NULL'
             parser_code = [libclinic.normalize_snippet(f"""
-                {argsname} = _PyArg_UnpackKeywords({unpack_args}, &_parser,
-                        /*minpos*/ {self.min_pos}, /*maxpos*/ {self.max_pos}, /*minkw*/ {self.min_kw_only}, /*varpos*/ {1 if self.varpos else 0}, argsbuf);
+                if ({"kwnames" if self.fastcall else "kwargs"}) {{{{
+                    PyObject *argsbuf[{len(self.converters) or 1}];
+
+                    {argsname} = _PyArg_UnpackKeywords({unpack_args}, &_parser,
+                               /*minpos*/ {self.min_pos}, /*maxpos*/ {self.max_pos}, /*minkw*/ {self.min_kw_only}, /*varpos*/ {1 if self.varpos else 0}, argsbuf);
+                }}}}
                 if (!{argsname}) {{{{
                     goto exit;
                 }}}}

Though, if I benchmark a loop with:

# b.py
import pyperf
from math import isfinite

def f(n):
    for _ in range(n):
        isfinite(1.0)

runner = pyperf.Runner()
for n in [1, 2, 10, 100]:
    s = f'isfinite(1.0) x {n:3} times'
    runner.bench_func(s, f, n)

it's still worse!. I got

Benchmark	ref	patch	patch0
isfinite(1.0) x 1 times	661 ns	699 ns: 1.06x slower	717 ns: 1.08x slower
isfinite(1.0) x 2 times	757 ns	802 ns: 1.06x slower	842 ns: 1.11x slower
isfinite(1.0) x 10 times	1.38 us	1.56 us: 1.13x slower	1.70 us: 1.23x slower
isfinite(1.0) x 100 times	8.40 us	10.1 us: 1.21x slower	11.3 us: 1.34x slower
Geometric mean	(ref)	1.11x slower	1.19x slower

where patch0 - version without if (kwnames) condition.

I suspect this might be related to specialization for c-methods calls in Python/specialize.c. (Edit: indeed, it's because specialize_c_call().)

@serhiy-storchaka, the AC patch alone does make sense for you?

[serhiy-storchaka]

No, this change is not correct. _PyArg_UnpackKeywords cannot be omitted if kwnames is NULL. For example, it checks that the correct number of positional arguments was passed.

There are two implementations of _PyArg_UnpackKeywords -- as a macro and as a C function. The macro version handles the fast path, it should add very little overhead. The function implementation is only used for non-fast path. So my suspect is that for some reasons the fast path may be not correctly detected, so the code goes the general slow path. Please check if the _PyArg_UnpackKeywords function was called in that case.

[skirpichev]

No, this change is not correct. _PyArg_UnpackKeywords cannot be omitted if kwnames is NULL. For example, it checks that the correct number of positional arguments was passed.

That was just a draft:)

So my suspect is that for some reasons the fast path may be not correctly detected, so the code goes the general slow path. Please check if the _PyArg_UnpackKeywords function was called in that case.

I already did this first. No - fast path was taken.

I think I've found a simpler solution: just add a static decorator for argsbuf. Here benchmark for math.fmin (ref - current main, patch-v0 - support for kwargs, patch-v1 - make also argsbuf static). Up to a noise - the v1 patch match timings in the main branch:

Benchmark	ref	patch-v0	patch-v1
fmin(1.0, 2.0)	168 ns	185 ns: 1.10x slower	not significant
fmin(1.0, 2.0) x 2 times	908 ns	956 ns: 1.05x slower	926 ns: 1.02x slower
fmin(1.0, 2.0) x 10 times	2.04 us	2.23 us: 1.09x slower	2.06 us: 1.01x slower
fmin(1.0, 2.0) x 100 times	14.8 us	16.5 us: 1.11x slower	not significant
Geometric mean	(ref)	1.09x slower	1.01x slower

# bench.py
import pyperf
from math import fmin

def f(n):
    for _ in range(n):
        fmin(1.0, 2.0)

runner = pyperf.Runner()
runner.bench_func('fmin(1.0, 2.0)', fmin, 1.0, 2.0)
for n in [2, 10, 100]:
    s = f'fmin(1.0, 2.0) x {n:3} times'
    runner.bench_func(s, f, n)

For one-arg functions (like isfinite()) - the difference holds also in the v1 version for loops. I hope this could be fixed too, using different specialized instructions.