_CallPythonObject() in Modules/_ctypes/callbacks.c still uses the old API (PyObject_CallObject()) to invoke methods. It should use _PyObject_Vectorcall(). Creating an issue for this since this method is a little more performance sensitive than normal Python library code.

Targeting Python 3.11 as it a performance improvement.

@hydroflask

It does not show impressive performance enhancement, so I decided not to change it
See PR 31138

@corona10

Thank you for looking into it.

The benchmark you did is not reflective of my use-case. I have a codebase that I have completely converted into a C extension using mypyc. So the ctypes callback does not call into interpreted Python code but actually another C-extension. I have annotated types aggressively to avoid invoking malloc(). In this use-case the extra overhead from the tuple allocation would be significant.

Additionally your benchmark may not be as micro as intended. You call some math in the callback function which may have made the savings from avoiding the tuple allocation seem more like noise.

Since you already did the work I still think it's worth putting it in since PyObject_Vectorcall is an established API and it would have been used anyway if this were written from scratch.

@hydroflask

I am one of the big fans of applying vectorcall :)

> The benchmark you did is not reflective of my use-case. I have a codebase that I have completely converted into a C extension using mypyc.

Can you provide me a minimal benchmarkable script(if you need to include mypyc it's okay) to persuade other core dev?

Okay I put together a benchmark that better exemplifies my use case and results are in favor of adding patch.

When bench_callback_v2.py is not compiled with mypyc I see a ~10% reduction in runtime:

    $ unpatched-env/bin/python bench_callback_v2.py
    1.1262263769749552
    $ patched-env/bin/python bench_callback_v2.py
    1.0174838998354971

When bench_callback_v2.py is compiled with mypyc, I am getting ~6% reduction in runtime:

    $ unpatched-env/bin/python -c "import bench_callback_v2"
    1.0056699379347265
    $ patched-env/bin/python -c "import bench_callback_v2"
    0.9415687420405447

bench_callback_v2.py is attached.

@vstinner

I got a similar result from hydroflask's benchmark.
I think that is worth reopening this issue WDYT?

@hydroflask

Would you like to run your benchmark with my latest PR?

Vanilla bench_callback_v2.py, not compiled with mypyc:

    $ env/bin/python bench_callback_v2.py
    1.114047016017139
    $ env2/bin/python bench_callback_v2.py
    0.9644024870358407

~13% reduction in runtime. Nice job!

With v3 benchmark

Mean +- std dev: [base] 1.32 us +- 0.02 us -> [opt] 1.18 us +- 0.02 us: 1.13x faster

A benchmark on calling C functions using ctypes sounds better than a benchmark calling Python functions. For Python functions, Python objects are converted to C types, and then C types are convered back to Python objects, the Python result is converted to a C type, and then the C type is converted back to a Python object for the final result...

It would be nice to have a benchmark on different number of arguments. Like: 0, 1, 3, 5, 8, 10.

New changeset 4cce1352bb47babaeefb68fcfcc48ffa073745c3 by Victor Stinner in branch 'main':
bpo-46323: _ctypes.CFuncPtr fails if _argtypes_ is too long (GH-31188)
https://github.com/python/cpython/commit/4cce1352bb47babaeefb68fcfcc48ffa073745c3

> A benchmark on calling C functions using ctypes sounds better than a benchmark calling Python functions.

Calling C functions from Python is not the code path handled by _CallPythonObject() so no difference in run-time would theoretically be observed by that benchmark for this patch. This bug report pertains to code paths where a C function calls back into a Python function. A practical example is using Python with an event loop library written in C.

Oh ok, thanks for the clarification :-)

Just to clarify further, the original benchmark by corona10 did indeed lead to `_CallPythonObject` being invoked but it was not quite the normal use-case. In the original benchmark it invoked the generated ctypes thunk via Python so as vstinner said it was doing this:

    Python -> converters -> thunk-> _CallPythonObject -> converters-> Python

Namely using `PyCFuncPtr_call` to invoke the thunk generated by `_ctypes_alloc_callback`. Normally when invoking C functions via `PyCFuncPtr_call` it looks like this:

    Python -> converters -> C_function

In the original benchmark setup no significant reduction in runtime was observed by this patch.  I noticed that it was not a typical use of `_CallPythonObject`, where instead it would be a top-level C function calling back into Python. Something like this:

    C -> thunk -> _CallPythonObject() -> Python

The benchmark I provided exercises that use case and the >=10% reduction in runtime was observed. Thanks to both corona10 and vstinner, I appreciate their effort in this issue.

@vstinner

PR 31188 cause reference leak, please check

Raised RLIMIT_NOFILE: 256 -> 1024
0:00:00 load avg: 5.38 Run tests sequentially
0:00:00 load avg: 5.38 [1/1] test_ctypes
beginning 6 repetitions
123456
......
test_ctypes leaked [1028, 1026, 1028] references, sum=3082
test_ctypes leaked [2, 1, 3] memory blocks, sum=6
test_ctypes failed (reference leak)

== Tests result: FAILURE ==

1 test failed:
    test_ctypes

Total duration: 3.2 sec
Tests result: FAILURE

No leak after if I revert PR 31188 on my local

@vstinner

PR 31209 is for fixing the leak.

New changeset e959dd9f5c1d8865d4e10c49eb30ee0a4fe2735d by Dong-hee Na in branch 'main':
bpo-46323 Fix ref leak if ctypes.CFuncPtr raises an error. (GH-31209)
https://github.com/python/cpython/commit/e959dd9f5c1d8865d4e10c49eb30ee0a4fe2735d

New changeset b5527688aae11d0b5af58176267a9943576e71e5 by Dong-hee Na in branch 'main':
bpo-46323: Use PyObject_Vectorcall while calling ctypes callback function (GH-31138)
https://github.com/python/cpython/commit/b5527688aae11d0b5af58176267a9943576e71e5

@hydroflask

All patches are merged!
Thanks for all your suggestions :)

Two things, one is a nit pick the other is more serious. I think vstinner mentioned this in his review of your patch but on this line:

https://github.com/python/cpython/commit/b5527688aae11d0b5af58176267a9943576e71e5#diff-706e65ee28911740bf638707e19578b8182e57c6a8a9a4a91105d825f95a139dR180

Instead of using PySequence_Fast_ITEMS(), you can just use PyTuple_GET_ITEM() since you know the converters are a tuple. In terms of runtime efficiency it doesn't change anything but is consistent with this line: https://github.com/python/cpython/commit/b5527688aae11d0b5af58176267a9943576e71e5#diff-706e65ee28911740bf638707e19578b8182e57c6a8a9a4a91105d825f95a139dR157

Though on second thought I think PySequence_Fast_ITEMS() is probably a better API overall in terms of efficiency if PyTuple_GET_ITEM() would eventually become a real function call given the long term push toward a stable API.

The other issue is more serious, you are always allocating an array of CTYPES_MAX_ARGCOUNT pointers on the stack on every C callback. This could cause stack overflows in situations where a relatively deep set of callbacks are invoked. This usage of CTYPES_MAX_ARGCOUNT differs its usage in callproc.c, where in that case `alloca()` is used to allocate the specific number of array entries on the stack. To avoid an unintended stack overflow I would use alloca() or if you don't like alloca() I would only allocate a relatively small constant number on the stack.

@hydroflask

Sound reasonable, I submitted a new patch to use alloca which is already used in _ctypes module.

And it even better from performance view

I don't have github so I can't comment there but just as a nitpick, alloca() never returns NULL, so you don't have to check that. I know that is inconsistent with its use in callproc.c so for consistency's sake the NULL check should stay but I would remove both checks in a future change.

More importantly you can completely avoid the args_stack variable since alloca() has the same end affect. I would also add an assert that the number of args is <= CTYPES_MAX_ARGCOUNT, that should be the case since GH 31188

I suggest to use PyTuple_GET_ITEM(), but PySequence_Fast_ITEMS() is more effecient. It's hard to beat an array in C.

New changeset d18120cd67b4297f78bfc9bf7b36774cf0bf15f2 by Dong-hee Na in branch 'main':
bpo-46323: Reduce stack usage of ctypes python callback function. (GH-31224)
https://github.com/python/cpython/commit/d18120cd67b4297f78bfc9bf7b36774cf0bf15f2

@corona10

I really hope I am not being annoying at this point :) One final nit, in this line:

https://github.com/python/cpython/pull/31224/files#diff-706e65ee28911740bf638707e19578b8182e57c6a8a9a4a91105d825f95a139dR168

You do not have to check if nargs > 0. alloca() can handle the case when nargs == 0. The usage of alloca() in callproc.c does not check for nargs > 0 either.

Otherwise thanks for this enhancement!

I failed to find the doc about alloca(0). It seems like the existing _ctypes_callproc() function *can* call alloca(0) if argtuple is empty and pIunk is 0 (pIunk is specific to Windows).

For reference, here are MSDN, Linux, OpenBSD, and GCC docs on alloca:

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/alloca?view=msvc-170

https://www.man7.org/linux/man-pages/man3/alloca.3.html

https://man.openbsd.org/alloca.3

https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_002a_005f_005fbuiltin_005falloca

@hydroflask @vstinner

Okay let's remove if statement, I will send a patch soon.

New changeset db052851a70fd95d047c6263fc16a75e4d47b3ed by Dong-hee Na in branch 'main':
bpo-46323: Allow alloca(0) for python callback function of ctypes (GH-31249)
https://github.com/python/cpython/commit/db052851a70fd95d047c6263fc16a75e4d47b3ed

Thanks again everyone, very much appreciated.

Was reviewing the code and noticed that a double-free was introduced in the recent changes:

https://github.com/python/cpython/blob/dd76b3f7d332dd6eced5cbc2ad2adfc397700b3d/Modules/_ctypes/callbacks.c#L192

That line should have been removed in https://github.com/python/cpython/commit/b5527688aae11d0b5af58176267a9943576e71e5 but it was missed!

Another bug, the array returned by alloca() should be zero-initialized. If an early return happens because of an intermediate error, Py_DECREF() will be called on uninitialized memory. Also it should be Py_XDECREF() I think.

Ignore the previous comment :)

> Was reviewing the code and noticed that a double-free was introduced in the recent changes:

Thanks, that looks like my mistake

Easy one to make, might be worth adding a test that would have exercised that code path.

New changeset 0ac5372bf6b937ed44a8f9c4e402d024fcd80870 by Dong-hee Na in branch 'main':
bpo-46323: Fix double-free issue for borrowed refs (GH-31272)
https://github.com/python/cpython/commit/0ac5372bf6b937ed44a8f9c4e402d024fcd80870