[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: 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 --

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

hydroflask added the comment: Easy one to make, might be worth adding a test that would have exercised that code path. -- ___ Python tracker ___

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: > Was reviewing the code and noticed that a double-free was introduced in the > recent changes: Thanks, that looks like my mistake -- ___ Python tracker

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Change by Dong-hee Na : -- pull_requests: +29434 pull_request: https://github.com/python/cpython/pull/31272 ___ Python tracker ___

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

hydroflask added the comment: Ignore the previous comment :) -- ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

hydroflask added the comment: 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. --

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

hydroflask added the comment: 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

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

hydroflask added the comment: Thanks again everyone, very much appreciated. -- ___ Python tracker ___ ___ Python-bugs-list mailing

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: 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 --

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Change by Dong-hee Na : -- pull_requests: +29418 pull_request: https://github.com/python/cpython/pull/31249 ___ Python tracker ___

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: @hydroflask @vstinner Okay let's remove if statement, I will send a patch soon. -- ___ Python tracker ___

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

hydroflask added the comment: 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

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

STINNER Victor added the comment: 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). -- ___ Python tracker

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

hydroflask added the comment: @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.

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: 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 --

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

STINNER Victor added the comment: I suggest to use PyTuple_GET_ITEM(), but PySequence_Fast_ITEMS() is more effecient. It's hard to beat an array in C. -- ___ Python tracker

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

hydroflask added the comment: 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

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: And it even better from performance view -- ___ Python tracker ___ ___ Python-bugs-list mailing

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: @hydroflask Sound reasonable, I submitted a new patch to use alloca which is already used in _ctypes module. -- ___ Python tracker ___

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Change by Dong-hee Na : -- pull_requests: +29395 pull_request: https://github.com/python/cpython/pull/31224 ___ Python tracker ___

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

hydroflask added the comment: 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:

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: @hydroflask All patches are merged! Thanks for all your suggestions :) -- stage: patch review -> resolved status: open -> closed ___ Python tracker

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: 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

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: 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 --

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: @vstinner PR 31209 is for fixing the leak. -- ___ Python tracker ___ ___ Python-bugs-list mailing

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Change by Dong-hee Na : -- pull_requests: +29379 pull_request: https://github.com/python/cpython/pull/31209 ___ Python tracker ___

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: No leak after if I revert PR 31188 on my local -- ___ Python tracker ___ ___ Python-bugs-list

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: @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]

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

hydroflask added the comment: 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

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

STINNER Victor added the comment: Oh ok, thanks for the clarification :-) -- ___ Python tracker ___ ___ Python-bugs-list mailing

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

hydroflask added the comment: > 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

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

STINNER Victor added the comment: 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 --

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

STINNER Victor added the comment: 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

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Change by STINNER Victor : -- pull_requests: +29359 pull_request: https://github.com/python/cpython/pull/31188 ___ Python tracker ___

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Change by Dong-hee Na : -- resolution: wont fix -> status: closed -> open ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Change by Dong-hee Na : -- stage: resolved -> patch review ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe:

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: With v3 benchmark Mean +- std dev: [base] 1.32 us +- 0.02 us -> [opt] 1.18 us +- 0.02 us: 1.13x faster -- ___ Python tracker ___

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Change by Dong-hee Na : Added file: https://bugs.python.org/file50610/bench_callback_v3.py ___ Python tracker ___ ___ Python-bugs-list

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

hydroflask added the comment: 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! --

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: @hydroflask Would you like to run your benchmark with my latest PR? -- ___ Python tracker ___ ___

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: @vstinner I got a similar result from hydroflask's benchmark. I think that is worth reopening this issue WDYT? -- nosy: +erlendaasland, vstinner ___ Python tracker

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

hydroflask added the comment: 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

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: @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

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

hydroflask added the comment: @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

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Dong-hee Na added the comment: @hydroflask It does not show impressive performance enhancement, so I decided not to change it See PR 31138 -- resolution: -> wont fix stage: patch review -> resolved status: open -> closed ___ Python tracker

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Change by Dong-hee Na : Added file: https://bugs.python.org/file50604/bench_callback.py ___ Python tracker ___ ___ Python-bugs-list mailing

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Change by Dong-hee Na : -- keywords: +patch pull_requests: +29316 stage: -> patch review pull_request: https://github.com/python/cpython/pull/31138 ___ Python tracker ___

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Change by Dong-hee Na : -- assignee: -> corona10 nosy: +corona10 ___ Python tracker ___ ___ Python-bugs-list mailing list

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Change by Kumar Aditya : -- nosy: -kumaraditya303 ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe:

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Change by STINNER Victor : -- nosy: -vstinner ___ Python tracker ___ ___ Python-bugs-list mailing list Unsubscribe:

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

Kumar Aditya added the comment: Targeting Python 3.11 as it a performance improvement. -- nosy: +kumaraditya303, vstinner versions: -Python 3.10, Python 3.7, Python 3.8, Python 3.9 ___ Python tracker

[issue46323] Use _PyObject_Vectorcall in Modules/_ctypes/callbacks.c

New submission from hydroflask : _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