GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-29312: use METH_FASTCALL for dict.update by jdemeyer · Pull Request #14589 · python/cpython

Conversation

@jdemeyer

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue29312

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Jul 4, 2019
@bedevere-bot bedevere-bot added the awaiting review label Jul 4, 2019
@jdemeyer
jdemeyer force-pushed the dict_update branch from 07fbc54 to b43143b Compare July 5, 2019 09:29
@jdemeyer jdemeyer mentioned this pull request Jul 5, 2019
bpo-37207: enable vectorcall for type.__call__ #14588
Merged
@jdemeyer
jdemeyer force-pushed the dict_update branch 2 times, most recently from 35484d9 to 06302fe Compare July 8, 2019 19:06
@jdemeyer

jdemeyer commented Jul 8, 2019

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@jdemeyer
jdemeyer force-pushed the dict_update branch from 06302fe to 0946cb1 Compare July 9, 2019 09:54
@jdemeyer

jdemeyer commented Jul 9, 2019

Copy link
Copy Markdown
Contributor Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Jul 9, 2019
View reviewed changes
Comment thread Lib/test/test_dict.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_dict.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c Outdated

@vstinner vstinner Jul 9, 2019

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jdemeyer jdemeyer Jul 9, 2019

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
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
bpo-29312: use METH_FASTCALL for dict.update()
df4444a
@jdemeyer
jdemeyer force-pushed the dict_update branch from 72df14e to df4444a Compare August 16, 2019 20:57
@jdemeyer

jdemeyer commented Aug 16, 2019

Copy link
Copy Markdown
Contributor Author

Rebased to latest master, using #14682.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner added the skip news label Aug 19, 2019
@vstinner

vstinner commented Aug 19, 2019

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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
methane
methane reviewed Sep 12, 2019
View reviewed changes
Comment thread Objects/dictobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
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
@csabella csabella added awaiting changes and removed awaiting review labels Feb 4, 2020
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

iritkatriel commented Apr 11, 2022

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@methane methane closed this Apr 12, 2022
@methane

methane commented Apr 12, 2022

Copy link
Copy Markdown
Member

Yes. This PR is also rejected.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@jdemeyer @vstinner @iritkatriel @methane @the-knights-who-say-ni @csabella @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.