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-133171: Make the executor_list thread-safe by aisk · Pull Request #140038 · python/cpython

Conversation

@aisk

@aisk aisk commented Oct 13, 2025 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

The idea is simple and naive: just add a PyMutex to the PyInterpreterState for the executor_list under the nogil build, and lock or unlock the mutex in all functions that operate on the executor_list.

Since I'm not very familiar with the related code, there might be a better way to do this, so any feedback would be appreciated!

  • Issue: Make the JIT thread-safe #133171

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
yihong0618 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@aisk
Add lock with operations on _PyExecutorObject list
e178e42
@aisk
aisk requested review from ZeroIntensity, ericsnowcurrently and markshannon as code owners October 13, 2025 12:28
@bedevere-app bedevere-app Bot added the awaiting review label Oct 13, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Oct 13, 2025
Make the JIT thread-safe #133171
Open
1 of 7 tasks
@aisk aisk added the skip news label Oct 13, 2025
@aisk aisk changed the title gh-133171: Add lock on operations to executor_list gh-133171: Make the executor_list thread-safe Oct 13, 2025
ZeroIntensity
ZeroIntensity reviewed Oct 13, 2025
View reviewed changes
Comment thread Include/internal/pycore_interp_structs.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
@aisk
Using FT_MUTEX_LOCK macro
f3a9c74
ZeroIntensity
ZeroIntensity reviewed Oct 14, 2025
View reviewed changes

@ZeroIntensity ZeroIntensity 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'm not entirely sure that a mutex is safe here, because we're locking code paths that call Py_DECREF. @Fidget-Spinner, can these paths escape?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Include/internal/pycore_interp_structs.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
@Fidget-Spinner

Fidget-Spinner commented Oct 14, 2025

Copy link
Copy Markdown
Member

In theory, Py_DECREF(executor) could clear the (future) constants pool that we plan to add, which would hold arbitrary constant objects, which can escape.
All reactions

Sorry, something went wrong.

Uh oh!

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

corona10 commented Oct 15, 2025 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

I am not sure if making executor_list operations thread-safe at this point is good idea.
There is no way to verify that we make the thread-safe until we actually integrate the whole pipeline. I do believe that we have to do more things before we start to work on this task.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fidget-Spinner

Fidget-Spinner commented Nov 7, 2025

Copy link
Copy Markdown
Member

@corona10 we need the executor list to be thread safe for it to work with the plan here #133171 (comment).

@aisk could you resume the PR please?
corona10 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Fidget-Spinner
Fidget-Spinner reviewed Nov 7, 2025
View reviewed changes
Comment thread Python/optimizer.c
void
_Py_Executors_InvalidateAll(PyInterpreterState *interp, int is_invalidation)
{
EXECUTOR_LIST_LOCK(interp);

@Fidget-Spinner Fidget-Spinner Nov 7, 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.

This should be a stop the world event.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/optimizer.c
}
/* Clearing an executor can deallocate others, so we need to make a list of
* executors to invalidate first */
EXECUTOR_LIST_LOCK(interp);

@Fidget-Spinner Fidget-Spinner Nov 7, 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.

This is fine. nice.!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Python/optimizer.c Resolved

Uh oh!

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

corona10 commented Nov 7, 2025

Copy link
Copy Markdown
Member

I will try to re review this PR in this weekend.
aisk reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

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

corona10 commented Nov 7, 2025

Copy link
Copy Markdown
Member

Share the DM context between @Fidget-Spinner and I.

Before merging this PR, we should investigate the current blocker #133171 (comment) to verify this PR in practice.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aisk added 3 commits November 9, 2025 15:43
@aisk
Merge remote-tracking branch 'upstream/main' into executor-list-lock
283f453
@aisk
Free the executors outside the lock in _Py_ClearExecutorDeletionList
6858161
@aisk
Fix a potential dead lock in add_to_pending_deletion_list
fa0dd0b
@aisk

aisk commented Nov 9, 2025

Copy link
Copy Markdown
Member Author

Hi @ZeroIntensity @corona10 @Fidget-Spinner , thanks for the review and feedback. I'll bring this PR up to date while waiting for #133171 (comment) to be resolved.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
aisk added 2 commits November 9, 2025 23:04
@aisk
Using a _PyRecursiveMutex for executors lock
bf48bb4
@aisk
Fix refcount
6a5a1a9
Fidget-Spinner
Fidget-Spinner reviewed Nov 9, 2025
View reviewed changes
Comment thread Python/optimizer.c
Comment on lines +303 to +307
/* Release the lock before calling _Py_ClearExecutorDeletionList
* to avoid deadlock, since it also tries to acquire the same lock */
EXECUTOR_LIST_UNLOCK(interp);
_Py_ClearExecutorDeletionList(interp);
EXECUTOR_LIST_LOCK(interp);

@Fidget-Spinner Fidget-Spinner Nov 9, 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.

Do we still need this?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@Fidget-Spinner

Fidget-Spinner commented Nov 10, 2025

Copy link
Copy Markdown
Member

Sorry, now that I think about it, locking isn't the right choice here. We should instead move the executor list to tstate.

Really sorry for taking up your time!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Fidget-Spinner Fidget-Spinner closed this Nov 10, 2025
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 left review comments
@ZeroIntensity ZeroIntensity ZeroIntensity left review comments
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

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

Assignees

No one assigned

Labels

awaiting review 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.

4 participants

@aisk @Fidget-Spinner @corona10 @ZeroIntensity
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.