X Tutup
Skip to content

bpo-20180: Use argument clinic for dict.pop() and dict.popitem()#12792

Merged
methane merged 5 commits intopython:masterfrom
methane:dict-clinic
Apr 12, 2019
Merged

bpo-20180: Use argument clinic for dict.pop() and dict.popitem()#12792
methane merged 5 commits intopython:masterfrom
methane:dict-clinic

Conversation

@methane
Copy link
Copy Markdown
Member

@methane methane commented Apr 12, 2019 •
edited by bedevere-bot
Loading

@tirkarthi
Copy link
Copy Markdown
Member

tirkarthi commented Apr 12, 2019

Does this provide performance improvement for dict.pop ? I am using a non-PGO build for benchmarking.

➜  cpython git:(master) ./python.exe -m timeit -s 'd = {"a": 1}' 'd.pop("a", None)'
2000000 loops, best of 5: 125 nsec per loop
➜  cpython git:(master) ./python.exe -m timeit -s 'd = {"a": 1}' 'd.pop("a", None)'
2000000 loops, best of 5: 127 nsec per loop
➜  cpython git:(master) ./python.exe -m timeit -s 'd = {"a": 1}' 'd.pop("a", None)'
2000000 loops, best of 5: 127 nsec per loop

➜  cpython git:(pr_12792) ./python.exe -m timeit -s 'd = {"a": 1}' 'd.pop("a", None)'
5000000 loops, best of 5: 82.7 nsec per loop
➜  cpython git:(pr_12792) ./python.exe -m timeit -s 'd = {"a": 1}' 'd.pop("a", None)'
5000000 loops, best of 5: 80.3 nsec per loop
➜  cpython git:(pr_12792) ./python.exe -m timeit -s 'd = {"a": 1}' 'd.pop("a", None)'
5000000 loops, best of 5: 74.5 nsec per loop

@methane
Copy link
Copy Markdown
Member Author

methane commented Apr 12, 2019 via email

@tirkarthi
Copy link
Copy Markdown
Member

tirkarthi commented Apr 12, 2019

Is it worth adding a NEWS entry for such speedups since this is a considerable one reducing 120ns to 80ns.

@methane
Copy link
Copy Markdown
Member Author

methane commented Apr 12, 2019 •
edited
Loading

I am not sure about it.

Some methods changed from METH_VARARGS to METH_FASTCALL without NEWS entry.
For example, #9560, #9164.

@methane methane changed the title clinic: dict.pop() and dict.popitem() bpo-20180: Use argument clinic for dict.pop() and dict.popitem() Apr 12, 2019
@tirkarthi
Copy link
Copy Markdown
Member

tirkarthi commented Apr 12, 2019

My rationale towards adding them is that unless someones follows CPython commits these optimizations are hard to find. Adding them to changelog might encourage users and give better outlook on how CPython is getting faster, given that it's dict.pop which is slightly more common. But it's your call on adding a NEWS entry.

@methane methane merged commit 9e4f2f3 into python:master Apr 12, 2019
@methane methane deleted the dict-clinic branch April 12, 2019 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@methane @tirkarthi @the-knights-who-say-ni @bedevere-bot
X Tutup