◐ Shell
reader mode source ↗
Skip to content

bpo-40512: Store pointer to interpreter state in a thread local variable#29228

Closed
markshannon wants to merge 11 commits into
python:mainfrom
faster-cpython:thread-local-interpreter-state
Closed

bpo-40512: Store pointer to interpreter state in a thread local variable#29228
markshannon wants to merge 11 commits into
python:mainfrom
faster-cpython:thread-local-interpreter-state

Conversation

@markshannon

@markshannon markshannon commented Oct 26, 2021 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

A more future proof way to handle interpreter state. 1% faster as well.

https://bugs.python.org/issue40512

@markshannon markshannon force-pushed the thread-local-interpreter-state branch from 1be2ad9 to ada883d Compare October 26, 2021 15:49
@markshannon markshannon added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 26, 2021
@bedevere-bot

bedevere-bot commented Oct 26, 2021

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @markshannon for commit ada883d6b97bb072ef8465b71671ef9db101c7ae 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the Test PR w/ buildbots; report in status section label Oct 26, 2021
@markshannon

markshannon commented Oct 27, 2021

Copy link
Copy Markdown
Member Author

Build failures look like the usual suspects.

@markshannon markshannon force-pushed the thread-local-interpreter-state branch from ada883d to f22036b Compare October 27, 2021 15:06
pablogsal
pablogsal reviewed Oct 27, 2021
View reviewed changes

@pablogsal pablogsal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

This looks goot to me in general but it would be good if @vstinner could take a look since he has been modifying these APIs recently a lot.

@pablogsal pablogsal mentioned this pull request Oct 27, 2021
Merged
@markshannon markshannon requested a review from vstinner October 29, 2021 11:45
@markshannon

markshannon commented Oct 29, 2021

Copy link
Copy Markdown
Member Author

@vstinner Any comments?

@tiran

tiran commented Oct 29, 2021

Copy link
Copy Markdown
Member

The old approach had a NULL check and Py_FatalError. The check is useful for embedders. They get a sensible error message when they forget to create a thread state for an external thread. Would it be possible to retain the check for debug builds?

@vstinner

vstinner commented Nov 19, 2021

Copy link
Copy Markdown
Member

@vstinner Any comments?

Sorry, I don't have the bandwidth to review this change :-(

@ericsnowcurrently ericsnowcurrently self-requested a review December 6, 2021 18:09
@vstinner vstinner mentioned this pull request Apr 10, 2022
Closed
@rhettinger rhettinger removed their request for review May 3, 2022 06:19
@ericsnowcurrently ericsnowcurrently mentioned this pull request Jul 19, 2022
Closed
@markshannon

markshannon commented Oct 25, 2022

Copy link
Copy Markdown
Member Author

I'm dropping this as there are a few reasons why passing the interpreter state explicitly make sense.

  • Accessing thread locals for DLL can be slow
  • Passing the interpreter state explicitly make migrating to a new API easier
  • We can pass around different aspects of the interpreter, e.g. the GIL, clarifying what parts of the system code is allowed to access.

@markshannon markshannon deleted the thread-local-interpreter-state branch September 26, 2023 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently left review comments

@pablogsal pablogsal

@erlend-aasland erlend-aasland erlend-aasland left review comments

@isidentical isidentical Awaiting requested review from isidentical

@vstinner vstinner Awaiting requested review from vstinner

Assignees

@ericsnowcurrently ericsnowcurrently

Labels

awaiting core review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@markshannon @bedevere-bot @tiran @vstinner @ericsnowcurrently @pablogsal @erlend-aasland @the-knights-who-say-ni @ezio-melotti