Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
bpo-29548: Fix some inefficient call API usage#97
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
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) ...".
Modules/_functoolsmodule.c Outdated
| for (;;) { | ||
| PyObject *op2; | ||
| if (args->ob_refcnt > 1) { |
There was a problem hiding this comment.
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 acall() 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.
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 usMedian +- 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 usMedian +- 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 msMedian +- 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%)There was a problem hiding this comment.
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); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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!
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 flagdisables explicit saving of %ebp/%rbp.On Unix-like systems the source file slp_transfer.c now gets compiledwith the additional flag -fno-inline-functions instead of -O2.https://bitbucket.org/stackless-dev/stackless/issues/98(grafted from 4f7698499ad53e5fad61fb415fbfe2c036672a9c)
This replaces the array of qbsr threads with a stack, whichallows us to get rid of the stop-the-world call. There wasan initialization order bug: registering a new qsbr threadmight stop-the-world, but stopping the world requires thethread to already be attached!This will probably make the qsbr scan even slower, but wecan improve that in the future.Fixes#97
Uh oh!
There was an error while loading.Please reload this page.
Whilebpo-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)