[issue32949] Simplify "with"-related opcodes

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Sure.

--
resolution:  -> out of date
stage: patch review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32949] Simplify "with"-related opcodes

[Cheryl Sabella]

Cheryl Sabella  added the comment:

Should this be closed now that PR6641 has been merged?

--
nosy: +cheryl.sabella

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32949] Simplify "with"-related opcodes

[Gregory P. Smith]

Change by Gregory P. Smith :


--
versions: +Python 3.9 -Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32949] Simplify "with"-related opcodes

[Inada Naoki]

Change by Inada Naoki :


--
nosy: +inada.naoki

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32949] Simplify "with"-related opcodes

[Gregory P. Smith]

Gregory P. Smith  added the comment:

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.

--
nosy: +gregory.p.smith

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32949] Simplify "with"-related opcodes

[Mark Shannon]

Mark Shannon  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32949] Simplify "with"-related opcodes

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32949] Simplify "with"-related opcodes

[Mark Shannon]

Mark Shannon  added the comment:

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:

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32949] Simplify "with"-related opcodes

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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?

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32949] Simplify "with"-related opcodes

[Mark Shannon]

Mark Shannon  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32949] Simplify "with"-related opcodes

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
pull_requests: +5873

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32949] Simplify "with"-related opcodes

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

Updated PR seems fixes issue29988 for synchronous "with".

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32949] Simplify "with"-related opcodes

[Serhiy Storchaka]

Serhiy Storchaka  added the comment:

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).

--
nosy: +ncoghlan

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32949] Simplify "with"-related opcodes

[Serhiy Storchaka]

Change by Serhiy Storchaka :


--
keywords: +patch
pull_requests: +5658
stage:  -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue32949] Simplify "with"-related opcodes

[Serhiy Storchaka]

New submission from Serhiy Storchaka :

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

--
components: Interpreter Core
messages: 312813
nosy: Mark.Shannon, benjamin.peterson, pitrou, serhiy.storchaka
priority: normal
severity: normal
status: open
title: Simplify "with"-related opcodes
type: performance
versions: Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com