Repository navigation
Move PyThreadState into Include/internal/pycore_pystate.h #80130
Description
| BPO | 35949 |
|---|---|
| Nosy | @ncoghlan, @scoder, @vstinner, @ericsnowcurrently |
| Superseder |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = <Date 2020-06-25.09:26:12.263>
created_at = <Date 2019-02-09.02:00:19.820>
labels = ['interpreter-core', 'type-bug', '3.8']
title = 'Move PyThreadState into Include/internal/pycore_pystate.h'
updated_at = <Date 2020-06-25.09:26:12.263>
user = 'https://github.com/ericsnowcurrently'bugs.python.org fields:
activity = <Date 2020-06-25.09:26:12.263>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2020-06-25.09:26:12.263>
closer = 'vstinner'
components = ['Interpreter Core']
creation = <Date 2019-02-09.02:00:19.820>
creator = 'eric.snow'
dependencies = []
files = []
hgrepos = []
issue_num = 35949
keywords = []
message_count = 9.0
messages = ['335122', '335465', '335677', '335678', '335859', '335864', '335865', '372317', '372318']
nosy_count = 4.0
nosy_names = ['ncoghlan', 'scoder', 'vstinner', 'eric.snow']
pr_nums = []
priority = 'normal'
resolution = 'duplicate'
stage = 'resolved'
status = 'closed'
superseder = '39947'
type = 'behavior'
url = 'https://bugs.python.org/issue35949'
versions = ['Python 3.8']
Activity
ericsnowcurrently commentedon Feb 9, 2019
(also see issue bpo-35886)
In November Victor created the Include/cpython directory and moved a decent amount of public (but not limited) API there. This included the PyThreadState struct. I'd like to move it into the "internal" headers since it is somewhat coupled to the internal runtime implementation. The alternative is extra complexity. I ran into this while working on another issue.
There are 2 problems, however, with PyThreadState which make a move potentially challenging. In fact, they could be (but probably aren't) cause for moving it back to Include/pystate.h.
# Include/object.h
Py_TRASHCAN_SAFE_BEGIN
Py_TRASHCAN_SAFE_END
Include.ceval.h
Py_EnterRecursiveCall
Py_LeaveRecursiveCall
_Py_MakeRecCheck
Py_ALLOW_RECURSION
Py_END_ALLOW_RECURSION
I'm not sure how that factors into the stable ABI (PyThreadState wasn't ever guarded by Py_LIMITED_API). However, keep in mind that Victor already moved PyThreadState out of the "stable" headers in November.
I'm fine with sorting out the situation with the macros if that's okay to do. Likewise the promised field ("interp") should be solvable one way or another (e.g. remove it or use a private struct).
...or we could do nothing or (if the change in Novemver is a problem) move it back to Include/pystate.h.
vstinner commentedon Feb 13, 2019
Same ratione than for PyInterpreterState: https://bugs.python.org/issue35886#msg335464
Except that I expect that a few more projects rely on PyThreadState fields. Maybe not. It's hard to guess :-(
I mean that I'm ok-ish with the change but it should be carefully prepared and announced.
scoder commentedon Feb 16, 2019
From Cython's point of view, the important fields in PyThreadState are the tracing/profiling and exception related ones. We're not using anything else. Users can explicitly opt out of the access to the exception fields by defining a C macro, at the expense of a bit of performance. I doubt that anyone is really doing that, though, because, why would they?
I'm not sure if we could avoid direct field access for profiling and tracing at all. You can look at the code, it's a whole bunch of macros that mimic what CPython does in ceval:
https://github.com/cython/cython/blob/master/Cython/Utility/Profile.c
I should note that both features are not enabled by default. Users have to explicitly enable profiling support via a directive, and even doubly opt in to tracing by enabling a compiler directive and a C macro. So production code will often not rely on these fields, but developers would want to use them and some might keep profiling support enabled also in their production code. My guess is that tracing is most often used for test coverage analysis.
scoder commentedon Feb 16, 2019
Oh, and I forgot the new trashcan support. Cython will also start to use that in its next release, so that adds the trashcan related attributes to the list.
https://github.com/cython/cython/pull/2842/files
ericsnowcurrently commentedon Feb 18, 2019
@Stefan, is it a problem for Cython if the relevant fields are exposed via C-API functions rather than directly on PyThreadState?
scoder commentedon Feb 18, 2019
Well … yes.
The exception fields are performance critical, and we try hard to make them visible to the C compiler so that swapping around exception state eats up as little CPU time as possible.
You could argue that profiling and tracing are less critical, but any nanosecond that is avoided while not tracing a function adds up to making the rest of the program faster, so I'd argue that that's performance critical, too. Profiling definitely is, because it should have as little impact on the code profile as possible. There is a huge difference between having the CPU pre-fetch a pointer and looking at the value, compared to calling into a C function and guessing what the result might be.
The trashcan is only used during deallocation, so … well, I guess it could be replaced by a different API, but that's a bit tricky due to the bracket nature of the current macros.
I also just noticed that "Py_EnterRecursiveCall" and "Py_LeaveRecursiveCall" are on your list. We use them in our inlined call helper functions, which mostly duplicate CPython functionality. Looking at these macros now, I find it a bit annoying that they call "PyThreadState_GET()" directly, rather than accepting one as input. Looking up the current thread-state is a non-local, atomic operation that can be surprisingly costly, and I've invested quite some work into reducing these lookups in Cython. Although it's probably not too bad around a call into an external function…
So, yeah, we do care about the thread state being readily available. :)
Could you explain what benefit you are expecting from hiding the thread state?
ericsnowcurrently commentedon Feb 18, 2019
Thanks for clarifying. :)
On Mon, Feb 18, 2019 at 1:17 PM Stefan Behnel <report@bugs.python.org> wrote:
Yeah, I had a feeling that was the case. :) There might be decent
approaches to avoiding a performance hit on this, but I'm not sure
it's worth the extra complexity. And I'm not terribly motivated to
make the effort personally. :)
Yep, definitely tricky.
Hmm, perhaps that's something we could address separately?
This isn't so much about the thread state specifically or about
necessarily hiding *all* the thread state. Rather it's about a
general effort to reduce the amount of private API we are exposing
publicly. In this case I was looking at PyInterpreterState (bpo-35886)
and opening a similar issue at the same time for PyThreadState made
sense.
Given your response it's clear that usage of PyThreadState doesn't
match the documentation very well. That would be worth addressing one
way or another, so I'm glad we're having this conversation. :)
vstinner commentedon Jun 25, 2020
I excluded the TRASHCAN API from the limited C API:
commit 0fa4f43
Author: Victor Stinner <vstinner@python.org>
Date: Wed Feb 5 12:23:27 2020 +0100
Then I modified these macros to hide implementation details:
commit 38965ec
Author: Victor Stinner <vstinner@python.org>
Date: Fri Mar 13 16:51:52 2020 +0100
Py_EnterRecursiveCall() and Py_LeaveRecursiveCall() are now function calls. I move the "inline" implementation the internal C API.
commit 224481a
Author: Victor Stinner <vstinner@python.org>
Date: Fri Mar 13 10:19:38 2020 +0100
Oh right, these macros should be modified to not access PyThreadState.recursion_critical member directly.
vstinner commentedon Jun 25, 2020
I mark this issue as a duplicate of bpo-39947 "[C API] Make the PyThreadState structure opaque (move it to the internal C API)" since I already pushed multiple changes there. But this issue contains interesting technical information!