bpo-29548: Fix some inefficient call API usage #97
Conversation
This file contains 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
vstinner
requested changes
Feb 14, 2017
Modules/_functoolsmodule.c
Outdated
| PyTuple_SetItem(args, 1, op2); | ||
| if ((result = PyEval_CallObject(func, args)) == NULL) | ||
| PyObject *args[] = {result, op2}; | ||
| if ((result = _PyObject_FastCall(func, args, 2)) == NULL) { |
There was a problem hiding this comment.
Would you mind to move the assignment out of the if() please? "result = ...; if (result == NULL) ...".
Modules/_functoolsmodule.c
Outdated
| for (;;) { | ||
| PyObject *op2; | ||
|
|
||
| if (args->ob_refcnt > 1) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
# 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
vstinner
requested changes
Feb 15, 2017
| @@ -2667,10 +2667,7 @@ FileHandler(ClientData clientData, int mask) | |||
| func = data->func; | |||
| file = data->file; | |||
|
|
|||
| arg = Py_BuildValue("(Oi)", file, (long) mask); | |||
There was a problem hiding this comment.
arg variable is no more used and can be removed, no?
There was a problem hiding this comment.
Thank you. I missed it.
Additionally, argument for "i" format should be int, not (long).
There was a problem hiding this comment.
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!
vstinner
approved these changes
Feb 16, 2017
akruis
pushed a commit
to akruis/cpython
that referenced
this pull request
Sep 9, 2017
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
Sep 9, 2017
Mr3x33
added a commit
to Mr3x33/cpython
that referenced
this pull request
Sep 17, 2021
colesbury
referenced
this pull request
in colesbury/nogil
Oct 6, 2021
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While bpo-29548, some inefficient
PyEval_Call*()usage are found.This is spin off pull request for fixing them.
_PyObject_CallNoArg()when there are no arguments.PyObject_CallMethodinstead of manually callingPy_BuildValue(). (temporary tuple may be skipped)