GeezerVison

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

I don't think the current code is "wrong" functionality wise, as deopt is safe for base instructions - you can do multiple deopts and it'll just be the same base opcode. However, it did deopt twice for the opcode, so I moved the deopt into the ENTER_EXECUTOR check. Notice that if the opted opcode is ENTER_EXECUTOR, it would still be ENTER_EXECUTOR after deopted.

I'm not sure if ENTER_EXECUTOR will be instrumented by instruction instrumentation (I would guess so), if it will, then we have to deopt the instruction before ENTER_EXECUTOR check, or it won't recognize it.

The problem is that there are cases where i++ doesn't give you a bytecode but a cache :-(

I'm thinking at bug gh-107082 which has been fixed by commit 233b878. cc @gvanrossum

I dislike this approach, iterating on each bytecode at a specific index, deoptimize it, and then look for the next bytecode. The problem is that there are cases where i++ doesn't give you a bytecode but a cache :-( You have to take care of ENTER_EXECUTOR. The exact implementation of bytecode changes often these days, and it's hard to keep all functions consuming bytecode to remain correct.

Would it be possible instead to write a function which creates a copy of the deoptimized code in one shot? Something like PyCode_GetOriginalBytecode() which would create a bytes object.

I'm not saying this is the best way to do it (it was there already to compare the code objects), but the logic here is correct (at least for now). i is specifically taken care of to avoid getting cache instructions in the loop.

A function like PyCode_GetOriginalBytecode() would just put this logic into another abstracted function right? We still need to keep track of everything when instructions changed. It just brings the problem to another person/piece of code.

A function like PyCode_GetOriginalBytecode() would just put this logic into another abstracted function right? We still need to keep track of everything when instructions changed. It just brings the problem to another person/piece of code.

Correct, but it's easier to implement when you iterate on a single code object, and this function can be reused in other places, and it can be moved closer to functions which modify bytecode.

Also I believe this piece of code was originally written to avoid allocating a new piece of memory. A new function returning bytes would contradict that - not saying that's the wrong way to go, just to mention.

Also I believe this piece of code was originally written to avoid allocating a new piece of memory. A new function returning bytes would contradict that - not saying that's the wrong way to go, just to mention.

Honestly, I don't think that code1==code2 operation matters for Python performance. It's usually used in the compiler/parser, not "at runtime". It shouldn't be part of "hot code".

By the way, for me, it's surprising that _Py_GetBaseOpcode() can still return ENTER_EXECUTOR and require to get the executor (opcode, oparg). Each caller has to take care of that. Would it be possible to have a function which returns the original base case, so don't return ENTER_EXECUTOR?

Maybe an "iterator-like" API which also gives an offset to the next "original" instruction (the next instruction which, one decoded, will give the original bytecode)?

By the way, for me, it's surprising that _Py_GetBaseOpcode() can still return ENTER_EXECUTOR and require to get the executor (opcode, oparg). Each caller has to take care of that. Would it be possible to have a function which returns the original base case, so don't return ENTER_EXECUTOR?

Maybe an "iterator-like" API which also gives an offset to the next "original" instruction (the next instruction which, one decoded, will give the original bytecode)?

I'm just saying, this code is explicitly converted to the current status from what you wanted in bd2e47c . _PyCode_GetCode is the utility function you described and it was used before the commit :)

Do you think we should revert that change? I would guess there might be reasons behind it.

Honestly, I don't think that code1==code2 operation matters for Python performance. It's usually used in the compiler/parser, not "at runtime". It shouldn't be part of "hot code"

I don't think that copying the original object is not beneficial for the scenario as you commented.
Every time we have to optimize a code object, do we have to copy the original object? I think that comparing the code object is more rare case than optimizing itself.

I prefer to abstraint myself from reviewing this PR :-) I didn't follow recent developments about optimization, so I don't have a strong opinion. I just shared my opinion and feedback on the recent code changes and issues that I saw ;-)

I confirm that this change fix issue #109052 crash.

Without this change, Python does crash with the following command:

$ ./python -m test test_sys_settrace -R 3:3
(...)
...Fatal Python error: Segmentation fault

Current thread 0x00007f9dce557740 (most recent call first):
  File "/home/vstinner/python/main/Lib/test/test_sys_settrace.py", line 1912 in trace
  File "/home/vstinner/python/main/Lib/test/test_sys_settrace.py", line 2162 in test_jump_backwards_into_while_block
  File "/home/vstinner/python/main/Lib/test/test_sys_settrace.py", line 1975 in run_test
  File "/home/vstinner/python/main/Lib/test/test_sys_settrace.py", line 2008 in test
(...)

With this change, the test does pass as expected, and Python doesn't crash.

$ ./python -m test test_sys_settrace -R 3:3
(...)
Result: SUCCESS

I'll merge in a couple of hours if nobody has any other concerns.

Let's just merge!

GH-112329 is a backport of this pull request to the 3.12 branch.