classification
Title: Move PyThreadState into Include/internal/pycore_pystate.h
Type: behavior Stage:
Components: Interpreter Core Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: eric.snow, ncoghlan, scoder, vstinner
Priority: normal Keywords:

Created on 2019-02-09 02:00 by eric.snow, last changed 2019-02-16 10:00 by scoder.

Messages (4)
msg335122 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-02-09 02:00
(also see issue #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.

1. the docs say: "The only public data member is PyInterpreterState *interp, which points to this thread’s interpreter state."
2. the struct has 6 fields that are exposed (likely unintentionally) in "stable" header files via the following macros:

  # 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.
msg335465 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-02-13 17:06
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.
msg335677 - (view) Author: Stefan Behnel (scoder) * Date: 2019-02-16 09:48
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.
msg335678 - (view) Author: Stefan Behnel (scoder) * Date: 2019-02-16 10:00
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
History
Date User Action Args
2019-02-16 10:00:46scodersetmessages: + msg335678
2019-02-16 09:48:09scodersetnosy: + scoder
messages: + msg335677
2019-02-13 17:06:53vstinnersetmessages: + msg335465
2019-02-09 02:00:19eric.snowcreate