Skip to content

Conversation

@Fidget-Spinner
Copy link
Member

@Fidget-Spinner Fidget-Spinner commented Dec 23, 2025

Fixes #143092

These super instructions need many special cases in the interpreter and JIT. It's best we convert them to normal instructions.

@markshannon
Copy link
Member

The changes to _CALL_LIST_APPEND look good, but I have concerns about the handling of _BINARY_OP_ADD_UNICODE

  • Because the current threshold for the JIT is quite high, the quadratic time for adding strings could still be observable in the interpreter.
  • The optimizer code is quite coupled to the front-end. The optimizer code should be quite narrow in scope so that it can handle changes in other parts of the JIT.

We can make BINARY_OP_INPLACE_ADD_UNICODE a normal instruction, without much change:
Instead of writing the result string to the local and skipping the following STORE_FAST, we could write NULL to the local and leave the result string on the stack for the STORE_FAST to store. It is a bit less efficient as we need to do some extra work, but it does the two things we need: gets rid of the superinstruction and avoids quadratic behavior.

Note: Adding the extra _POP_TOP isn't that cheap, as requires an extra push, pop, decref and instruction dispatch in the interpreter. It is really cheap in the JIT though as it is effectively free. It is absolutely worth it for the simplification and improved robustness, but there is some cost.

@Fidget-Spinner
Copy link
Member Author

We can make BINARY_OP_INPLACE_ADD_UNICODE a normal instruction, without much change: Instead of writing the result string to the local and skipping the following STORE_FAST, we could write NULL to the local and leave the result string on the stack for the STORE_FAST to store. It is a bit less efficient as we need to do some extra work, but it does the two things we need: gets rid of the superinstruction and avoids quadratic behavior.

This requires an oparg for both BINARY_OP (original) and STORE_FAST. How do you do this? Unless you want to pack bits into the oparg, which I'm not in favour of as we need more code to get it atomic on FT.

@Fidget-Spinner
Copy link
Member Author

Wait I think I misunderstood you, let me implement this.

@Fidget-Spinner Fidget-Spinner changed the title gh-143092: Make CALL_LIST_APPEND a normal instruction, disable superinstructions in the JIT gh-143092: Make CALL_LIST_APPEND and BINARY_OP_INPLACE_ADD_UNICODE normal instructions Dec 24, 2025
@Fidget-Spinner
Copy link
Member Author

@markshannon I converted both of them to normal instructions. Again, I highly doubt any of them will show up on benchmarks considering how few super instructions we have, and also IIRC the original superinstructions PR only added at most 1% speedup, and that contained LOAD_FAST_LOAD_FAST and friends as well.

Copy link
Member

@markshannon markshannon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that looks good. I like the way it simplifies both the JIT and the specializer.

BINARY_OP_INPLACE_ADD_UNICODE is a bit weird, but I think it is the best we can do given the constraints.

@Fidget-Spinner Fidget-Spinner added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Dec 24, 2025
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @Fidget-Spinner for commit 880b48d 🤖

Results will be shown at:

https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F143124%2Fmerge

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Dec 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

JIT: Segfault from invalid memory read in _PyTier2Interpreter

3 participants