Skip to content

GH-137959: Replace shim code in jitted code with a single trampoline function. #137961

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
Merged
merged 7 commits into from
Aug 21, 2025
Merged

GH-137959: Replace shim code in jitted code with a single trampoline function. #137961

merged 7 commits into from
Aug 21, 2025

Conversation

markshannon
Copy link
Member

@markshannon markshannon commented Aug 19, 2025 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Aug 19, 2025
Open
@markshannon markshannon marked this pull request as ready for review August 19, 2025 16:24
@markshannon
Copy link
Member Author

Benchmark results show a hint of a speedup, but in the noise. No slowdown, though.

Fidget-Spinner
Fidget-Spinner approved these changes Aug 20, 2025
View reviewed changes
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

Some things I observed. Correct me if I'm wrong:

  1. This actually splits out the tier 2 interpreter from the tier 1 interpreter. Which is likely a significant saving for our tier 2 debug build time and tier 2 interpreter. Nice!
  2. The shim/trampoline is only compiled once lazily and reused across the whole runtime. Then reused across all JIT functions. We currently do two jumps (one into the shim/trampoline, one into the exec->jit_code). You need to lock this with a mutex in case multiple threads try to compile the shim at the same time.

If my understanding above is correct, then this is alright to me.

@markshannon
Copy link
Member Author

Some things I observed. Correct me if I'm wrong:

  1. This actually splits out the tier 2 interpreter from the tier 1 interpreter. Which is likely a significant saving for our tier 2 debug build time and tier 2 interpreter. Nice!
  2. The shim/trampoline is only compiled once lazily and reused across the whole runtime. Then reused across all JIT functions. We currently do two jumps (one into the shim/trampoline, one into the exec->jit_code). You need to lock this with a mutex in case multiple threads try to compile the shim at the same time.

If my understanding above is correct, then this is alright to me.

That is all correct. I'll add a comment explaining it.

@markshannon markshannon merged commit a8d9d94 into python:main Aug 21, 2025
73 checks passed
@brandtbucher
Copy link
Member

Nice, sorry I wasn't able to review in time, but it looks mostly good (love the unification of the two JIT execution engines).

Two notes:

  • I prefer the old name "shim" throughout to the new name "trampoline". Trampoline already has another meaning in the JIT compiler, and we renamed this to "shim" a while back to make the distinction clearer.
  • I don't love that we leak the shim code when the runtime is finalized. We should free it in Py_Finalize or whatever.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes

@brandtbucher brandtbucher Awaiting requested review from brandtbucher brandtbucher is a code owner

@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

@ZeroIntensity ZeroIntensity Awaiting requested review from ZeroIntensity ZeroIntensity is a code owner

@savannahostrowski savannahostrowski Awaiting requested review from savannahostrowski savannahostrowski is a code owner

@diegorusso diegorusso Awaiting requested review from diegorusso diegorusso is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@markshannon @brandtbucher @Fidget-Spinner