Skip to content

broken optimization in gen_close() #111107

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.

Sign up for GitHub

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

Jump to bottom
Closed
iritkatriel opened this issue Oct 20, 2023 · 4 comments
Closed

broken optimization in gen_close() #111107

iritkatriel opened this issue Oct 20, 2023 · 4 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)

Comments

@iritkatriel
Copy link
Member

iritkatriel commented Oct 20, 2023 •
edited
Loading

There is an optimization in gen_close which attempts to shortcut the case where a sub-generator is being closed, and it is wrapped in only one try block (which is presumed to be for the StopIteration so can be ignored).

However, there is a bug there where the exception depth is taken from frame->prev_instr[0].op.code, where it should be frame->prev_instr[0].op.arg.

At the moment this optimization is not doing anything because exception_handler_depth is never 1, it is either 118 (opcode of YIELD_VALUE) or 241 (opcode of INSTRUMENTED_YIELD_VALUE). If one of those opcodes will happen to be mapped to 1, then this code will have some impact but obviously not the intended one.

CC @markshannon
@iritkatriel iritkatriel mentioned this issue Oct 20, 2023
Merged
@iritkatriel
Copy link
Member Author

iritkatriel commented Oct 20, 2023 •
edited
Loading

If I try to fix the typo and use op.arg instead of op.code, some tests fail: #111069

I think the return value is incorrect in some cases.

@iritkatriel
Copy link
Member Author

iritkatriel commented Oct 20, 2023 •
edited
Loading

The failure in testcontextlib is fixed by setting the frame state in case we return early:

         if (exception_handler_depth == 1) {
+            gen->gi_frame_state = FRAME_COMPLETED;
             Py_RETURN_NONE;
         }

@Eclips4
Copy link
Member

Eclips4 commented Oct 20, 2023

Probably related: #111058

@Eclips4 Eclips4 added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Oct 20, 2023
@iritkatriel
Copy link
Member Author

Perf measurement for #111069:

https://github.com/faster-cpython/benchmarking/blob/main/results/bm-20231020-3.13.0a1%2B-90d6409/bm-20231020-linux-x86_64-iritkatriel-refleak-3.13.0a1%2B-90d6409-vs-base.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@iritkatriel @Eclips4