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.

Sign up for GitHub

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

Frames should store next_instr instead of lasti #91333

Closed
brandtbucher opened this issue Mar 31, 2022 · 2 comments
Closed

Frames should store next_instr instead of lasti #91333

brandtbucher opened this issue Mar 31, 2022 · 2 comments
Assignees
@brandtbucher
Labels
3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@brandtbucher
Copy link
Member

BPO 47177
Nosy @markshannon, @brandtbucher
PRs
  • bpo-47177: Replace f_lasti with prev_instr #32208
    		•

    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/brandtbucher'
closed_at = <Date 2022-04-07.19:32:09.034>
created_at = <Date 2022-03-31.01:12:26.728>
labels = ['interpreter-core', '3.11', 'performance']
title = 'Frames should store next_instr instead of lasti'
updated_at = <Date 2022-04-07.19:32:09.033>
user = 'https://github.com/brandtbucher'

    bugs.python.org fields:

    activity = <Date 2022-04-07.19:32:09.033>
actor = 'brandtbucher'
assignee = 'brandtbucher'
closed = True
closed_date = <Date 2022-04-07.19:32:09.034>
closer = 'brandtbucher'
components = ['Interpreter Core']
creation = <Date 2022-03-31.01:12:26.728>
creator = 'brandtbucher'
dependencies = []
files = []
hgrepos = []
issue_num = 47177
keywords = ['patch']
message_count = 2.0
messages = ['416409', '416938']
nosy_count = 2.0
nosy_names = ['Mark.Shannon', 'brandtbucher']
pr_nums = ['32208']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'performance'
url = 'https://bugs.python.org/issue47177'
versions = ['Python 3.11']
    @brandtbucher
    Copy link
    Member Author

    Rather than maintaining the offset of the "last instruction" (f_lasti), interpreter frames should instead just maintain a pointer to the true next instruction. This has several benefits, most notably reducing the register pressure associated with loading first_instr on every instruction and call in the main interpreter loop:

    When entering a frame:

    • Before: next_instr = first_instr + frame->f_lasti + 1;
    • After: next_instr = frame->next_instr;

    When starting a new instruction:

    • Before: frame->next_instr = next_instr++ - first_instr;
    • After: frame->next_instr = ++next_instr;

    Benchmarks suggest that this overhead is surprisingly significant (something like 2%).

    @brandtbucher brandtbucher added the 3.11 only security fixes label Mar 31, 2022
    @brandtbucher brandtbucher self-assigned this Mar 31, 2022
    @brandtbucher brandtbucher added interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.11 only security fixes performance Performance or resource usage labels Mar 31, 2022
    @brandtbucher brandtbucher self-assigned this Mar 31, 2022
    @brandtbucher brandtbucher added interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage labels Mar 31, 2022
    @brandtbucher
    Copy link
    Member Author

    New changeset ef6a482 by Brandt Bucher in branch 'main':
    bpo-47177: Replace f_lasti with prev_instr (GH-32208)
    ef6a482

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @brandtbucher brandtbucher

    Labels
    3.11 only security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    1 participant
    @brandtbucher