classification
Title: Simplify "with"-related opcodes
Type: performance Stage: patch review
Components: Interpreter Core Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: Mark.Shannon, benjamin.peterson, gregory.p.smith, ncoghlan, pitrou, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2018-02-25 13:43 by serhiy.storchaka, last changed 2018-07-08 19:42 by gregory.p.smith.

Pull Requests
URL Status Linked Edit
PR 5883 open serhiy.storchaka, 2018-02-25 13:48
PR 5112 closed serhiy.storchaka, 2018-03-13 20:22
Messages (9)
msg312813 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-02-25 13:43
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
msg312878 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-02-26 06:18
This will not solve issue29988 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).
msg312880 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-02-26 08:03
Updated PR seems fixes issue29988 for synchronous "with".
msg314013 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2018-03-17 21:05
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
msg314039 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-18 13:15
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?
msg314058 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2018-03-18 21:07
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:
msg314059 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-03-18 21:16
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.
msg314060 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2018-03-18 21:37
It is fiddly to get the frame-setlineno code right for duplicated catch blocks, but it is far from impossible.
msg321287 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2018-07-08 19:42
Additional related PR: https://github.com/python/cpython/pull/6641 tied to issue33387 which proposes replacing the existing with and try related bytecodes with two simpler ones: RERAISE and WITH_EXCEPT_FINISH.
History
Date User Action Args
2018-07-08 19:42:39gregory.p.smithsetnosy: + gregory.p.smith
messages: + msg321287
2018-03-18 21:37:32Mark.Shannonsetmessages: + msg314060
2018-03-18 21:16:51serhiy.storchakasetmessages: + msg314059
2018-03-18 21:07:29Mark.Shannonsetmessages: + msg314058
2018-03-18 13:15:49serhiy.storchakasetmessages: + msg314039
2018-03-17 21:05:45Mark.Shannonsetmessages: + msg314013
2018-03-13 20:27:13serhiy.storchakalinkissue33072 superseder
2018-03-13 20:22:55serhiy.storchakasetpull_requests: + pull_request5873
2018-02-26 08:03:10serhiy.storchakasetmessages: + msg312880
2018-02-26 06:18:42serhiy.storchakasetnosy: + ncoghlan
messages: + msg312878
2018-02-25 13:48:21serhiy.storchakasetkeywords: + patch
stage: patch review
pull_requests: + pull_request5658
2018-02-25 13:43:14serhiy.storchakacreate