bpo-39245: Make Vectorcall public #17893

encukou · 2020-01-07T12:45:08Z

As per PEP-590, in Python 3.9 the Vectorcall API will be public, i.e. without leading underscores.
This PR also includes some other new call API that got added together with vectorcall.

@vstinner, note that this partially reverts commit 2ff58a2 : IMO PyObject_CallNoArgs should always stay behind #ifndef Py_LIMITED_API (i.e. be in Include/cpython/).

https://bugs.python.org/issue39245

Change made automatically with: for name in \ PyObject_Vectorcall \ Py_TPFLAGS_HAVE_VECTORCALL \ PyObject_VectorcallMethod \ PyObject_FastCallDict \ PyVectorcall_Function \ PyObject_CallOneArg \ PyObject_CallMethodNoArgs \ PyObject_CallMethodOneArg \ ; do echo $name git grep -lwz _$name | xargs -0 sed -i "s/\b_$name\b/$name/g" done

This partially reverts commit 2ff58a2 which added PyObject_CallNoArgs to the 3.9+ stable ABI. This should not be done; there are enough other call APIs in the stable ABI to choose from.

Mark all newly public functions as added in 3.9. Add a note about the 3.8 provisional names. Add notes on public API.

vstinner · 2020-01-07T17:50:14Z

@vstinner, note that this partially reverts commit 2ff58a2 : IMO PyObject_CallNoArgs should always stay behind #ifndef Py_LIMITED_API (i.e. be in Include/cpython/).

The problem is that Include/cpython/ doesn't work with checks on the Python limited API version:

#if !defined(Py_LIMITED_API) || Py_LIMITED_API+0 >= 0x03090000
/* Call a callable Python object without any arguments */
PyAPI_FUNC(PyObject *) PyObject_CallNoArgs(PyObject *func);
#endif

Include/cpython/abstract.h is not included if Py_LIMITED_API is defined!

vstinner · 2020-01-07T17:54:11Z

I'm not sure that I understood your comment properly. It was decided to add PyObject_CallNoArgs() to the limited API on purpose (commit 2ff58a2, bpo-37194). Do you see a reason to exclude it from the limited API?

encukou · 2020-01-07T19:03:39Z

Hmm, I see I misunderstood the bpo-37194 conversation.
The way I read it, there are 3 layers of the C apis: limited (Py_LIMITED_API), public (cpython/) and private (internal/).
I don't think adding something to the public API necessarily means adding it to the stable ABI, especially if there are other ways of doing the same thing.

I see now that you were talking about the limited API (stable ABI) when you said "public".
Do you think PyObject_CallNoArgs should be in the stable ABI? I don't.

ncoghlan · 2020-01-07T20:38:39Z

The main reasons for excluding things from the stable API/ABI is if we think they might be difficult for other implementations to provide, or if they're intrinsically exposed to ABI compatibility issues.

PyObject_CallNoArgs() doesn't suffer from either of those problems - worst case is that other implementations just need to define it as a naive wrapper around PyObject_Call()

vstinner · 2020-01-07T21:03:50Z

Do you think PyObject_CallNoArgs should be in the stable ABI?

Yes, I consider that it is safe to add it to the stable ABI, since it should be trivial to implement.

vstinner

Before making the API public, I would like to see a public discussion if we should pass tstate to functions or not (change the calling convention). For subinterpreters, we will need more and more to access tstate. There are ways to get the tstate, but they may be "inefficient" when calling very frequently.

@ericsnowcurrently: What do you think?

bedevere-bot · 2020-01-08T12:47:40Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

encukou · 2020-01-08T12:56:04Z

stable API/ABI

Got it! Thanks for explaining and sorry for the misunderstanding.

I would like to see a public discussion if we should pass tstate to functions or not (change the calling convention).

I think that's a good change, but I don't think I'll have time myself to do that for 3.9.

The vectorcall API should be added in 3.9, but it can still be changed before the final release. (If it is, I'd prefer the underscored versions to stay as they are for 3.8 compatibility.)

This PR touches a lot of files; if it's open for a long time it might need to be rebased often. Can we not block this on the tstate discussion?

vstinner · 2020-01-08T15:48:38Z

I think that's a good change, but I don't think I'll have time myself to do that for 3.9.

I started a thread on python-dev: https://mail.python.org/archives/list/python-dev@python.org/thread/PIXJAJPWKDGHSQD65VOO2B7FDLU2QLHH/

If we agree that it's a good idea to pass tstate a part of VECTORCALL, I can work on the actual implementation.

encukou · 2020-01-09T12:13:12Z

@vstinner

Before making the API public, I would like to see a public discussion if we should pass tstate to functions or not (change the calling convention).

Why must this be done before this PR is merged?
The discussion is not about making the API public. And this PR doesn't need to be final – if the discussion concludes we need changes to the API, that can still be done.

vstinner · 2020-01-09T13:50:21Z

Why must this be done before this PR is merged?

There are two open questions:

Should VECTORCALL calling convention be modified to add a tstate parameter?
Should the public VECTORCALL C API be modified to add a tstate parameter?

Early answers on python-dev are that the calling convention should be modified, but the public C API should not include tstate (passed implicitly if the calling convention is changed). We can use a private C API which accepts a tstate.

So far, these questions don't seem to block this PR. Changing the calling convention can be done later.

For the private C API with tstate parameter, I added for example _PyObject_VectorcallTstate() function. I think that we can keep this name for now. We cannot use _PyObject_Vectorcall() name, since it is kept as an alias to PyObject_Vectorcall() to backward compatibility.

encukou · 2020-01-10T09:43:31Z

So far, these questions don't seem to block this PR.

That's my impression as well.
I have made the requested changes; please review again
(i.e. I made all of the zero requested changes.)

For the private C API with tstate parameter, I added for example _PyObject_VectorcallTstate() function. I think that we can keep this name for now. We cannot use _PyObject_Vectorcall() name, since it is kept as an alias to PyObject_Vectorcall() to backward compatibility.

If the calling convention passes tstate explicitly, it would make sense to make PyObject_Vectorcall* take tstate as well. (But, for consistency, the new PyObject_Call* ones should not take tstate.)

bedevere-bot · 2020-01-10T09:43:34Z

Thanks for making the requested changes!

@vstinner: please review the changes made to this pull request.

vstinner

This PR is very long: it renames functions, adds backward compatibility aliases, and update usage of the renamed functions, all in the same PR. Would it be possible to revert "update usage of the renamed functions" to have a way shorter PR, easier to review, to focus on the API?

vstinner · 2020-01-28T14:11:39Z

Doc/c-api/call.rst

 +------------------------------------------+------------------+--------------------+---------------+
-| :c:func:`_PyObject_FastCallDict`         | ``PyObject *``   | vectorcall         | dict/``NULL`` |
+| :c:func:`PyObject_FastCallDict`          | ``PyObject *``   | vectorcall         | dict/``NULL`` |


It's a little bit confusing to have "vectorcall" and "fastcall" names, especially since "PyObject_FastCallDict" is documented as "vectorcall". What about calling it PyObject_VectorcallDict()?

Doc/c-api/call.rst

vstinner · 2020-01-28T14:18:24Z

Include/cpython/abstract.h

+#define _PyVectorcall_Function PyVectorcall_Function
+#define _PyObject_CallOneArg PyObject_CallOneArg
+#define _PyObject_CallMethodNoArgs PyObject_CallMethodNoArgs
+#define _PyObject_CallMethodOneArg PyObject_CallMethodOneArg


Sadly, Py_DEPRECATED() cannot be used on these aliases, but I think that it's fine to remove them at once in a future Python release like 3.10 or later. We don't provide any backward compatibility on private functions.

Another option is to not provide this backward compatibility layer and force users to upgrade. Do you know which projects are impacted if these symbols are removed? Does it impact all projects which ship C code generated by Cython?

What about _Py_TPFLAGS_HAVE_VECTORCALL?

Another option is to keep the aliases indefinitely. Would that be a problem?
But you're right, they aren't supported API and we can just remove them in 3.10.
I don't know what projects used them. They were documented for 3.8 (with a warning).

The alias for Py_TPFLAGS_HAVE_VECTORCALL is near its definition.

Another option is to keep the aliases indefinitely. Would that be a problem?

Usually, I prefer to remove legacy stuff to reduce the maintenance burden. But these aliases are not an issue in the short term.

vstinner · 2020-01-28T14:22:16Z

Misc/NEWS.d/3.9.0a1.rst

@@ -678,7 +678,7 @@ The :keyword:`assert` statement now works properly if the

 Removed object cache (``free_list``) for bound method objects.  Temporary
 bound method objects are less used than before thanks to the ``LOAD_METHOD``
-opcode and the ``_PyObject_VectorcallMethod`` C API.
+opcode and the ``PyObject_VectorcallMethod`` C API.


I don't think that you should modify 3.9.0a1 changelog. I would prefer to only document that functions are renamed in a more recent Changelog (NEWS) entry.

Indeed, this was a rebase mistake. Thanks for catching it!

vstinner · 2020-01-28T14:22:36Z

Misc/NEWS.d/next/C API/2020-01-07-13-46-40.bpo-39245.G7wog6.rst

@@ -0,0 +1 @@
+The Vectorcall API (PEP 590) was made public.


Would you mind to list renamed renamed symbols/macros here? Or is it possible to at least add a link from here to the Doc/c-api/call.rst documentation where the change is documented?

bedevere-bot · 2020-01-28T14:25:34Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

vstinner · 2020-01-28T14:26:50Z

cc @scoder @serhiy-storchaka @pablogsal @markshannon @antocuni: Would you mind to have a look? It's an important part of the C API.

encukou · 2020-01-30T12:27:31Z

Would it be possible to revert "update usage of the renamed functions" to have a way shorter PR, easier to review, to focus on the API?

OK, at least we'll know the backcompat aliases work!

vstinner

LGTM. I just requested to update two comments.

Thank you, it is waaaay easier to review a PR modifying 5 files, rather than a PR modying 61 files :-)

I'm not sure if we should provide aliases for backward compatibility. The point of a private API which was also provisional is that we are free to change it whenever we want. If we still support old names, projects may continue to use the old names forever. But this can be discussed afterwards.

vstinner · 2020-01-30T14:58:28Z

Include/cpython/abstract.h

@@ -63,7 +63,7 @@ PyVectorcall_NARGS(size_t n)
 }



Would you mind to remove the underscore prefix from _PyObject_Vectorcall() and _PyObject_FastCallDict() in _PyObject_MakeTpCall() and _PY_FASTCALL_SMALL_STACK comments?

Sure. I thought I'd do that as part of changing all the call sites, but it can be done now.

vstinner · 2020-01-30T15:02:31Z

Include/cpython/abstract.h

+#define _PyVectorcall_Function PyVectorcall_Function
+#define _PyObject_CallOneArg PyObject_CallOneArg
+#define _PyObject_CallMethodNoArgs PyObject_CallMethodNoArgs
+#define _PyObject_CallMethodOneArg PyObject_CallMethodOneArg


Another option is to keep the aliases indefinitely. Would that be a problem?

Usually, I prefer to remove legacy stuff to reduce the maintenance burden. But these aliases are not an issue in the short term.

vstinner · 2020-02-06T14:49:07Z

I merged your PR @encukou, thanks! Now you may want to redo your PR to use the newer function names (remove "_") prefix. By the way, I removed the following section from the commit message (since you reverted it in the meanwhile):

* Remove leading underscores from provisional call/Vectorcall API

Change made automatically with:

    for name in \
        PyObject_Vectorcall \
        Py_TPFLAGS_HAVE_VECTORCALL \
        PyObject_VectorcallMethod \
        PyObject_FastCallDict \
        PyVectorcall_Function \
        PyObject_CallOneArg \
        PyObject_CallMethodNoArgs \
        PyObject_CallMethodOneArg \
    ;
    do
        echo $name
        git grep -lwz _$name | xargs -0 sed -i "s/\b_$name\b/$name/g"
    done

encukou added 4 commits January 7, 2020 13:15

Fix up formatting in docs

116d0d0

Add backcompat defines and move non-limited API declaration to cpython/

f0c005f

This partially reverts commit 2ff58a2 which added PyObject_CallNoArgs to the 3.9+ stable ABI. This should not be done; there are enough other call APIs in the stable ABI to choose from.

Adjust documentation

d948983

Mark all newly public functions as added in 3.9. Add a note about the 3.8 provisional names. Add notes on public API.

encukou requested review from 1st1, asvetlov, berkerpeksag, brettcannon, ericsnowcurrently, gpshead, methane, ncoghlan, pablogsal, rhettinger and warsaw as code owners January 7, 2020 12:45

the-knights-who-say-ni added the CLA signed label Jan 7, 2020

bedevere-bot added the awaiting core review label Jan 7, 2020

Add a NEWS entry

5e340bf

Put PyObject_CallNoArgs back in the limited API

60a8833

vstinner requested changes Jan 8, 2020

View reviewed changes

bedevere-bot removed the awaiting core review label Jan 8, 2020

bedevere-bot added the awaiting changes label Jan 8, 2020

brettcannon removed their request for review January 8, 2020 22:06

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 10, 2020

bedevere-bot requested a review from vstinner January 10, 2020 09:43

vstinner requested changes Jan 28, 2020

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting change review labels Jan 28, F438 2020

encukou added 3 commits January 30, 2020 13:15

Revert "update usage of the renamed functions"

29a8331

Rename PyObject_FastCallDict to PyObject_VectorcallDict

1f7c746

Expand the NEWS entry

3fc5442

vstinner approved these changes Jan 30, 2020

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Jan 30, 2020

Remove underscores from comments

9fff79b

vstinner merged commit 3f563ce into python:master Feb 6, 2020

bedevere-bot removed the awaiting merge label Feb 6, 2020

encukou mentioned this pull request Feb 11, 2020

bpo-39245: Switch to public API for Vectorcall #18460

Merged

Uh oh!

bpo-39245: Make Vectorcall public #17893

bpo-39245: Make Vectorcall public #17893

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!