msg287714 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-02-13 17:36 |
As reading call.c, PyEval_Call* APIs are mostly duplicate of PyObject_Call* APIs.
While they are not documented, they seems part of limited APIs.
So we can't remove them easily. Let's deprecate them for now.
|
msg287717 - (view) |
Author: Marc-Andre Lemburg (lemburg) * |
Date: 2017-02-13 18:10 |
Please note that the two sets of APIs are not identical, e.g. you cannot simply replace PyEval_CallObject() with PyObject_Call(), since the former applies a few extra checks and defaults, which the latter doesn't.
|
msg287732 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-02-14 03:00 |
Thanks, Lemburg to pointing it out.
Here is detail of the difference.
## PyEval_CallFunction(), PyEval_CallMethod()
They are very similar to PyObject_CallFunction() and PyObject_CallMethod(). difference are:
* PyEval_Call...() doesn't respect Py_SSIZE_T_CLEAN
* PyObject_Call... has following special case. PyEval_CallFunction(callable, "i", (int)i) will raise TypeError("keyword list must be a tuple") and PyObject_CallFunction(callable, "i", (int)i) calls `callable(i)`
if (nargs == 1 && PyTuple_Check(stack[0])) {
/* Special cases for backward compatibility:
- PyObject_CallFunction(func, "O", tuple) calls func(*tuple)
- PyObject_CallFunction(func, "(OOO)", arg1, arg2, arg3) calls
func(*(arg1, arg2, arg3)): func(arg1, arg2, arg3) */
PyObject *args = stack[0];
result = _PyObject_FastCall(callable,
&PyTuple_GET_ITEM(args, 0),
PyTuple_GET_SIZE(args));
}
PyEval_CallFunction is not called from Python source tree.
PyEval_CallMethod has only one caller in tree and format string is "(Oi)". It can be replaced with PyObject_CallMethod safely.
## PyEval_CallObject(), PyEval_CallObjectWithKeywords()
PyEval_CallObject() is a just macro calling PyEval_CallObjectWithKeywords() (call it COWK later).
PyEval_CallObject() is identical to PyObject_CallObject(). Only difference is it's a macro or function.
COWK is similar to PyObject_Call(), but COWK raise TypeError when args is not a tuple or kwds is not a dictionary and PyObject_Call() uses assert.
There are only two caller of PyEval_CallObjectWithKeywords() other than PyEval_CallObject and PyObject_CallObject.
One is tp_call of weakcallableproxy. Type of kwargs is checked before calling tp_call.
Another is in threading (boot->keyw). The threading module checks it's a dict.
So replacing them to PyObject_CallObject() is safe.
----
While they are complex API, there are few (or no) callers in Python tree.
It's too hard to maintain. Actually, I found regression of COWK in Python 3.6.
https://github.com/python/cpython/commit/155ea65e5c88d250a752ee5321860ef11ede4085
It calls _PyObject_FastCallDict() when args is NULL. If kwargs is not dict, it can crash instead of raising TypeError.
|
msg287734 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-02-14 04:31 |
PR 87 fixes the regression in 3.6 branch
|
msg287742 - (view) |
Author: Mark Dickinson (mark.dickinson) * |
Date: 2017-02-14 08:46 |
See also #11165
|
msg287743 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2017-02-14 09:05 |
See also issue485165, issue8276 and issue11173.
|
msg287744 - (view) |
Author: Marc-Andre Lemburg (lemburg) * |
Date: 2017-02-14 09:06 |
Thanks, but you missed the main difference:
The argument tuple can be NULL for PyEval_CallObject() (it then gets replaced with an empty tuple), but will raise a segfault for PyObject_Call().
Now, apart from looking at the use cases in the core, you also have to check whether you are removing useful functionality when deprecating APIs. Code in third party extensions may not be that easy to adapt to the PyObject_Call*() APIs, since they lack several checks which the PyEval_Call*() APIs apply.
Deprecation of existing published APIs is only an option in case there are other APIs which can reasonably replace them and those other APIs would have to have been around for a while, since otherwise third party code would have to provide wrappers for Python versions which don't supply these APIs or only ones which do not implement the extra functionality.
E.g. if you now add the extra support for args being NULL to PyObject_Call() in Python 3.7, third party code would have to either switch being using PyEval_CallObject() and PyObject_Call() depending on Python version or provide its own wrappers around the two APIs depending on Python version.
Since calling objects is rather common in Python extensions, special care has to be taken.
It would have been better to add the special case for args == NULL and the extra types checks to PyObject_Call() a long long time ago, since it's the only API that allows calling an object with both args and kwargs. Alas, didn't happen, so we have to live with it.
It may actually be better to add a new API PyObject_CallObjectWithKeywords() which works like the PyEval_CallObjectWithKeywords() API and then deprecate the PyEval_COWK() API. Third party code can then use a simple macro to provide a backwards compatibility shim for older Python versions.
|
msg287752 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-02-14 09:49 |
> The argument tuple can be NULL for PyEval_CallObject() (it then gets replaced with an empty tuple), but will raise a segfault for PyObject_Call().
PyObject_CallObject() accepts NULL as args. Macro vs function is only difference of them.
On the other hand, PyObject_CallObjectWithKeyword() doesn't have identical function.
So I agree your suggestion to add NULL support to PyObject_Call().
We have more fast _PyObject_FastCall* APIs for performance critical code.
Additional cost to check NULL in PyObject_Call() would be negligible.
Any opinion from others?
|
msg287758 - (view) |
Author: Marc-Andre Lemburg (lemburg) * |
Date: 2017-02-14 10:09 |
Looking through Python's history, it's interesting that PyObject_Call() did apply the args == NULL checks up until Python 2.1.
In Python 2.2 this was replaced by a direct call to tp_call, without the checks. However, the tp_call slots don't do this check as you can see in function_call() of function objects. And indeed, the documentation of PyObject_Call() was changed in that version as well to disallow args == NULL.
|
msg287759 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-02-14 10:13 |
"you now add the extra support for args being NULL to PyObject_Call()"
I dislike this idea. I don't want to change the API of this function.
If it is likely that NULL is the result of a previous error:
args = Py_BuildValue(...);
res = PyObject_Call(func, args, NULL);
There are already enough ways to call a function with no argument:
res = PyObject_CallFunction(func, NULL);
res = PyObject_CallFunctionObjArgs(func, NULL);
res = _PyObject_CallNoArg(func) # private
If you want to call a function only with keyword arguments, well, create an empty tuple ... which is a singleton in CPython, no risk of memory allocation failure ... and use PyObject_Call(), no?
|
msg287760 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-02-14 10:16 |
> Please note that the two sets of APIs are not identical, e.g. you cannot simply replace PyEval_CallObject() with PyObject_Call(), since the former applies a few extra checks and defaults, which the latter doesn't.
IMHO these checks are too expensive at runtime for little benefit. If you pass non-tuple to PyObject_Call(), Python immediately crash. You are immediately noticied of the bug :-) I don't think that such bugs are common enough to justify the overhead.
Any idea of the popularity of the undocumented PyEval_xxx() functions? Are they used by Cython for example? By a single random extension module in the world?
I'm more in favor of modifying PyEval_xxx() to call PyObject_xxx() and deprecate them.
|
msg287763 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-02-14 10:35 |
> I'm more in favor of modifying PyEval_xxx() to call PyObject_xxx() and deprecate them.
That's PR 75 :)
|
msg287766 - (view) |
Author: Marc-Andre Lemburg (lemburg) * |
Date: 2017-02-14 11:00 |
On 14.02.2017 11:16, STINNER Victor wrote:
>
>> Please note that the two sets of APIs are not identical, e.g. you cannot simply replace PyEval_CallObject() with PyObject_Call(), since the former applies a few extra checks and defaults, which the latter doesn't.
>
> IMHO these checks are too expensive at runtime for little benefit. If you pass non-tuple to PyObject_Call(), Python immediately crash. You are immediately noticied of the bug :-) I don't think that such bugs are common enough to justify the overhead.
From the design of the abstract API layer, it is rather
uncommon to have these not do extra checks to prevent segfaults.
They were originally designed to be safe and developer friendly.
OTOH, the PyEval_* APIs were designed to be fast, only for people
who know what they are doing and for interpreter internals.
Historically, this design approach appears to have been swapped
somewhere between Python 2.1 and 2.2 for the call APIs,
which is unfortunate.
So from a design perspective, it would be better to have the
abstract APIs again do proper checks and leave the low level,
"segfault protected" :-) call APIs around as PyEval_Call*()
or better yet: not make them public at all.
> Any idea of the popularity of the undocumented PyEval_xxx() functions? Are they used by Cython for example? By a single random extension module in the world?
Well, I know that our eGenix extensions are using them
and there are quite a few hits on github as well:
https://github.com/search?utf8=%E2%9C%93&q=PyEval_CallObjectWithKeywords&type=Code&ref=searchresults
> I'm more in favor of modifying PyEval_xxx() to call PyObject_xxx() and deprecate them.
|
msg287767 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2017-02-14 11:05 |
PyEval_CallObject() was added at Jan 12 1995 (05360cd616ae). PyObject_CallObject(), PyObject_CallFunction() and PyObject_CallMethod() were added with Include/abstract.h at Jul 18 1995 (d5398acafa2c) and implemented in terms of PyEval_CallObject(). PyEval_CallObjectWithKeywords() was added few minutes later (0261bf5b3819). PyObject_Call() was added at Aug 02 2001 (09df3254b49d) as a simple wrapper around tp_call. PyObject_CallFunction() and PyObject_CallMethod() were reimplemented in terms of PyObject_Call() at Aug 16 2002 (255d1d3e66a3).
|
msg287768 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2017-02-14 11:11 |
> Any idea of the popularity of the undocumented PyEval_xxx() functions?
You can just search code at GitHub.
I would suggest to deprecate PyEval_Call*() functions first only in the documentation. In 3.8 or 3.9 add the Py_DEPRECATED attribute.
|
msg287769 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-02-14 11:37 |
> I would suggest to deprecate PyEval_Call*() functions first only in the documentation. In 3.8 or 3.9 add the Py_DEPRECATED attribute.
<Include/ceval.h> doesn't excluded by PEP 384.
And these 3 functions and 1 macro is outside of "#ifndef Py_LIMITED_API" check.
So they are part of stable ABI. We can't remove them until Python 4.
No reason to hurry up.
I'll remove Py_DEPRECATED for Python 3.7.
|
msg287775 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-02-14 13:39 |
> So they are part of stable ABI. We can't remove them until Python 4.
Please just stop right now using "Python 4" as the starting point to
break -again- the Python world.
If you plan to remove the function, plan it right now with versions
like "in 2 cycles".
If the function is part of the stable ABI, we simply cannot remove it.
Since these functions are used outside CPython and they are part of
the stable ABI, I'm not sure anymore that there is any value to remove
them.
Maybe just document them and write "please don't use them", but don't
deprecate the functions.
|
msg287777 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-02-14 15:22 |
I stopped deprecating PyEval_Call APIs, and removing it's usage in PR 75.
Summary of current pull requests:
PR 87 contains fix regression of PyEval_CallObjectWithKeywords for Python 3.6.
PR 75 is for master branch. It contains fix same to PR 87. Additionally, PyEval_CallFunction and PyEval_CallMethod is now copy of PyObject_CallFunction and PyObject_CallMethod. And comment about PyObject_Call preference is added.
|
msg287829 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2017-02-15 09:43 |
In general, we should have a strong aversion to deprecation except in cases where something is actually broken. API changes make it more difficult for people to migrate to Python 3 or to upgrade between minor releases.
The longer an API has existed, the more pronounced are effects of changing it (invalidating published references, killing weakly maintained projects, and affecting more code that we may not know about).
|
msg287833 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-02-15 09:56 |
I've stopped to deprecating. changed issue title.
|
msg287834 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2017-02-15 10:01 |
I think first at all PyEval_Call* functions should be documented (issue11165). The documentation should recommend to use corresponding PyObject_Call* functions and explicitly describe the difference between PyEval_Call* and PyObject_Call* APIs.
Few releases after deprecating PyEval_Call APIs in documentation we can add the Py_DEPRECATED attribute for emitting compiler warnings. Few releases after deprecating in code we can remove PyEval_Call* declarations from headers, but keep exporting them in binary library. In Python 4 (or other major release that will break binary compatibility) they can be removed at all.
|
msg287837 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-02-15 10:11 |
"I think first at all PyEval_Call* functions should be documented
(issue11165). The documentation should recommend to use corresponding
PyObject_Call* functions (...)"
I *now* agree :-)
|
msg287865 - (view) |
Author: STINNER Victor (vstinner) * |
Date: 2017-02-15 17:27 |
> # builtin min and max doesn't support FASTCALL
> (...): 1.16x slower (+16%)
Hum, the slowdown is not negligible, even if functools.reduce() is rarely used.
Do you think that it would be insane to have two code paths instead?
* New FASTCALL path if func suports FASTCALL calling _PyObject_FastCall()
* Current code path using cached args tuple otherwise
For example, you can use args==NULL marker for the FASTCALL path. Maybe we need a _PyObject_SupportFastCall() function which would return 1 for Python functions and C functions, except of C functions with METH_VARARGS flag set.
My expectation is a speedup for functions supporting FASTCALL, but *no slowdown* for functions not supporting FASTCALL.
--
property_descr_get() also caches args tuple. I started my work on FASTCALL because this optimization caused bugs (but also because I wanted to make Python faster, but that's a different topic ;-)).
In the past, the _pickle module also used cached args tuple, but the cache was removed because it was vulnerable to race conditions.
For this issue, I suggest to leave functools.reduce() unchanged, but open a new issue to discuss what to do with code still using a cached args tuple.
My long term goal with FASTCALL was to remove completely cached tuple used to call functions. I wrote tp_fastcall for that, but tp_fastcall (issue #29259) was rejected. The rejected tp_fastcall blocked my long term plan, we have to find a different approach.
Maybe we should add support for FASTCALL for a little bit more functions, and later simply remove the optimization (hack, cached tuple)?
|
msg287907 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-02-16 00:26 |
New changeset 72dccde884d89586b0cafd990675b7e21720a81f by GitHub in branch 'master':
bpo-29548: Fix some inefficient call API usage (GH-97)
https://github.com/python/cpython/commit/72dccde884d89586b0cafd990675b7e21720a81f
|
msg290186 - (view) |
Author: Inada Naoki (methane) * |
Date: 2017-03-24 22:20 |
New changeset aa289a59ff6398110e1122877c073c9354ee53db by INADA Naoki in branch 'master':
bpo-29548: Recommend PyObject_Call APIs over PyEval_Call APIs. (GH-75)
https://github.com/python/cpython/commit/aa289a59ff6398110e1122877c073c9354ee53db
|
msg347664 - (view) |
Author: Jeroen Demeyer (jdemeyer) * |
Date: 2019-07-11 09:11 |
I understand the arguments for not removing these functions. However, I still think that we should deprecate them but without planning in advance when they should be removed. Victor said that we should document these functions as "please don't use them", and that is exactly what a deprecation message accomplishes.
Most other projects that I know have only a minimum deprecation period (i.e. feature X can only be removed if it was deprecated for Y time). I don't understand why CPython insists that the minimum of 2 releases should also be a maximum (i.e. feature X MUST be removed after it was deprecated for Y time). I don't see the problem with long-term deprecations for stuff that we don't plan to remove soon.
|
msg347689 - (view) |
Author: Inada Naoki (methane) * |
Date: 2019-07-11 15:48 |
FYI, PyEval_CallFunction and PyEval_CallMethod doesn't respect Py_SSIZE_T_CLEAN.
So runtime warning is raised when they are used with "#" format.
|
msg347690 - (view) |
Author: Inada Naoki (methane) * |
Date: 2019-07-11 15:58 |
New changeset 1dbd084f1f68d7293718b663df675cfbd0c65712 by Inada Naoki (Jeroen Demeyer) in branch 'master':
bpo-29548: no longer use PyEval_Call* functions (GH-14683)
https://github.com/python/cpython/commit/1dbd084f1f68d7293718b663df675cfbd0c65712
|
msg348382 - (view) |
Author: Inada Naoki (methane) * |
Date: 2019-07-24 12:03 |
New changeset 151b91dfd21a100ecb1eba9e293c0a8695bf3bf5 by Inada Naoki (Jeroen Demeyer) in branch 'master':
bpo-29548: deprecate PyEval_Call* functions (GH-14804)
https://github.com/python/cpython/commit/151b91dfd21a100ecb1eba9e293c0a8695bf3bf5
|
|
Date |
User |
Action |
Args |
2022-04-11 14:58:43 | admin | set | github: 73734 |
2019-07-24 12:03:11 | methane | set | messages:
+ msg348382 |
2019-07-17 08:00:28 | jdemeyer | set | pull_requests:
+ pull_request14600 |
2019-07-11 15:58:01 | methane | set | messages:
+ msg347690 |
2019-07-11 15:48:42 | methane | set | messages:
+ msg347689 |
2019-07-11 09:11:37 | jdemeyer | set | nosy:
+ jdemeyer messages:
+ msg347664
|
2019-07-10 11:06:08 | jdemeyer | set | pull_requests:
+ pull_request14488 |
2017-03-24 22:20:08 | methane | set | messages:
+ msg290186 |
2017-03-14 09:01:29 | methane | set | status: open -> closed dependencies:
- Document PyEval_Call* functions resolution: fixed stage: resolved |
2017-02-18 05:27:33 | ezio.melotti | set | messages:
- msg288059 |
2017-02-18 05:13:18 | ncoghlan | set | nosy:
+ ncoghlan messages:
+ msg288059
|
2017-02-16 00:26:03 | methane | set | messages:
+ msg287907 |
2017-02-15 17:27:01 | vstinner | set | messages:
+ msg287865 |
2017-02-15 16:04:01 | methane | set | pull_requests:
+ pull_request82 |
2017-02-15 10:11:24 | vstinner | set | messages:
+ msg287837 title: Recommend PyObject_Call* APIs over PyEval_Call*() APIs -> Recommend PyObject_Call* APIs over PyEval_Call*() APIs |
2017-02-15 10:01:06 | serhiy.storchaka | set | dependencies:
+ Document PyEval_Call* functions messages:
+ msg287834 |
2017-02-15 09:56:51 | methane | set | messages:
+ msg287833 title: deprecate PyEval_Call*() functions. -> Recommend PyObject_Call* APIs over PyEval_Call*() APIs |
2017-02-15 09:43:27 | rhettinger | set | nosy:
+ rhettinger messages:
+ msg287829
|
2017-02-14 15:22:50 | methane | set | messages:
+ msg287777 |
2017-02-14 13:39:12 | vstinner | set | messages:
+ msg287775 |
2017-02-14 11:37:38 | methane | set | messages:
+ msg287769 |
2017-02-14 11:11:13 | serhiy.storchaka | set | messages:
+ msg287768 |
2017-02-14 11:05:08 | serhiy.storchaka | set | messages:
+ msg287767 |
2017-02-14 11:00:44 | lemburg | set | messages:
+ msg287766 |
2017-02-14 10:35:32 | methane | set | messages:
+ msg287763 |
2017-02-14 10:16:29 | vstinner | set | messages:
+ msg287760 |
2017-02-14 10:13:06 | vstinner | set | messages:
+ msg287759 |
2017-02-14 10:09:53 | lemburg | set | messages:
+ msg287758 |
2017-02-14 09:49:03 | methane | set | messages:
+ msg287752 |
2017-02-14 09:06:53 | lemburg | set | messages:
+ msg287744 |
2017-02-14 09:05:58 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka messages:
+ msg287743
|
2017-02-14 08:46:40 | mark.dickinson | set | nosy:
+ mark.dickinson messages:
+ msg287742
|
2017-02-14 04:31:56 | methane | set | messages:
+ msg287734 pull_requests:
+ pull_request55 |
2017-02-14 03:07:41 | methane | set | nosy:
+ vstinner
|
2017-02-14 03:00:32 | methane | set | messages:
+ msg287732 |
2017-02-13 18:10:11 | lemburg | set | nosy:
+ lemburg messages:
+ msg287717
|
2017-02-13 17:38:21 | methane | set | pull_requests:
+ pull_request54 |
2017-02-13 17:36:52 | methane | create | |