[WIP] bpo-17611: Move unwinding of stack from interpreter to compiler #2827

pitrou · 2017-07-23T19:20:30Z

https://bugs.python.org/issue17611

mention-bot · 2017-07-23T19:20:36Z

@pitrou, thanks for your PR! By analyzing the history of the files in this pull request, we identified @1st1, @serhiy-storchaka and @benjaminp to be potential reviewers.

pitrou · 2017-07-23T19:36:30Z

The test_dis failures are expected, as I haven't updated that file yet.

pitrou · 2017-07-23T19:42:10Z

However, I don't understand the additional test failures under Windows.

pitrou · 2017-07-23T20:55:33Z

Ok, it seems the Windows failures were due to a tracing bug without computed gotos.

serhiy-storchaka · 2017-07-23T20:58:41Z

Python/compile.c

+            ADDOP_O(c, LOAD_CONST, Py_None, consts);
+            compiler_nameop(c, handler->v.ExceptHandler.name, Store);
+            compiler_nameop(c, handler->v.ExceptHandler.name, Del);
+            ADDOP_JREL(c, JUMP_FORWARD, end);

            /* finally: */
            ADDOP_O(c, LOAD_CONST, Py_None, consts);


Seems this is a dead code.

Thank you, removed.

serhiy-storchaka

Published the part of review comments since the PR is changed.

serhiy-storchaka · 2017-07-24T06:09:00Z

Objects/frameobject.c

@@ -52,7 +52,7 @@ frame_getlineno(PyFrameObject *f, void *closure)
 *  o Lines with an 'except' statement on them can't be jumped to, because
 *    they expect an exception to be on the top of the stack.
 *  o Lines that live in a 'finally' block can't be jumped from or to, since
- *    the END_FINALLY expects to clean up the stack after the 'try' block.
+ *    the RERAISE expects to clean up the stack after the 'try' block.
 *  o 'try'/'for'/'while' blocks can't be jumped into because the blockstack


I think the 'while' block should be excluded from this and the 'with' and 'async with' blocks should be added.

serhiy-storchaka · 2017-07-24T06:33:39Z

Objects/frameobject.c

@@ -52,7 +52,7 @@ frame_getlineno(PyFrameObject *f, void *closure)
 *  o Lines with an 'except' statement on them can't be jumped to, because
 *    they expect an exception to be on the top of the stack.
 *  o Lines that live in a 'finally' block can't be jumped from or to, since


Now there is a different reason for this. Since a 'finally' block is duplicated, different instruction offsets correspond to the same line.

serhiy-storchaka · 2017-07-24T06:40:02Z

Objects/frameobject.c

@@ -176,14 +178,18 @@ frame_setlineno(PyFrameObject *f, PyObject* p_new_lineno)
    }

    /* You can't jump into or out of a 'finally' block because the 'try'
-     * block leaves something on the stack for the END_FINALLY to clean
-     * up.      So we walk the bytecode, maintaining a simulated blockstack.
+     * block leaves something on the stack for the RETRY to clean up.


RERAISE instead of RETRY.

serhiy-storchaka · 2017-07-24T08:04:05Z

Objects/frameobject.c

+                /* This is the start of the 'finally' block.
+                 * It will end with RERAISE (if a 'try..finally' block)
+                 * or WITH_EXCEPT_FINISH (if a 'with' block).
+                 */
                in_finally[blockstack_top-1] = 1;
            }
            else {
                blockstack_top--;


There may be multiple POP_BLOCK instructions following SETUP_EXCEPT. For example see the disassemble of

for i in a: try: if i: break except: b c

serhiy-storchaka · 2017-07-24T08:25:50Z

Objects/frameobject.c

                in_finally[blockstack_top-1] = 1;
            }
            else {
                blockstack_top--;
            }
            break;

-        case END_FINALLY:
-            /* Ignore END_FINALLYs for SETUP_EXCEPTs - they exist


Why this comment is removed? If it no longer valid perhaps some following checks can be replaced with asserts.

serhiy-storchaka · 2017-07-24T08:30:35Z

Objects/frameobject.c

            delta_iblock++;
            break;
-
        case POP_BLOCK:


Due to code duplication several POP_BLOCK instructions can correspond to the single SETUP_EXCEPT or SETUP_FINALLY instruction.

I've now removed all code duplication.

Sorry, I was not clear. Every break, continue and return in try...except block adds a POP_BLOCK if it leaves the block. The number of POP_BLOCKs may be larger than the number of SETUP_EXCEPTs and SETUP_FINALLYs. blockstack_top may become negative. See my comment at https://github.com/python/cpython/pull/2827/files/693b9398b5fd202fa5864f9cc76fa1bc7f84f62e#r128971050.

I see. Would you like to suggest another approach?

I don't have a ready solution. May be we can use target addresses of SETUP_EXCEPT and SETUP_FINALLY instructions (similarly to FOR_ITER). Or add special never executed stop-code at the ends of blocks. Or use additional data structure for starts and ends of all blocks (actually this would allow to get rid of instructions SETUP_EXCEPT, SETUP_FINALLY and POP_BLOCK and speed up stack unrolling; I'm going to try this approach in separate issue).

serhiy-storchaka

I don't understand why JUMP_FINALLY should be a jumping instruction.

serhiy-storchaka · 2017-07-24T10:04:13Z

Python/ceval.c

@@ -2844,6 +2854,18 @@ _PyEval_EvalFrameDefault(PyFrameObject *f, int throwflag)
            FAST_DISPATCH();
        }

+        TARGET(JUMP_FINALLY) {


Can we push 3 NULLs?

serhiy-storchaka · 2017-07-24T10:08:02Z

Python/ceval.c

+            PUSH(exc);
+            PUSH(exc);
+            PUSH(exc);
+            JUMPBY(oparg);


Is it needed? It seems to me that JUMP_FINALLY always jumps to the next instruction.

That might be, I'll give it a try.

…xception

serhiy-storchaka · 2017-07-25T06:02:38Z

Objects/frameobject.c

+        case FOR_ITER:
+            /* Store the loop end in forstack[] */
+            oparg = code[addr + 1];
+            target_addr = addr + oparg + 2;


Use sizeof(_Py_CODEUNIT) instead of 2.

Wow, the entire frameobject code deals with char * code objects? I admit I had glossed a bit over it when doing this PR. I am surprised this hasn't been migrated to more modern spellings...

serhiy-storchaka · 2017-07-25T06:03:06Z

Objects/frameobject.c

+        switch (op) {
+        case FOR_ITER:
+            /* Store the loop end in forstack[] */
+            oparg = code[addr + 1];


Don't forget about EXTENDED_ARG. See get_arg() in peephole.c.

serhiy-storchaka · 2017-07-25T06:13:55Z

Objects/frameobject.c

+            oparg = code[addr + 1];
+            target_addr = addr + oparg + 2;
+            assert(target_addr < code_len);
+            forstack[forstack_top++] = target_addr;


forstack is not needed for determining whether the jump is allowed.

The jump is forbidden if (addr < first_addr && first_addr < target_addr) != (addr < second_addr && second_addr < target_addr) for any 'for' block.

If assume first_addr < second_addr and iterate while addr < second_addr this may be optimized to (first_addr < target_addr) && ((addr < first_addr) != (second_addr < target_addr)).

The jump is forbidden if (addr < first_addr && first_addr < target_addr) != (addr < second_addr && second_addr < target_addr) for any 'for' block.

I don't think this is true. It is actually allowed to jump out of a for loop, just not into.

Then following pseudocode:

first_in = addr < first_addr && first_addr < target_addr; second_in = addr < second_addr && second_addr < target_addr; if (!first_in && second_in) goto forbidden; /* or first_in < second_in */ for_loop_delta += second_in - first_in; /* non-positive */

serhiy-storchaka · 2017-07-25T07:23:50Z

Objects/frameobject.c

            delta_iblock++;
            break;
-
        case POP_BLOCK:


Sorry, I was not clear. Every break, continue and return in try...except block adds a POP_BLOCK if it leaves the block. The number of POP_BLOCKs may be larger than the number of SETUP_EXCEPTs and SETUP_FINALLYs. blockstack_top may become negative. See my comment at https://github.com/python/cpython/pull/2827/files/693b9398b5fd202fa5864f9cc76fa1bc7f84f62e#r128971050.

pitrou · 2017-07-29T15:20:35Z

Serhiy, do you want to take over this PR? I am not much motivated by the remaining cleanup / dealing with existing cruft.

serhiy-storchaka · 2017-07-31T11:24:28Z

I take over this PR. Thank you Antoine, you have done hard work for which I did not dare to undertake. I have fetched the branch from your repository.

pitrou · 2017-07-31T16:52:52Z

Thank you Serhiy :-)

JimJJewett · 2017-08-18T23:17:15Z

Python/compile.c

-enum fblocktype { LOOP, EXCEPT, FINALLY_TRY, FINALLY_END };
+enum fblocktype { LOOP, EXCEPT, FINALLY_TRY, FINALLY_END, ASYNC_WITH,
+                  HANDLER_CLEANUP, WITH, NO_TYPE };
+


Nit: Is there a reason WITH doesn't come before ASYNC_WITH? Also, should HANDLER_CLEANUP be directly after the FINALLY_END?

nascheme · 2017-11-06T20:50:52Z

It would be quite good to get this change "to the finish line", so to speak. This is an ugly little corner of CPython and cleaning it up is difficult. That's why it has hung around all these years. This patch looks like it does the job, even though it is complicated. It seems to me that 95% of the hard work has been done by Antoine and Mark already.

I have rebased it on the current "master" branch, see nascheme:unwind_stack.

The rebase was not totally painless. I had to re-generate the importlib stuff (not surprising). In ceval, I had to make the following changes:

use exc_info->exc_* instead of tstate->exc_*
remove the block under fast_yield that calls restore_and_clear_exc_state() or swap_exc_state()

The unit test test_dist and test_importlib fail for me. It looks like test_dis hasn't been updated to match the latest changes made to the bytecode. Not sure what's wrong with importlib.

pitrou · 2017-12-28T19:43:00Z

Closing this PR as outdated. See PR #4682 and PR #5006 for more recent implementations.

bpo-17611: Move unwinding of stack from interpreter to compiler

656e4f3

pitrou added the type-feature A feature request or enhancement label Jul 23, 2017

the-knights-who-say-ni added the CLA signed label Jul 23, 2017

serhiy-storchaka self-requested a review July 23, 2017 19:27

Cleanup

c8068ab

pitrou added 4 commits July 23, 2017 22:15

Improve stack size calculation for SETUP_{EXCEPT,FINALLY}

b699364

Update frozen importlib

541e7ce

Remove useless code

d3dc002

Don't predict FOR_ITER as it breaks tracing without computed gotos

b6d597c

serhiy-storchaka reviewed Jul 23, 2017

View reviewed changes

pitrou added 4 commits July 23, 2017 23:22

Remove dead code and fix tracing tests

31e46dc

Fix test_dis

693b939

Add JUMP_FINALLY to avoid finally block duplication

d6ca2b9

Remove finally block duplication

aefdbd1

serhiy-storchaka reviewed Jul 24, 2017

View reviewed changes

pitrou added 9 commits July 24, 2017 12:16

More precise computation of code object stack size

9d12612

JUMP_FINALLY pushes 6 values to be consistent with the effect of an e…

4ce87b6

…xception

Exact stack size computation by popping stale exception state in RERAISE

029f825

Remove the now pointless POP_MANY

bd0167f

Add comment for RERAISE

e5c1db6

Remove JUMP_FINALLY

0ec9d12

Remove last block duplication

9c3cfd0

Get rid of END_ITER

9a420d1

Fix comments in frameobject.c

336808c

serhiy-storchaka reviewed Jul 25, 2017

View reviewed changes

Add stack size tests

61ee3fa

JimJJewett reviewed Aug 18, 2017

View reviewed changes

bedevere-bot added the awaiting core review label Aug 18, 2017

Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017

nascheme mentioned this pull request Dec 2, 2017

bpo-17611: Move unwinding of stack from interpreter to compiler #4682

Closed

pitrou closed this Dec 28, 2017

pitrou deleted the unwind_stack branch December 28, 2017 19:43

serhiy-storchaka mentioned this pull request Jan 1, 2018

bpo-24340: Fix estimation of the code stack size. #5076

Merged

[WIP] bpo-17611: Move unwinding of stack from interpreter to compiler #2827

[WIP] bpo-17611: Move unwinding of stack from interpreter to compiler #2827

Conversation

pitrou commented Jul 23, 2017 • edited by bedevere-bot Loading

mention-bot commented Jul 23, 2017

pitrou commented Jul 23, 2017

pitrou commented Jul 23, 2017

pitrou commented Jul 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou Jul 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitrou commented Jul 29, 2017

serhiy-storchaka commented Jul 31, 2017

pitrou commented Jul 31, 2017

Choose a reason for hiding this comment

nascheme commented Nov 6, 2017 • edited Loading

pitrou commented Dec 28, 2017

pitrou commented Jul 23, 2017 •

edited by bedevere-bot

Loading

pitrou Jul 29, 2017 •

edited

Loading

nascheme commented Nov 6, 2017 •

edited

Loading