GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-133231: Changes to executor management to support proposed `sys._jit` module by markshannon · Pull Request #133287 · python/cpython

Conversation

@markshannon

@markshannon markshannon commented May 2, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This PR:

  • Tracks the current executor on the thread-state, not the previous one
  • Sets the current executor to NULL when entering PyEval_EvalDefault and resetting it when leaving.
  • Batch executors for deallocation to avoid having to constantly incref executors; this is an ad-hoc form of deferred reference counting.

Should simplify the implementation of sys._jit.is_enabled() as it is simply tstate->current_executor != NULL and doesn't require additional state to be tracked.

  • Issue: Add JIT utilities in sys._jit #133231

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@markshannon
Track the current executor on the thread-state, not the previous one.…
3a2ccda
… Batch executors for deallocation to avoid having to constantly incref executors; this is an ad-hoc form of deferred reference counting.
@markshannon
markshannon requested a review from brandtbucher May 2, 2025 10:08
@markshannon
markshannon requested a review from ericsnowcurrently as a code owner May 2, 2025 10:08
@bedevere-app bedevere-app Bot mentioned this pull request May 2, 2025
Add JIT utilities in sys._jit #133231
Closed
@bedevere-app bedevere-app Bot added the awaiting core review label May 2, 2025
@markshannon
Handle tail-calling interpreter
46ef280
@markshannon markshannon added the skip news label May 2, 2025
@markshannon
Merge branch 'main' into current-executor
01ecf18
@markshannon

markshannon commented May 2, 2025

Copy link
Copy Markdown
Member Author

The msvc jit failure might be fixed by #132746

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon
Merge branch 'main' into current-executor
89e4668
@Fidget-Spinner

Fidget-Spinner commented May 2, 2025

Copy link
Copy Markdown
Member

The msvc jit failure might be fixed by #132746

Wow I did not expect that to fix it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

brandtbucher
brandtbucher requested changes May 3, 2025
View reviewed changes

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think the overall approach is neat, but I think there are a couple of potential issues (and possible cleanups) here.

Long story short, anything that uses the entry_frame for this purpose is going to run into issues with the tail-calling interpreter (whether it's your approach, or mine). It's a hard problem.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Python/ceval.c
Comment on lines +993 to +996
typedef struct {
_PyInterpreterFrame frame;
_PyStackRef stack[1];
} _PyEntryFrame;

@brandtbucher brandtbucher May 3, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Is this needed? We only need one slot in localsplus, which _PyInterpreterFrame already has.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@markshannon markshannon May 3, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We now need two slots. One on the stack for the return value, as before, and one local variable to hold the current executor.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@brandtbucher brandtbucher May 3, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Ah, I see. Forgot about the return value.

Maybe add a comment that the one stack slot is for the return value, and the one local is for the executor.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/optimizer.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/optimizer.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/bytecodes.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added awaiting changes and removed awaiting core review labels May 3, 2025
@bedevere-app

bedevere-app Bot commented May 3, 2025

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@brandtbucher

brandtbucher commented May 3, 2025

Copy link
Copy Markdown
Member

Idead: if we have a struct struct { PyExecutorObject *executor, struct node *prev} node;, we can stack allocate one of these in every GOTO_TIER_TWO call and restore it afterwards. Then keep the top on the tstate.

Something like this:

diff --git a/Python/ceval_macros.h b/Python/ceval_macros.h
index e1d2673848c..c77c2cafee7 100644
--- a/Python/ceval_macros.h
+++ b/Python/ceval_macros.h
@@ -360,10 +360,13 @@ do {                                                   \
     OPT_STAT_INC(traces_executed);                     \
     _PyExecutorObject *_executor = (EXECUTOR);         \
     jit_func jitted = _executor->jit_code;             \
+    node prev = tstate->current_executor;              \
+    tstate->current_executor = (node){_executor, &prev}; \
     /* Keep the shim frame alive via the executor: */  \
     Py_INCREF(_executor);                              \
     next_instr = jitted(frame, stack_pointer, tstate); \
     Py_DECREF(_executor);                              \
+    tstate->current_executor = prev;                   \
     Py_CLEAR(tstate->previous_executor);               \
     frame = tstate->current_frame;                     \
     stack_pointer = _PyFrame_GetStackPointer(frame);   \
@@ -379,6 +382,9 @@ do { \
     OPT_STAT_INC(traces_executed); \
     next_uop = (EXECUTOR)->trace; \
     assert(next_uop->opcode == _START_EXECUTOR); \
+    /* prev is declared local to the tier two interpreter: */ \
+    prev = tstate->current_executor; \
+    tstate->current_executor = (node){_executor, &prev}; \
     goto enter_tier_two; \
 } while (0)
 #endif
@@ -386,6 +392,7 @@ do { \
 #define GOTO_TIER_ONE(TARGET)                                         \
     do                                                                \
     {                                                                 \
+        tstate->current_executor = prev;                              \
         next_instr = (TARGET);                                        \
         OPT_HIST(trace_uop_execution_counter, trace_run_length_hist); \
         _PyFrame_SetStackPointer(frame, stack_pointer);               \

No entry frame needed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon
Don't use entry.frame in tailcalling interpreter
2f42cb0
@markshannon

markshannon commented May 3, 2025

Copy link
Copy Markdown
Member Author

I don't see how that would work, as macros don't have scope. But its moot anyway as we can use the entry frame.

The entry frame exists in the tail-calling interpreter, it is the entry variable that we can't use. But we don't need to. Anywhere we return, either in INTERPRETER_EXIT or exit_unwind, the frame is the entry frame: frame == &entry.frame.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

markshannon added 2 commits May 3, 2025 09:57
@markshannon
Address review comments
f938d68
@markshannon
Fix compiler warning
a30e444
@markshannon
markshannon requested a review from savannahostrowski as a code owner May 3, 2025 09:06
@markshannon

markshannon commented May 3, 2025

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app bedevere-app Bot added awaiting change review and removed awaiting changes labels May 3, 2025
@bedevere-app

bedevere-app Bot commented May 3, 2025

Copy link
Copy Markdown

Thanks for making the requested changes!

@brandtbucher: please review the changes made to this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-app
bedevere-app Bot requested a review from brandtbucher May 3, 2025 10:53
@brandtbucher

brandtbucher commented May 3, 2025

Copy link
Copy Markdown
Member

I don't see how that would work, as macros don't have scope.

I mean, it would just be a local declared in the _PyEval_EvalFrameDefault itself for the tail-calling interpreter, and would become local to whatever bytecode is using the macro on other builds.

But as you say...

But its moot anyway as we can use the entry frame.

The entry frame exists in the tail-calling interpreter, it is the entry variable that we can't use. But we don't need to. Anywhere we return, either in INTERPRETER_EXIT or exit_unwind, the frame is the entry frame: frame == &entry.frame.

Ah, this is what I missed. Makes sense: we're always able to set it when entering because we have entry handy. And we're always able to reset it when exiting because it's just the current frame. Thanks for explaining.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

brandtbucher
brandtbucher approved these changes May 3, 2025
View reviewed changes

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Looks good, thanks. I'm just going to kick off some JIT benchmarks to be safe.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Python/ceval_macros.h
{ \
tstate->current_executor = NULL; \
next_instr = (TARGET); \
assert(tstate->current_executor == NULL); \

@brandtbucher brandtbucher May 3, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Minor: this seems redundant, given that we're setting it one line above.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Tools/jit/template.c
#undef GOTO_TIER_ONE
#define GOTO_TIER_ONE(TARGET) \
do { \
tstate->current_executor = NULL; \

@brandtbucher brandtbucher May 3, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You can either do this here, or in the second half of the _Py_JIT implementation of GOTO_TIER_TWO in ceval_macros.h, after the JIT code returns. No need to do it in both places.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

markshannon reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Python/bytecodes.c
@@ -5316,7 +5322,6 @@ dummy_func(
}

tier2 op(_START_EXECUTOR, (executor/4 --)) {

@brandtbucher brandtbucher May 3, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Side note, this instruction now exists solely to set the current_executor in the tier two interpreter. We should try to remove it as a follow-up (doesn't need to be in this PR).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@markshannon markshannon May 4, 2025

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

We can replace all uses of current_executor with an operand, which will remove the current_executor local variable from the tier 2 interpreter and the _JIT_EXECUTOR patch from the JIT template.

We can also shrink _CHECK_VALIDITY a bit by replacing current_executor->vm_data.valid with *validity_ptr:

tier2 op(_CHECK_VALIDITY, (validity_ptr/4 --)) {
    DEOPT_IF(*validity_ptr == 0);
}

For another PR.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting change review labels May 3, 2025
@brandtbucher

brandtbucher commented May 4, 2025

Copy link
Copy Markdown
Member

Looks fine.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon
Remove redundant code
0b9dcb7
@markshannon
markshannon merged commit ac7d5ba into python:main May 4, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label May 4, 2025
diegorusso added a commit to diegorusso/cpython that referenced this pull request May 4, 2025
@diegorusso
Merge remote-tracking branch 'origin/main' into call-len
c03babc
* origin/main: (111 commits)
  pythongh-91048: Add filename and line number to external inspection routines (pythonGH-133385)
  pythongh-131178: Add tests for `ast` command-line interface (python#133329)
  Regenerate pcbuild.sln in Visual Studio 2022 (python#133394)
  pythongh-133042: disable HACL* HMAC on Emscripten (python#133064)
  pythongh-133351: Fix remote PDB's multi-line block tab completion (python#133387)
  pythongh-109700: Improve stress tests for interpreter creation (pythonGH-109946)
  pythongh-81793: Skip tests for os.link() to symlink on Android (pythonGH-133388)
  pythongh-126835: Rename `ast_opt.c` to `ast_preprocess.c` and related stuff after moving const folding to the peephole optimizier (python#131830)
  pythongh-91048: Relax test_async_global_awaited_by to fix flakyness (python#133368)
  pythongh-132457: make staticmethod and classmethod generic (python#132460)
  pythongh-132805: annotationlib: Fix handling of non-constant values in FORWARDREF (python#132812)
  pythongh-132426: Add get_annotate_from_class_namespace replacing get_annotate_function (python#132490)
  pythongh-81793: Always call linkat() from os.link(), if available (pythonGH-132517)
  pythongh-122559: Synchronize C and Python implementation of the io module about pickling (pythonGH-122628)
  pythongh-69605: Add PyREPL import autocomplete feature to 'What's New' (python#133358)
  bpo-44172: Keep reference to original window in curses subwindow objects (pythonGH-26226)
  pythonGH-133231: Changes to executor management to support proposed `sys._jit` module (pythonGH-133287)
  pythongh-133363: Fix Cmd completion for lines beginning with `! ` (python#133364)
  pythongh-132983: Introduce `_zstd` bindings module (pythonGH-133027)
  pythonGH-91048: Add utils for printing the call stack for asyncio tasks (python#133284)
  ...
@markshannon
markshannon deleted the current-executor branch May 5, 2025 11:53
Pranjal095 pushed a commit to Pranjal095/cpython that referenced this pull request Jul 12, 2025
@markshannon @Pranjal095
pythonGH-133231: Changes to executor management to support proposed `…
33499e6
…sys._jit` module (pythonGH-133287)

* Track the current executor, not the previous one, on the thread-state. 

* Batch executors for deallocation to avoid having to constantly incref executors; this is an ad-hoc form of deferred reference counting.
@Zheaoli Zheaoli mentioned this pull request Oct 4, 2025
JIT executors are not properly freed #139540
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brandtbucher brandtbucher brandtbucher approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@savannahostrowski savannahostrowski Awaiting requested review from savannahostrowski

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@markshannon @Fidget-Spinner @brandtbucher
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.