classification
Title: Recommend PyObject_Call* APIs over PyEval_Call*() APIs
Type: Stage: resolved
Components: Interpreter Core Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: haypo, inada.naoki, lemburg, mark.dickinson, ncoghlan, rhettinger, serhiy.storchaka
Priority: normal Keywords:

Created on 2017-02-13 17:36 by inada.naoki, last changed 2017-03-14 09:01 by inada.naoki. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 75 merged inada.naoki, 2017-02-13 17:38
PR 87 open inada.naoki, 2017-02-14 04:31
PR 97 merged inada.naoki, 2017-02-15 16:04
Messages (24)
msg287714 - (view) Author: INADA Naoki (inada.naoki) * (Python committer) 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) * (Python committer) 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 (inada.naoki) * (Python committer) 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 (inada.naoki) * (Python committer) Date: 2017-02-14 04:31
PR 87 fixes the regression in 3.6 branch
msg287742 - (view) Author: Mark Dickinson (mark.dickinson) * (Python committer) Date: 2017-02-14 08:46
See also #11165
msg287743 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-02-14 09:05
See also issue485165, issue8276 and issue11173.
msg287744 - (view) Author: Marc-Andre Lemburg (lemburg) * (Python committer) 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 (inada.naoki) * (Python committer) 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) * (Python committer) 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 (haypo) * (Python committer) 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 (haypo) * (Python committer) 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 (inada.naoki) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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 (inada.naoki) * (Python committer) 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 (haypo) * (Python committer) 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 (inada.naoki) * (Python committer) 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) * (Python committer) 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 (inada.naoki) * (Python committer) Date: 2017-02-15 09:56
I've stopped to deprecating.  changed issue title.
msg287834 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) 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 (haypo) * (Python committer) 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 (haypo) * (Python committer) 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 (inada.naoki) * (Python committer) 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
History
Date User Action Args
2017-03-14 09:01:29inada.naokisetstatus: open -> closed
dependencies: - Document PyEval_Call* functions
resolution: fixed
stage: resolved
2017-02-18 05:27:33ezio.melottisetmessages: - msg288059
2017-02-18 05:13:18ncoghlansetnosy: + ncoghlan
messages: + msg288059
2017-02-16 00:26:03inada.naokisetmessages: + msg287907
2017-02-15 17:27:01hayposetmessages: + msg287865
2017-02-15 16:04:01inada.naokisetpull_requests: + pull_request82
2017-02-15 10:11:24hayposetmessages: + msg287837
title: Recommend PyObject_Call* APIs over PyEval_Call*() APIs -> Recommend PyObject_Call* APIs over PyEval_Call*() APIs
2017-02-15 10:01:06serhiy.storchakasetdependencies: + Document PyEval_Call* functions
messages: + msg287834
2017-02-15 09:56:51inada.naokisetmessages: + msg287833
title: deprecate PyEval_Call*() functions. -> Recommend PyObject_Call* APIs over PyEval_Call*() APIs
2017-02-15 09:43:27rhettingersetnosy: + rhettinger
messages: + msg287829
2017-02-14 15:22:50inada.naokisetmessages: + msg287777
2017-02-14 13:39:12hayposetmessages: + msg287775
2017-02-14 11:37:38inada.naokisetmessages: + msg287769
2017-02-14 11:11:13serhiy.storchakasetmessages: + msg287768
2017-02-14 11:05:08serhiy.storchakasetmessages: + msg287767
2017-02-14 11:00:44lemburgsetmessages: + msg287766
2017-02-14 10:35:32inada.naokisetmessages: + msg287763
2017-02-14 10:16:29hayposetmessages: + msg287760
2017-02-14 10:13:06hayposetmessages: + msg287759
2017-02-14 10:09:53lemburgsetmessages: + msg287758
2017-02-14 09:49:03inada.naokisetmessages: + msg287752
2017-02-14 09:06:53lemburgsetmessages: + msg287744
2017-02-14 09:05:58serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg287743
2017-02-14 08:46:40mark.dickinsonsetnosy: + mark.dickinson
messages: + msg287742
2017-02-14 04:31:56inada.naokisetmessages: + msg287734
pull_requests: + pull_request55
2017-02-14 03:07:41inada.naokisetnosy: + haypo
2017-02-14 03:00:32inada.naokisetmessages: + msg287732
2017-02-13 18:10:11lemburgsetnosy: + lemburg
messages: + msg287717
2017-02-13 17:38:21inada.naokisetpull_requests: + pull_request54
2017-02-13 17:36:52inada.naokicreate