bpo-29548: Fix some inefficient call API usage by methane · Pull Request #97 · python/cpython (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation7 Commits4 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

While bpo-29548, some inefficient PyEval_Call*() usage are found.
This is spin off pull request for fixing them.

Use _PyObject_CallNoArg() when there are no arguments.
Use PyObject_CallMethod instead of manually calling Py_BuildValue(). (temporary tuple may be skipped)

PyTuple_SetItem(args, 1, op2);
if ((result = PyEval_CallObject(func, args)) == NULL)
PyObject *args[] = {result, op2};
if ((result = _PyObject_FastCall(func, args, 2)) == NULL) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind to move the assignment out of the if() please? "result = ...; if (result == NULL) ...".

for (;;) {
PyObject *op2;

if (args->ob_refcnt > 1) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect a ltitle slowdown if the function doesn't support fastcall. For example, a Python object with a call() method.

Can you please try to run a quick benchmark on the following two cases:

function supporting FASTCALL, like a builtin function
function not supporting FASTCALL

In general, I'm in favor of removing such hack, since playing with reference count can lead to complex and annoying bugs. But I would prefer to first see the cost on performances.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

# operator.add is FASTCALL C function
$ ./python -m perf timeit --compare-to $HOME/work/python/cpython/python -s  'L = [0]*10000' -s 'import functools, operator' -- 'functools.reduce(operator.add, L)'
/home/inada-n/work/python/cpython/python: ..................... 498 us +- 3 us
/home/inada-n/work/python/fix-call-usage/python: ..................... 412 us +- 8 us

Median +- std dev: [/home/inada-n/work/python/cpython/python] 498 us +- 3 us -> [/home/inada-n/work/python/fix-call-usage/python] 412 us +- 8 us: 1.21x faster (-17%)

# lambda is Python function (supports FASTCALL)
$ ./python -m perf timeit --compare-to $HOME/work/python/cpython/python -s  'L = [0]*10000' -s 'import functools' -- 'functools.reduce(lambda x,y: x, L)'
/home/inada-n/work/python/cpython/python: ..................... 831 us +- 7 us
/home/inada-n/work/python/fix-call-usage/python: ..................... 788 us +- 11 us

Median +- std dev: [/home/inada-n/work/python/cpython/python] 831 us +- 7 us -> [/home/inada-n/work/python/fix-call-usage/python] 788 us +- 11 us: 1.06x faster (-5%)

# builtin min and max doesn't support FASTCALL
$ ./python -m perf timeit --compare-to $HOME/work/python/cpython/python -s  'L = [0]*10000' -s 'import functools' -- 'functools.reduce(min, L)'
/home/inada-n/work/python/cpython/python: ..................... 1.23 ms +- 0.00 ms
/home/inada-n/work/python/fix-call-usage/python: ..................... 1.42 ms +- 0.04 ms

Median +- std dev: [/home/inada-n/work/python/cpython/python] 1.23 ms +- 0.00 ms -> [/home/inada-n/work/python/fix-call-usage/python] 1.42 ms +- 0.04 ms: 1.16x slower (+16%)

vstinner changed the title~~Fix some inefficient call API usage~~ bpo-29548: Fix some inefficient call API usage

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I missed the "arg" variable.

@@ -2667,10 +2667,7 @@ FileHandler(ClientData clientData, int mask)
func = data->func;
file = data->file;

arg = Py_BuildValue("(Oi)", file, (long) mask);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arg variable is no more used and can be removed, no?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I missed it.
Additionally, argument for "i" format should be int, not (long).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, argument for "i" format should be int, not (long).

Right, I spotted it as well, but then I forgot to report it, sorry :-) Hopefully you fixed it!

akruis pushed a commit to akruis/cpython that referenced this pull request

This change removes the "static" declaration from the function slp_switch(). This forces the compiler to adhere to the ABI specification.

On i386 and amd64 it is now save to configure stackless with --enable-stacklessfewerregisters, except if you compile for darwin. The flag disables explicit saving of %ebp/%rbp.

On Unix-like systems the source file slp_transfer.c now gets compiled with the additional flag -fno-inline-functions instead of -O2.

https://bitbucket.org/stackless-dev/stackless/issues/98 (grafted from 4f7698499ad53e5fad61fb415fbfe2c036672a9c)

akruis pushed a commit to akruis/cpython that referenced this pull request

Mr3x33 added a commit to Mr3x33/cpython that referenced this pull request

colesbury referenced this pull request in colesbury/nogil

This replaces the array of qbsr threads with a stack, which allows us to get rid of the stop-the-world call. There was an initialization order bug: registering a new qsbr thread might stop-the-world, but stopping the world requires the thread to already be attached!

This will probably make the qsbr scan even slower, but we can improve that in the future.

Fixes #97

This was referenced

Feb 11, 2025

ambv pushed a commit to ambv/cpython that referenced this pull request