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

methane · 2019-04-12T04:33:41Z

https://bugs.python.org/issue20180

tirkarthi · 2019-04-12T05:22:40Z

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 · 2019-04-12T05:27:06Z

Yes. Calling convention is changed from METH_VARARGS to METH_FASTCALL.

tirkarthi · 2019-04-12T06:22:01Z

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

methane · 2019-04-12T06:32:25Z

I am not sure about it.

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

tirkarthi · 2019-04-12T07:08:11Z

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 added 2 commits Apr 12, 2019

clinic: dict.pop()

6355604

clinic: dict.popitem()

8320c7e

methane added skip issue skip news labels Apr 12, 2019

the-knights-who-say-ni added the CLA signed label Apr 12, 2019

bedevere-bot added the awaiting core review label Apr 12, 2019

run clinic

c32ca13

update dict.popitem() docstring

c164150

methane changed the title ~~clinic: dict.pop() and dict.popitem()~~ bpo-20180: Use argument clinic for dict.pop() and dict.popitem() Apr 12, 2019

methane removed skip issue skip news labels Apr 12, 2019

add NEWS entry

aeca91c

methane merged commit 9e4f2f3 into python:master Apr 12, 2019

bedevere-bot removed the awaiting core review label Apr 12, 2019

methane deleted the dict-clinic branch Apr 12, 2019

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

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

methane commented Apr 12, 2019 •

edited by bedevere-bot

tirkarthi commented Apr 12, 2019

methane commented Apr 12, 2019

tirkarthi commented Apr 12, 2019

methane commented Apr 12, 2019 •

edited

tirkarthi commented Apr 12, 2019

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

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

Conversation

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

tirkarthi commented Apr 12, 2019

methane commented Apr 12, 2019

tirkarthi commented Apr 12, 2019

methane commented Apr 12, 2019 • edited

tirkarthi commented Apr 12, 2019

methane commented Apr 12, 2019 •

edited by bedevere-bot

methane commented Apr 12, 2019 •

edited