bpo-33387: Simplify bytecodes for try-finally, try-except and with blocks. #6641
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
For the record, here is the result of pyperformance. |
markshannon
force-pushed
the
python-better-try-finally
branch
from
0dfe44c to
d8b54c9
Compare
|
@markshannon FYI, |
markshannon
force-pushed
the
python-better-try-finally
branch
from
d8b54c9 to
3efb201
Compare
DinoV
reviewed
Sep 13, 2018
DinoV
reviewed
Sep 13, 2018
DinoV
reviewed
Sep 13, 2018
DinoV
reviewed
Sep 14, 2018
DinoV
reviewed
Sep 14, 2018
DinoV
reviewed
Sep 14, 2018
DinoV
reviewed
Sep 14, 2018
DinoV
reviewed
Sep 14, 2018
DinoV
reviewed
Sep 14, 2018
DinoV
reviewed
Sep 14, 2018
DinoV
reviewed
Sep 14, 2018
DinoV
reviewed
Sep 14, 2018
markshannon
force-pushed
the
python-better-try-finally
branch
from
3efb201 to
b16b8e8
Compare
|
All comments addressed and rebased to fix merge conflict with ddd1949 |
markshannon
force-pushed
the
python-better-try-finally
branch
from
b16b8e8 to
d64244b
Compare
|
Rebased again |
markshannon
force-pushed
the
python-better-try-finally
branch
from
d64244b to
6f916e9
Compare
|
Rebased yet again. |
markshannon
force-pushed
the
python-better-try-finally
branch
2 times, most recently
from
b9c698e to
cdb2581
Compare
|
Rebased yet again (twice) |
brettcannon
reviewed
Jun 7, 2019
Doc/library/dis.rst
Outdated
|
|
||
| Calls the function in position 7 on the stack with the top three | ||
| items on the stack as arguments. | ||
| Used to implement the call ``exit(*exc_info())`` when an exception |
There was a problem hiding this comment.
This was initially misleading to me as my brain thought of sys.exit() instead of context_manager.__exit__().
Suggested change
| Used to implement the call ``exit(*exc_info())`` when an exception | |
| Used to implement the call ``context_manager.__exit__(*exc_info())`` when an exception |
| } codetracker; | ||
|
|
||
| static void | ||
| reset(codetracker *tracker) |
There was a problem hiding this comment.
Perhaps a comment as to why some fields don't require resetting while others do? And maybe why the fields in init_codetracker() differ from those in reset()? Basically my brain is wondering why e.g. start_line is initialized but not reset, and vice-versa.
There's also a naming discrepancy between reset() and init_codetracker() in that one includes codetracker and the other does not.
Objects/frameobject.c
Outdated
| } | ||
| while (tracker->offset < tracker->lnotab_len && | ||
| tracker->addr >= tracker->line_addr + tracker->lnotab[tracker->offset]) { | ||
| tracker->line_addr = tracker->line_addr + tracker->lnotab[tracker->offset]; |
There was a problem hiding this comment.
Suggested change
| tracker->line_addr = tracker->line_addr + tracker->lnotab[tracker->offset]; | |
| tracker->line_addr += tracker->lnotab[tracker->offset]; |
| return op == SETUP_FINALLY && !is_try_except(op, target_op) && !is_async_for(op, target_op); | ||
| } | ||
|
|
||
| #define TRY_EXCEPT 250 |
There was a problem hiding this comment.
I would expect to find this value in opcode.h but I'm not, so why the new definition of an opcode here? (A comment would be enough for me to clarify why this is here.)
Objects/frameobject.c
Outdated
| } | ||
| else if (is_try_finally(op, target_op)) { | ||
| int addr = tracker->addr; | ||
| // Skip over duplicate finally blocks if line is after body. |
There was a problem hiding this comment.
Suggested change
| // Skip over duplicate finally blocks if line is after body. | |
| // Skip over duplicate 'finally' blocks if line is after body. |
Objects/frameobject.c
Outdated
| PyErr_SetString(PyExc_ValueError, | ||
| "can't jump into the middle of a block"); | ||
| return -1; | ||
| /* Validate change of block stack */ |
There was a problem hiding this comment.
Suggested change
| /* Validate change of block stack */ | |
| /* Validate change of block stack. */ |
Objects/frameobject.c
Outdated
| } | ||
| break; | ||
| } | ||
| /* Check for illegal jumps out of finally or except blocks */ |
There was a problem hiding this comment.
Suggested change
| /* Check for illegal jumps out of finally or except blocks */ | |
| /* Check for illegal jumps out of finally or except blocks. */ |
Objects/frameobject.c
Outdated
| { | ||
| /* Pop the exit function. */ | ||
| delta++; | ||
| /* Unwind block stack */ |
There was a problem hiding this comment.
Suggested change
| /* Unwind block stack */ | |
| /* Unwind block stack. */ |
Python/compile.c
Outdated
| @@ -2587,7 +2620,7 @@ compiler_for(struct compiler *c, stmt_ty s) | |||
| if (start == NULL || end == NULL || cleanup == NULL) | |||
| return 0; | |||
|
|
|||
| if (!compiler_push_fblock(c, FOR_LOOP, start, end)) | |||
| if (!compiler_push_fblock(c, FOR_LOOP, start, end, NULL)) | |||
There was a problem hiding this comment.
If you're up for it, would you mind fixing these if blocks to use {}? Otherwise the style is shifting and I would rather promote the new style than the old one.
Python/compile.c
Outdated
| compiler_use_next_block(c, finally); | ||
| if (!compiler_push_fblock(c, FINALLY_END, finally, NULL)) | ||
|
|
||
| /* End of body; start the cleanup */ |
There was a problem hiding this comment.
Suggested change
| /* End of body; start the cleanup */ | |
| /* End of body; start the cleanup. */ |
|
All review comments addressed. |
|
FYI no need to wait on me on an approving review. |
markshannon
force-pushed
the
python-better-try-finally
branch
from
db08c6d to
c611d1b
Compare
|
Rebased yet again... Time to press that merge button ⬇️ 🙂 |
|
@markshannon is there anything preventing you from doing the merge at this point? |
markshannon
force-pushed
the
python-better-try-finally
branch
from
c611d1b to
1bd40b8
Compare
|
@brettcannon No technical obstacles, but I think that for large PRs like this one, it is best if someone other than the author does the merge. |
|
@markshannon Are you choosing to skip my doc change suggestions? (I'm asking because you said all the comments were addressed but those were left open/unresolved, so I just wanted to check the PR is in its final state sans merge conflict.) As for clicking "approved" on a review, that's as close as people typically get for approval on a core dev's PR. Historically core devs have done their own merges as they will be the ones to handle reversions, etc. IOW if you want to address my remaining doc comments and are willing to handle reversion duties, etc., then I can click the merge button for you. |
markshannon
force-pushed
the
python-better-try-finally
branch
from
1bd40b8 to
385b2df
Compare
markshannon
force-pushed
the
python-better-try-finally
branch
2 times, most recently
from
54f381c to
23cfe89
Compare
markshannon
force-pushed
the
python-better-try-finally
branch
from
23cfe89 to
62a3625
Compare
markshannon
force-pushed
the
python-better-try-finally
branch
from
62a3625 to
8457a00
Compare
…parate code for normal and exceptional paths. Remove BEGIN_FINALLY, END_FINALLY, CALL_FINALLY and POP_FINALLY bytecodes. Implement finally blocks by code duplication. Reimplement frame.lineno setter using line numbers rather than bytecode offsets.
markshannon
force-pushed
the
python-better-try-finally
branch
from
8457a00 to
6364ee7
Compare
|
I've been holding off on this as I wanted to let the 3.8 release stabilize before changing the bytecode. @brettcannon I have addressed all your comments. |
|
@markshannon: Please replace |
jacobneiltaylor
pushed a commit
to jacobneiltaylor/cpython
that referenced
this pull request
Dec 5, 2019
…parate code for normal and exceptional paths. (python#6641) Remove BEGIN_FINALLY, END_FINALLY, CALL_FINALLY and POP_FINALLY bytecodes. Implement finally blocks by code duplication. Reimplement frame.lineno setter using line numbers rather than bytecode offsets.
shihai1991
pushed a commit
to shihai1991/cpython
that referenced
this pull request
Jan 31, 2020
…parate code for normal and exceptional paths. (python#6641) Remove BEGIN_FINALLY, END_FINALLY, CALL_FINALLY and POP_FINALLY bytecodes. Implement finally blocks by code duplication. Reimplement frame.lineno setter using line numbers rather than bytecode offsets.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Quoting the bpo issue:
The implementation of
withstatements is a bit more complex. The with statementis implemented as:
https://bugs.python.org/issue33387