Skip to content
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

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

Closed
wants to merge 11 commits into from
Closed

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

wants to merge 11 commits into from

Conversation

markshannon
Copy link
Member

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

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
Copy link

🤖 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-with-buildbots Test PR w/ buildbots; report in status section label Oct 26, 2021
@markshannon
Copy link
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
Copy link
Member

@pablogsal pablogsal left a comment

Choose a reason for hiding this comment

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

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
erlend-aasland
erlend-aasland reviewed Oct 28, 2021
View reviewed changes
Modules/posixmodule.c
@@ -7333,7 +7333,7 @@ os_forkpty_impl(PyObject *module)
int master_fd = -1;
pid_t pid;

if (_PyInterpreterState_GET() != PyInterpreterState_Main()) {
if (PyInterpreterState_Get() != PyInterpreterState_Main()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (PyInterpreterState_Get() != PyInterpreterState_Main()) {
PyInterpreterState *interp = PyInterpreterState_Get();
if (!_Py_IsMainInterpreter(interp)) {

Copy link
Member Author

Choose a reason for hiding this comment

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

It is too easy to overlook the ! when making this change. Using the comparison operator makes it clearer that we are checking that interp is not the main interpreter.
Both @ericsnowcurrently and I have made this mistake recently.

@markshannon markshannon requested a review from vstinner October 29, 2021 11:45
@markshannon
Copy link
Member Author

@vstinner Any comments?

@tiran
Copy link
Member

tiran commented Oct 29, 2021

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
Copy link
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
ericsnowcurrently
ericsnowcurrently reviewed Dec 7, 2021
View reviewed changes
Include/internal/pycore_pystate.h
@@ -40,6 +34,9 @@ _Py_GetMainConfig(void)
return _PyInterpreterState_GetConfig(interp);
}

#ifdef _Py_THREAD_LOCAL
extern _Py_THREAD_LOCAL PyInterpreterState *_py_current_interpreter;
Copy link
Member

Choose a reason for hiding this comment

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

Why the current interpreter and not the current PyThreadState?

Copy link
Member Author

Choose a reason for hiding this comment

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

Two reasons.

  1. I though this would add the most value, as we need the interpreter state to get to freelists which are performance critical.
  2. We already pass the threadstate as an argument to a lot of functions, so I wanted to verify this approach before rewriting all that code.

Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to do both? (They are both hot in different situations.) What is the cost to the number of thread-local variables?

Modules/_io/_iomodule.c
@@ -509,7 +509,7 @@ _io_text_encoding_impl(PyObject *module, PyObject *encoding, int stacklevel)
/*[clinic end generated code: output=91b2cfea6934cc0c input=bf70231213e2a7b4]*/
{
if (encoding == NULL || encoding == Py_None) {
PyInterpreterState *interp = _PyInterpreterState_GET();
PyInterpreterState *interp = PyInterpreterState_Get();
Copy link
Member

@ericsnowcurrently ericsnowcurrently Dec 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Why the switch to PyInterpreterState_Get()?

Copy link
Member Author

Choose a reason for hiding this comment

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

Modules cannot access a thread-local directly because they are dynamically loaded. It is a loader, rather a language, limitation, I think.

Copy link
Member

Choose a reason for hiding this comment

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

That's good to know. Thanks for explaining. This might not be obvious to maintainers. Would you mind adding a short comment next to the declaration of _py_current_interpreter noting the constraint?

Also, this doesn't affect builtin modules, right? Aren't some of the modules updated here builtin modules?

@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
Copy link
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 pablogsal left review comments

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

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

@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