Skip to content

bpo-29312: use METH_FASTCALL for dict.update #14589

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

bpo-29312: use METH_FASTCALL for dict.update #14589

wants to merge 1 commit into from
+98 −12

Conversation

jdemeyer
Copy link
Contributor

@jdemeyer jdemeyer commented Jul 4, 2019 •
edited by bedevere-bot
Loading

@jdemeyer jdemeyer mentioned this pull request Jul 5, 2019
Merged
@jdemeyer jdemeyer force-pushed the dict_update branch 2 times, most recently from 35484d9 to 06302fe Compare July 8, 2019 19:06
@jdemeyer
Copy link
Contributor Author

jdemeyer commented Jul 8, 2019

Here are some benchmarks:

  1. ./python -m perf timeit --duplicate 200 -s 'L = [("x",None)]' 'dict(L)'
    before: Mean +- std dev: 221 ns +- 1 ns
    after: Mean +- std dev: 186 ns +- 1 ns
  2. ./python -m perf timeit --duplicate 200 -s 'L = [("x",None)]' '{}.update(L)'
    before: Mean +- std dev: 189 ns +- 2 ns
    after: Mean +- std dev: 161 ns +- 1 ns
  3. ./python -m perf timeit --duplicate 200 -s 'D = {"x":None}' 'dict(D)'
    before: Mean +- std dev: 196 ns +- 1 ns
    after: Mean +- std dev: 126 ns +- 1 ns
  4. ./python -m perf timeit --duplicate 200 -s 'D = {"x":None}' '{}.update(D)'
    before: Mean +- std dev: 174 ns +- 1 ns
    after: Mean +- std dev: 116 ns +- 0 ns
  5. ./python -m perf timeit --duplicate 200 'dict(x=None)'
    before: Mean +- std dev: 200 ns +- 1 ns
    after: Mean +- std dev: 119 ns +- 1 ns
  6. ./python -m perf timeit --duplicate 200 '{}.update(x=None)'
    before: Mean +- std dev: 201 ns +- 1 ns
    after: Mean +- std dev: 120 ns +- 1 ns
  7. ./python -m perf timeit --duplicate 200 -s 'D = {"x":None}' 'dict(**D)'
    before: Mean +- std dev: 149 ns +- 1 ns
    after: Mean +- std dev: 171 ns +- 1 ns
  8. ./python -m perf timeit --duplicate 200 -s 'D = {"x":None}' '{}.update(**D)'
    before: Mean +- std dev: 139 ns +- 1 ns
    after: Mean +- std dev: 171 ns +- 1 ns
  9. ./python -m perf timeit --duplicate 200 -s 'D = {f"x{i}":None for i in range(10000)}' 'dict(**D)'
    before: Mean +- std dev: 321 us +- 1 us
    after: Mean +- std dev: 393 us +- 3 us
  10. ./python -m perf timeit --duplicate 200 -s 'D = {f"x{i}":None for i in range(10000)}' '{}.update(**D)'
    before: Mean +- std dev: 321 us +- 3 us
    after: Mean +- std dev: 394 us +- 4 us

So the calls dict(**D) and dict.update(**D) got slower but the rest got faster.

@jdemeyer jdemeyer marked this pull request as ready for review July 8, 2019 19:53
@jdemeyer jdemeyer requested a review from methane as a code owner July 8, 2019 19:53
vstinner
vstinner reviewed Jul 9, 2019
View reviewed changes
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your PR title says "use METH_FASTCALL for dict.update" but you modified PyDictType.tp_init as well. Can you please move your tp_init change in a separated PR?

@jdemeyer
Copy link
Contributor Author

jdemeyer commented Jul 9, 2019

I changed the branch to match the title, reverting the changes to __init__.

Note that this PR has more code duplication: the code paths for dict.update and dict.__init__ used to be the same but that's no longer the case with this PR. When dict.__init__ is also changed (as in my original PR), the code paths will be the same again.

vstinner
vstinner reviewed Jul 9, 2019
View reviewed changes
Objects/dictobject.c

_Py_IDENTIFIER(keys);
PyObject *func;
if (_PyObject_LookupAttrId(obj, &PyId_keys, &func) < 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds redundant with dict_merge() which calls PyMapping_Keys(). Maybe dict_merge() could be modified to dispatch to PyDict_MergeFromSeq2() if PyMapping_Keys() keys with an attribute error, rather than trying to get the attribute twice?

It's not directly related to your change, it can be done/discussed in a separated PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, I also noticed this. There is some room for improvement here. Let's do it in a new PR after this is merged.

miss-islington pushed a commit that referenced this pull request Aug 15, 2019
@jdemeyer @miss-islington
bpo-37207: enable vectorcall for type.__call__ (GH-14588)
37806f4 
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
@jdemeyer
Copy link
Contributor Author

Rebased to latest master, using #14682.

@vstinner
Copy link
Member

@methane: I let you decide what do to do with this change.

lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
@jdemeyer @lisroach
bpo-37207: enable vectorcall for type.__call__ (pythonGH-14588)
3c51383 
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
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
@jdemeyer @DinoV
bpo-37207: enable vectorcall for type.__call__ (pythonGH-14588)
c784435 
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
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
@jdemeyer @websurfer5
bpo-37207: enable vectorcall for type.__call__ (pythonGH-14588)
9cea68a 
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
@iritkatriel
Copy link
Member

https://bugs.python.org/issue29312 is closed. What is that status of this PR?

@methane methane closed this Apr 12, 2022
@methane
Copy link
Member

methane commented Apr 12, 2022

Yes. This PR is also rejected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vstinner vstinner vstinner left review comments

@methane methane methane left review comments

Assignees
No one assigned
Labels
awaiting changes skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jdemeyer @vstinner @iritkatriel @methane @the-knights-who-say-ni @csabella @bedevere-bot