Remove C stack use by specializing BINARY_SUBSCR, STORE_SUBSCR, LOAD_ATTR, and STORE_ATTR

[markshannon]

BPO	45829
Nosy	@gvanrossum, @pmp-p, @markshannon, @pablogsal, @brandtbucher, @sweeneyde
PRs	bpo-45829: Specialize BINARY_SUBSCR for __getitem__ implemented in Python. #29592 bpo-45829: Check __getitem__'s version for overflow before specializing #30129

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/markshannon'
closed_at = None
created_at = <Date 2021-11-17.09:50:11.128>
labels = ['interpreter-core', '3.11', 'performance']
title = 'Remove C stack use by specializing BINARY_SUBSCR, STORE_SUBSCR, LOAD_ATTR, and STORE_ATTR'
updated_at = <Date 2021-12-16.11:08:35.974>
user = 'https://github.com/markshannon'

bugs.python.org fields:

activity = <Date 2021-12-16.11:08:35.974>
actor = 'Mark.Shannon'
assignee = 'Mark.Shannon'
closed = False
closed_date = None
closer = None
components = ['Interpreter Core']
creation = <Date 2021-11-17.09:50:11.128>
creator = 'Mark.Shannon'
dependencies = []
files = []
hgrepos = []
issue_num = 45829
keywords = ['patch']
message_count = 7.0
messages = ['406461', '406473', '406476', '406477', '406530', '406573', '408687']
nosy_count = 6.0
nosy_names = ['gvanrossum', 'pmpp', 'Mark.Shannon', 'pablogsal', 'brandtbucher', 'Dennis Sweeney']
pr_nums = ['29592', '30129']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue45829'
versions = ['Python 3.11']

Linked PRs

• GH-89987: Shrink the BINARY_SUBSCR caches #103022

[markshannon]

We can remove the C stack use and general overhead of calling special methods implemented in Python for attribute access and indexing.

Each operation has a special method that implements it. When that special method is implemented in Python, we should avoid the tp_xxx slot machinery and use the same mechanism we use for normal calls to Python functions.

• BINARY_SUBSCR: __getitem__
• STORE_SUBSCR: __setitem__
• LOAD_ATTR: __getattribute__ (and maybe __getattr__)
• STORE_ATTR: __setattr__

It probably isn't worth bothering with the deletion forms.

The getters (__getitem__ and __getattribute__) are relatively simple, as the call returns the result.

The setters are a bit more complicated as the return value needs to be discarded, so an additional frame which discards the result of the call needs to be inserted.

[gvanrossum]

Of these, presumably LOAD_GETATTR is by far the most used, so should we try that first?

[markshannon]

I don't think it matter much which we do first.
I happened to do BINARY_SUBSCR first.

[gvanrossum]

That's a good one too, and perhaps simpler.

[markshannon]

New changeset 21fa7a3 by Mark Shannon in branch 'main':
bpo-45829: Specialize BINARY_SUBSCR for __getitem__ implemented in Python. (GH-29592)
21fa7a3

[sweeneyde]

This snippet occurs a couple of times in ceval.c (BINARY_SUBSCR_GETITEM and CALL_FUNCTION_PY_SIMPLE):

        new_frame->previous = frame;
        frame = cframe.current_frame = new_frame;
        new_frame->depth = frame->depth + 1;

Maybe I'm reading it wrong, but I think the last line is just setting new_frame->depth++, leaving new_frame->depth = 1 instead of frame->previous->depth + 1.

I think the second and third lines should be swapped?

[markshannon]

New changeset 62a8a0c by Brandt Bucher in branch 'main':
bpo-45829: Check __getitem__'s version for overflow before specializing (GH-30129)
62a8a0c

[iritkatriel]

Is there anything left to do here?

[arhadthedev]

Is there anything left to do here?

@markshannon ping?

[arhadthedev]

The issue and associated PRs have neither activity for one and a half year nor a checklist or a comment stating further work. Thus closing as completed.