Recommend PyObject_Call* APIs over PyEval_Call*() APIs

[methane]

BPO	29548
Nosy	@malemburg, @rhettinger, @mdickinson, @ncoghlan, @vstinner, @methane, @serhiy-storchaka, @jdemeyer
PRs	bpo-29548: Recommend PyObject_Call APIs over PyEval_Call APIs. #75 bpo-29684: Fix regression of PyEval_CallObjectWithKeywords #87 bpo-29548: Fix some inefficient call API usage #97 bpo-29548: no longer use PyEval_Call* functions #14683 bpo-29548: deprecate PyEval_Call* functions #14804

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2017-03-14.09:01:29.804>
created_at = <Date 2017-02-13.17:36:52.181>
labels = ['interpreter-core', '3.7']
title = 'Recommend PyObject_Call* APIs over PyEval_Call*() APIs'
updated_at = <Date 2019-07-24.12:03:11.163>
user = 'https://github.com/methane'

bugs.python.org fields:

activity = <Date 2019-07-24.12:03:11.163>
actor = 'methane'
assignee = 'none'
closed = True
closed_date = <Date 2017-03-14.09:01:29.804>
closer = 'methane'
components = ['Interpreter Core']
creation = <Date 2017-02-13.17:36:52.181>
creator = 'methane'
dependencies = []
files = []
hgrepos = []
issue_num = 29548
keywords = []
message_count = 29.0
messages = ['287714', '287717', '287732', '287734', '287742', '287743', '287744', '287752', '287758', '287759', '287760', '287763', '287766', '287767', '287768', '287769', '287775', '287777', '287829', '287833', '287834', '287837', '287865', '287907', '290186', '347664', '347689', '347690', '348382']
nosy_count = 8.0
nosy_names = ['lemburg', 'rhettinger', 'mark.dickinson', 'ncoghlan', 'vstinner', 'methane', 'serhiy.storchaka', 'jdemeyer']
pr_nums = ['75', '87', '97', '14683', '14804']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue29548'
versions = ['Python 3.7']

[methane]

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.

[malemburg]

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.

[methane]

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.

155ea65

It calls _PyObject_FastCallDict() when args is NULL. If kwargs is not dict, it can crash instead of raising TypeError.

[methane]

PR 87 fixes the regression in 3.6 branch

[mdickinson]

See also bpo-11165

[serhiy-storchaka]

See also bpo-485165, bpo-8276 and bpo-11173.

[malemburg]

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.

[methane]

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?

[malemburg]

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.

[vstinner]

"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?

[vstinner]

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.

[methane]

I'm more in favor of modifying PyEval_xxx() to call PyObject_xxx() and deprecate them.

That's PR 75 :)

[malemburg]

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.

[serhiy-storchaka]

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).

[serhiy-storchaka]

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.

[methane]

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.

[vstinner]

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.

[methane]

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.

[rhettinger]

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).

[methane]

I've stopped to deprecating. changed issue title.

[serhiy-storchaka]

I think first at all PyEval_Call* functions should be documented (bpo-11165). 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.

[vstinner]

"I think first at all PyEval_Call* functions should be documented
(bpo-11165). The documentation should recommend to use corresponding
PyObject_Call* functions (...)"

I *now* agree :-)

[vstinner]

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 bpo-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)?

[methane]

New changeset 72dccde by GitHub in branch 'master':
bpo-29548: Fix some inefficient call API usage (GH-97)
72dccde

[methane]

New changeset aa289a5 by INADA Naoki in branch 'master':
bpo-29548: Recommend PyObject_Call APIs over PyEval_Call APIs. (GH-75)
aa289a5

[jdemeyer]

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.

[methane]

FYI, PyEval_CallFunction and PyEval_CallMethod doesn't respect Py_SSIZE_T_CLEAN.
So runtime warning is raised when they are used with "#" format.

[methane]

New changeset 1dbd084 by Inada Naoki (Jeroen Demeyer) in branch 'master':
bpo-29548: no longer use PyEval_Call* functions (GH-14683)
1dbd084

[methane]

New changeset 151b91d by Inada Naoki (Jeroen Demeyer) in branch 'master':
bpo-29548: deprecate PyEval_Call* functions (GH-14804)
151b91d

[Mekk]

I just spent 2 hours trying to find what should I replace PyEval_CallObjectWithKeywords (used in some obscure maintained code) with, then debugging coredumps which started to appear once I replaced it with PyObject_Call (which doesn't handle NULL as args).

Please, next time you decide to deprecate API, include writing detailed instruction of what to replace it with.

PS My code before change:

PyObject* ret = PyEval_CallObjectWithKeywords(func, arg, kw);

my code after change:

if (arg != NULL && ! PyTuple_Check(arg)) {
    PyErr_SetString(PyExc_TypeError,
                    "argument list must be a tuple");
    return NULL;
}
if (kw != NULL && !PyDict_Check(kw)) {
    PyErr_SetString(PyExc_TypeError,
                    "keyword list must be a dictionary");
    return NULL;
}
PyObject* ret = (arg == NULL)
    ? _PyObject_FastCallDict(func, NULL, 0, kw)
    : PyObject_Call(func, arg, kw);

I am by far unconvinced that this is an improvement.

[vstinner]

I proposed PR #105108 to remove these functions in Python 3.13.

@Mekk:

Please, next time you decide to deprecate API, include writing detailed instruction of what to replace it with.

I'm sorry about that and you're right. I took your remark in account in my PR #105108 which removes these functions. I proposed detailed instructions on how to replace removed functions. Your recipe uses private functions which must be avoided (they can disappear or change anytime). I proposed recipes only with public functions.