Skip to content

gh-91407: Merge BINARY_SUBSCR_LIST_INT with BINARY_SUBSCR_TUPLE_INT#32404

Closed
eendebakpt wants to merge 3 commits intopython:mainfrom
eendebakpt:combine_list_tuple_opcode
Closed

gh-91407: Merge BINARY_SUBSCR_LIST_INT with BINARY_SUBSCR_TUPLE_INT#32404
eendebakpt wants to merge 3 commits intopython:mainfrom
eendebakpt:combine_list_tuple_opcode

Conversation

@eendebakpt
Copy link
Contributor

@eendebakpt eendebakpt commented Apr 7, 2022 •
edited
Loading

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)

See #91407

https://bugs.python.org/issue47251

@eendebakpt eendebakpt requested a review from markshannon as a code owner April 7, 2022 19:12
@eendebakpt eendebakpt marked this pull request as draft April 7, 2022 19:12
@eendebakpt eendebakpt marked this pull request as ready for review April 7, 2022 20:00
corona10
corona10 reviewed Apr 8, 2022
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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

@markshannon
Copy link
Member

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 April 9, 2022 11:43
@eendebakpt eendebakpt mannequin mentioned this pull request Apr 11, 2022
Closed
@eendebakpt eendebakpt 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
@eendebakpt eendebakpt closed this Apr 25, 2022
@eendebakpt eendebakpt deleted the combine_list_tuple_opcode branch June 26, 2025 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@corona10 corona10 corona10 left review comments

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@eendebakpt @markshannon @corona10 @the-knights-who-say-ni @bedevere-bot