Skip to content
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.

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

gh-91407: Merge BINARY_SUBSCR_LIST_INT with BINARY_SUBSCR_TUPLE_INT #32404

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

gh-91407: Merge BINARY_SUBSCR_LIST_INT with BINARY_SUBSCR_TUPLE_INT #32404

wants to merge 3 commits into from
+95 −124

Conversation

eendebakpt
Copy link
Contributor

@eendebakpt eendebakpt commented Apr 7, 2022 •
edited

Merge BINARY_SUBSCR_LIST_INT and BINARY_SUBSCR_TUPLE_INT. This reduces the number of opcodes by 1 and eliminates some duplicated code.

Details The reason for this PR is not performance, but here microbenchmarks to check there is no slowdown: 
list: Mean +- std dev: [base] 14.7 ns +- 0.2 ns -> [patch] 14.7 ns +- 0.2 ns: 1.00x faster
tuple: Mean +- std dev: [base] 14.6 ns +- 0.2 ns -> [patch] 14.8 ns +- 0.4 ns: 1.02x slower
mixed: Mean +- std dev: [base] 25.7 us +- 0.9 us -> [patch] 25.0 us +- 0.3 us: 1.03x faster

Geometric mean: 1.01x faster

Code

import pyperf
runner = pyperf.Runner()

setup='l=[1,2,None,4,5,6,7,8,9,10]; t=(1,None,3,4)'

runner.timeit(name=f"list", stmt=f"x=l[3]",setup=setup)
runner.timeit(name=f"tuple", stmt=f"x=t[2]",setup=setup)
code="""
for ii in range(500):
	idx=ii%4
	a=l[idx]
	b=t[idx]	
"""
runner.timeit(name=f"mixed", stmt=code,setup=setup)

https://bugs.python.org/issue47251

@eendebakpt eendebakpt marked this pull request as draft Apr 7, 2022
@eendebakpt eendebakpt marked this pull request as ready for review Apr 7, 2022
corona10
corona10 reviewed Apr 8, 2022
View changes
Copy link
Member

@corona10 corona10 left a comment •
edited

IMHO, you may need to run all pyperformance benchmarks with a reliable machine.

@markshannon
Copy link
Member

@markshannon markshannon commented Apr 8, 2022

Please read https://github.com/python/cpython/blob/main/Python/adaptive.md#implementation-of-specialized-instructions
This adds branches into specialized instructions, which we are trying to avoid.

We could reduce code duplication by merging the code paths after the type check and array pointer extraction.
Something like:

PyObject **array

tuple:
     DEOPT_IF(Py_TYPE(obj) != &PyTuple_Type);
     array = &PyTuple_GET_ITEM(0);
     goto shared_code;
tuple:
     DEOPT_IF(Py_TYPE(obj) != &PyList_Type);
     array = &PyList_GET_ITEM(0);
     goto shared_code;

@eendebakpt eendebakpt marked this pull request as draft Apr 9, 2022
@eendebakpt eendebakpt mannequin mentioned this pull request Apr 11, 2022
Open
@eendebakpt eendebakpt mannequin changed the title bpo-47251: Merge BINARY_SUBSCR_LIST_INT with BINARY_SUBSCR_LIST_TUPLE gh-91407: Merge BINARY_SUBSCR_LIST_INT with BINARY_SUBSCR_TUPLE_INT Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corona10 corona10

@markshannon markshannon

Assignees
No one assigned
Labels
awaiting review CLA signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@eendebakpt @markshannon @corona10 @the-knights-who-say-ni @bedevere-bot