bpo-37207: Use PEP 590 vectorcall to speed up range(), list() and dict() by about 30% by markshannon · Pull Request #13930 · python/cpython

markshannon · 2019-06-09T09:40:25Z

https://bugs.python.org/issue37207

tirkarthi · 2019-06-09T18:18:33Z

test_range seems to leak references. Using test.bisect_cmd gives me test.test_range.RangeTest.test_invalid_invocation as the relevant test.

➜ cpython git:(pr_13930) ./python.exe -m test -R 3:3 test_range
Run tests sequentially
0:00:00 load avg: 1.75 [1/1] test_range
beginning 6 repetitions
123456
......
test_range leaked [11, 11, 11] references, sum=33
test_range failed
== Tests result: FAILURE ==
1 test failed:
test_range
Total duration: 19 sec 722 ms
Tests result: FAILURE

ZackerySpytz · 2019-06-11T05:46:52Z

I don't think there should be a newline right after the (.

jdemeyer · 2019-06-11T13:17:02Z

CC @vstinner because this PR actually affects the stable ABI for PyTypeObject (unlike #13858 where this was erroneously claimed)

jdemeyer · 2019-06-14T09:23:23Z

This change may or may not make sense, but in any case I think that it needs to be justified individually and a test added, preferably on a separate PR.

The comment in Include/cpython/abstract.h says that _PyStack_UnpackDict does not check the type of keyword arguments. So this change is inconsistent with that. If you think that this should be changed, please open a separate issue explaining the rationale for this change.

The additional validation should have been in PyVectorcall_Call which does need to do validation. I've moved it there.

I still think that it's better to move that change to a different PR with rationale explained for this change.

jdemeyer · 2019-06-14T09:26:27Z

Do you have an idea why this speeds up those calls that much? I'm just wondering if we could instead try to optimize the existing code path instead of implementing a mostly duplicate code path.

jdemeyer · 2019-06-17T12:53:48Z

What are your plans for inheritance of tp_vectorcall? That should be clarified and documented.

jdemeyer

I agree with the overall approach here. However I have various minor comments (some already mentioned):

I'm not convinced that vectorcall should be used for dict(). There was a discussion about using METH_FASTCALL for dict.update() in bpo-29312 and the conclusion was that it was a bad idea. So either we need to argue that the conclusion on bpo-29312 was wrong or that it doesn't apply to the dict() constructor.
You should try to minimize duplication of functionality. For example range_new and range_vectorcall serve essentially the same purpose, so why are there two functions? You could implement both in terms of a common function or implement range_new as

staticPyObject*range_new(PyTypeObject*type, PyObject*args, PyObject*kw)
{
if (kw&&PyDict_GET_SIZE(kw) !=0) {
PyErr_Format(PyExc_TypeError, "range() takes no keyword arguments");
returnNULL;
}
returnrange_vectorcall(type, _PyTuple_ITEMS(args),
PyTuple_GET_SIZE(args), NULL);
}

Use Py_ssize_t for the nargs variable.
The change to PyVectorcall_Call does not belong here. Please open a separate PR for that.
I'm getting a compiler warning

Objects/rangeobject.c:765:26: warning: initialization from incompatible pointer type [-Wincompatible-pointer-types]
.tp_vectorcall = range_vectorcall
^~~~~~~~~~~~~~~~

jdemeyer · 2019-07-04T13:38:17Z

Since dict() and dict.update() are very similar (basically, dict(*args, **kwargs) starts from an empty dict and then calls update(*args, **kwargs) on it), we should handle them together. I suggest to take that change out of this PR.

methane · 2019-07-04T13:44:36Z

dict and dict.update are different.
dict(d1, **d2) is well known idiom to merge two dicts. While there is {**d1, **d2} now, many people are familiar with old idiom.

jdemeyer · 2019-07-04T13:49:56Z

@methane, I don't understand which point you are trying to make.

Right now, there is a common implementation of dict() and dict.update() using VARARGS. I would like to change that to a common implementation of dict() and dict.update() using FASTCALL. Do you agree?

jdemeyer · 2019-07-10T08:16:22Z

I opened https://bugs.python.org/issue37540 for the issue of checking that keyword names are strings.

jdemeyer · 2019-08-05T15:01:45Z

I created #14588 and #14682 to separate two pieces from this PR. May I suggest again to merge those first? That would allow simplifying this PR and allowing to move forward with other PRs depending on #14588 and #14682.

@markshannon

Base PR for other PRs that want to play with `type.__call__` such as #13930 and #14589. The author is really @markshannon I just made the PR. https://bugs.python.org/issue37207 Automerge-Triggered-By: @encukou

@markshannon

Base PR for other PRs that want to play with `type.__call__` such as python#13930 and python#14589. The author is really @markshannon I just made the PR. https://bugs.python.org/issue37207 Automerge-Triggered-By: @encukou

…ch previous error messages.

encukou · 2019-09-11T17:29:50Z

It would make sense to delegate tp_call for range() and list() to vectorcall, rather than duplicating the implementations.

methane · 2019-09-12T11:53:43Z

 {
 type->tp_flags |= _Py_TPFLAGS_HAVE_VECTORCALL;
+ /* Also inherit tp_vectorcall if tp_call of the
+ * metaclass is type.__call__ */


Suggested change
*metaclassistype.__call__*/
*metaclassistype.__call__*/

methane · 2019-09-12T11:56:26Z

+ if (nargs == 1) {
+ int err = 0;
+ PyObject *arg = args[0];
+ _Py_IDENTIFIER(keys);


I don't like two same identifiers in single source file.
Please make _Py_IDENTIFIER(keys); global static variable.

@markshannon

Base PR for other PRs that want to play with `type.__call__` such as python#13930 and python#14589. The author is really @markshannon I just made the PR. https://bugs.python.org/issue37207 Automerge-Triggered-By: @encukou

encukou · 2020-02-11T16:45:20Z

This PR has 4 relatively independent parts, and all of it seems stuck on one of them. I opened #18464 to get range() out of the way.

This continues the `range()` part of #13930. The complete pull request is stalled on discussions around dicts, but `range()` should not be controversial. (And I plan to open PRs for other parts if this is merged.) On top of Mark's change, I unified `range_new` and `range_vectorcall`, which had a lot of duplicate code. https://bugs.python.org/issue37207

vstinner · 2020-03-30T12:18:08Z

This PR was splitted into multiple smaller PRs which all are now all merged: see https://bugs.python.org/issue37207

@markshannon

Base PR for other PRs that want to play with `type.__call__` such as python#13930 and python#14589. The author is really @markshannon I just made the PR. https://bugs.python.org/issue37207 Automerge-Triggered-By: @encukou

markshannon requested a review from methane as a code owner June 9, 2019 09:40

the-knights-who-say-ni added the CLA signed label Jun 9, 2019

bedevere-bot added the awaiting core review label Jun 9, 2019

ulfalizer reviewed Jun 9, 2019
View reviewed changes

Comment threadObjects/rangeobject.c Outdated

tirkarthi reviewed Jun 9, 2019
View reviewed changes

Comment threadObjects/listobject.c Outdated

methane reviewed Jun 10, 2019
View reviewed changes

Comment threadObjects/rangeobject.c Outdated

ZackerySpytz reviewed Jun 11, 2019
View reviewed changes

jdemeyer reviewed Jun 14, 2019
View reviewed changes

jdemeyer requested changes Jul 4, 2019
View reviewed changes

jdemeyer mentioned this pull request Jul 5, 2019
bpo-37207: enable vectorcall for type.__call__ #14588
Merged

markshannon added 8 commits September 11, 2019 10:18

Use vector call for range().
c01d5b5

Use vector call for list()
6075fc1

Use vector call for dict()
9d86ea5

Add news entry.
20eea92

Fix up error messages in list/dict/range vectorcall implements to mat…
821af67
…ch previous error messages.

Fix refcount leak in range_vectorcall.
0049db8

Reformat vectorcall functions to conform to standard coding style.
8d4a250

Minor formatting changes.
989a3d7

markshannon force-pushed the vectorcall-range branch from 4671080 to fec6969Compare September 11, 2019 09:23

markshannon added 2 commits September 12, 2019 10:08

Inherit tp_vectorcall when safe to do so.
525ff6c

Fix compiler warning.
c319141

markshannon force-pushed the vectorcall-range branch from fec6969 to c319141Compare September 12, 2019 09:11

methane reviewed Sep 12, 2019
View reviewed changes

encukou mentioned this pull request Feb 11, 2020
bpo-37207: Use vectorcall for range() #18464
Merged

encukou mentioned this pull request Mar 11, 2020
bpo-37207: Use vertorcall for list() #18928
Merged

vstinner closed this Mar 30, 2020

Uh oh!

Uh oh!

Conversation

markshannon commented Jun 9, 2019• edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tirkarthi commented Jun 9, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ZackerySpytzJun 11, 2019

Choose a reason for hiding this comment

Uh oh!

jdemeyer commented Jun 11, 2019

Uh oh!

jdemeyerJun 14, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdemeyerJun 19, 2019

Choose a reason for hiding this comment

Uh oh!

markshannonJun 23, 2019

Choose a reason for hiding this comment

Uh oh!

jdemeyerJun 23, 2019

Choose a reason for hiding this comment

Uh oh!

jdemeyer commented Jun 14, 2019

Uh oh!

jdemeyer commented Jun 17, 2019

Uh oh!

jdemeyer left a comment

Choose a reason for hiding this comment

Uh oh!

jdemeyer commented Jul 4, 2019

Uh oh!

methane commented Jul 4, 2019

Uh oh!

jdemeyer commented Jul 4, 2019

Uh oh!

jdemeyer commented Jul 10, 2019

Uh oh!

jdemeyer commented Aug 5, 2019

Uh oh!

encukou commented Sep 11, 2019

Uh oh!

methaneSep 12, 2019

Choose a reason for hiding this comment

Uh oh!

methaneSep 12, 2019

Choose a reason for hiding this comment

Uh oh!

encukou commented Feb 11, 2020

Uh oh!

vstinner commented Mar 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

markshannon commented Jun 9, 2019•
edited by bedevere-bot
Loading

jdemeyerJun 14, 2019•
edited
Loading