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

Simplify "with"-related opcodes #77130

Closed
serhiy-storchaka opened this issue Feb 25, 2018 · 11 comments
Closed

Simplify "with"-related opcodes #77130

serhiy-storchaka opened this issue Feb 25, 2018 · 11 comments
Labels
3.9 interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage

Comments

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Feb 25, 2018

BPO 32949
Nosy @gpshead, @ncoghlan, @pitrou, @benjaminp, @methane, @markshannon, @serhiy-storchaka, @csabella
PRs
  • bpo-32949: Simplify "with"-related opcodes. #5883
  • bpo-32949: Better bytecodes for "with" statement. #5112
  • 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 = None
    closed_at = <Date 2019-12-13.06:52:43.543>
    created_at = <Date 2018-02-25.13:43:14.188>
    labels = ['interpreter-core', '3.9', 'performance']
    title = 'Simplify "with"-related opcodes'
    updated_at = <Date 2019-12-13.06:52:43.542>
    user = 'https://github.com/serhiy-storchaka'

    bugs.python.org fields:

    activity = <Date 2019-12-13.06:52:43.542>
    actor = 'serhiy.storchaka'
    assignee = 'none'
    closed = True
    closed_date = <Date 2019-12-13.06:52:43.543>
    closer = 'serhiy.storchaka'
    components = ['Interpreter Core']
    creation = <Date 2018-02-25.13:43:14.188>
    creator = 'serhiy.storchaka'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 32949
    keywords = ['patch']
    message_count = 11.0
    messages = ['312813', '312878', '312880', '314013', '314039', '314058', '314059', '314060', '321287', '358315', '358322']
    nosy_count = 8.0
    nosy_names = ['gregory.p.smith', 'ncoghlan', 'pitrou', 'benjamin.peterson', 'methane', 'Mark.Shannon', 'serhiy.storchaka', 'cheryl.sabella']
    pr_nums = ['5883', '5112']
    priority = 'normal'
    resolution = 'out of date'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue32949'
    versions = ['Python 3.9']

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Feb 25, 2018

    There are some issues with "with"-related opcodes.

    All other opcodes has constant stack effect for particular control flow. For example FOR_ITER always has the stack effect 1 if not jump (pushes the next item) and -1 if jumps (pops the iterator). The only exceptions are WITH_CLEANUP_START which pushes 1 or 2 values depending on TOS, and WITH_CLEANUP_FINISH which pops 2 or 3 values depending on values pushed by preceding WITH_CLEANUP_START. This breaks consistency and may make debugging harder.

    WITH_CLEANUP_START duplicates a one of values on the stack without good reasons. Even the comment in the initial commit exposed uncertainty in this.

    The proposed PR simplifies WITH_CLEANUP_START and WITH_CLEANUP_FINISH. They will be now executed only when the exception is raised. In normal case they will be replaced with calling a function __exit__(None, None, None).

    LOAD_CONST               0 ((None, None, None))
    CALL_FUNCTION_EX         0
    POP_TOP
    

    WITH_CLEANUP_FINISH will be merged with the following END_FINALLY.

    This PR is inspired by PR 5112 by Mark Shannon, but Mark goes further.

    In addition to simplifying the implementation and the mental model, the PR adds a tiny bit of performance gain.

    $ ./python -m perf timeit -s 'class CM:' -s '  def __enter__(s): pass' -s '  def __exit__(*args): pass' -s 'cm = CM()' -- 'with cm: pass'

    Unpatched: Mean +- std dev: 227 ns +- 6 ns
    Patched: Mean +- std dev: 205 ns +- 10 ns

    @serhiy-storchaka serhiy-storchaka added 3.8 interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage labels Feb 25, 2018
    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Feb 26, 2018

    This will not solve bpo-29988 but will open a way for solving it at least for synchronous "with" (swap POP_BLOCK and the following LOAD_CONST and disable interrupting after POP_BLOCK).

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Feb 26, 2018

    Updated PR seems fixes bpo-29988 for synchronous "with".

    @markshannon
    Copy link
    Member

    markshannon commented Mar 17, 2018

    We have two competing PRs for this issue. Again.

    For comparison, using the same micro-benchmark, PR 5112 has these timings:
    Master branch: Mean +- std dev: 252 ns +- 4 ns
    PR 5112: Mean +- std dev: 216 ns +- 4 ns

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Mar 18, 2018

    Thank you for your PR Mark.

    The main difference between PR 5883 and PR 5112 is that in PR 5883 the pair of old WITH_CLEANUP_FINISH and END_FINALLY are replaced with a single new WITH_CLEANUP_FINISH, and in PR 5112 it is replaced with a sequence of 7 opcodes including a new opcode RERAISE.

    POP_JUMP_IF_TRUE  L
    RERAISE
    

    L:
    POP_TOP
    POP_TOP
    POP_TOP
    POP_EXCEPT
    POP_TOP

    This doesn't affect a performance in normal case because this code is executed only when an exception has been raised (an in that case the performance is less important). And seems this doesn't introduce new race conditions.

    The number of opcodes is the same in both PRs. The implementation of RERAISE in ceval.c in PR 5112 is a tiny bit simpler than the implementation of WITH_CLEANUP_FINISH in PR 5883. But the generated bytecode and the compiler are a tiny bit simpler in PR 5883. If RERAISE be used for other purposes besides implementing a "with" statement, it would be a great advantage.

    For now both approaches look to me not having significant advantages or disadvantages against the other one. Does anybody have preferences?

    @markshannon
    Copy link
    Member

    markshannon commented Mar 18, 2018

    I intend to reuse RERAISE to implement the exceptional case for a finally block. Something like:

    SETUP_FINALLY final
    body
    finalbody
    JUMP exit
    final:
    finalbody
    RERAISE
    exit:

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Mar 18, 2018

    There are problems with the f_lineno setter when duplicate a finally body. The duplication works for "with" only because the cleanup code for "with" doesn't correspond any line number.

    @markshannon
    Copy link
    Member

    markshannon commented Mar 18, 2018

    It is fiddly to get the frame-setlineno code right for duplicated catch blocks, but it is far from impossible.

    @gpshead
    Copy link
    Member

    gpshead commented Jul 8, 2018

    Additional related PR: #6641 tied to bpo-33387 which proposes replacing the existing with and try related bytecodes with two simpler ones: RERAISE and WITH_EXCEPT_FINISH.

    @gpshead gpshead added 3.9 and removed 3.8 labels Aug 21, 2019
    @csabella
    Copy link
    Contributor

    csabella commented Dec 13, 2019

    Should this be closed now that PR6641 has been merged?

    @serhiy-storchaka
    Copy link
    Member Author

    serhiy-storchaka commented Dec 13, 2019

    Sure.

    @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
    Labels
    3.9 interpreter-core Interpreter core (Objects, Python, Grammar, and Parser dirs) performance Performance or resource usage
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants