classification
Title: Pass _PyRuntimeState as an argument rather than using the _PyRuntime global variable
Type: Stage: patch review
Components: Interpreter Core Versions: Python 3.8
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: eric.snow, jdemeyer, nascheme, ncoghlan, steve.dower, vstinner
Priority: normal Keywords: patch

Created on 2019-04-24 12:58 by vstinner, last changed 2019-06-19 22:05 by vstinner.

Pull Requests
URL Status Linked Edit
PR 12934 merged vstinner, 2019-04-24 14:02
PR 12935 merged vstinner, 2019-04-24 14:32
PR 12936 merged vstinner, 2019-04-24 14:50
PR 12937 merged vstinner, 2019-04-24 14:58
PR 12939 merged vstinner, 2019-04-24 15:28
PR 12956 merged vstinner, 2019-04-25 23:38
PR 12958 merged vstinner, 2019-04-25 23:52
PR 12962 merged vstinner, 2019-04-26 03:25
PR 13540 merged vstinner, 2019-05-24 08:57
PR 13547 merged vstinner, 2019-05-24 13:16
PR 14060 merged vstinner, 2019-06-13 16:15
PR 14218 merged vstinner, 2019-06-19 00:39
PR 14221 merged vstinner, 2019-06-19 01:01
PR 14249 merged vstinner, 2019-06-19 21:16
PR 14250 closed vstinner, 2019-06-19 21:57
Messages (30)
msg340772 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-24 12:58
Eric Snow moved global variables into a _PyRuntimeState structure which is made of sub-structures. There is a single instance of _PyRuntimeState: the _PyRuntime global variable.

I would like to add "_PyRuntimeState *" parameters to functions to avoid relying directly on _PyRuntime global variable. The long term goal is to have "stateless" code: don't rely on global variables, only on input parameters. In practice, we will continue to use thread local storage (TLS) to get the "current context" like the current interpreter and the current Python thread state.
msg340775 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-24 14:47
New changeset 8bb3230149538c25c1bacced5e64a3c071475f73 by Victor Stinner in branch 'master':
bpo-36710: Add runtime parameter to _PyThreadState_Init() (GH-12935)
https://github.com/python/cpython/commit/8bb3230149538c25c1bacced5e64a3c071475f73
msg340779 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-24 15:14
New changeset b930a2d2b1247bdba560db341ba90a9cbb538eb3 by Victor Stinner in branch 'master':
bpo-36710: PyOS_AfterFork_Child() pass runtime parameter (GH-12936)
https://github.com/python/cpython/commit/b930a2d2b1247bdba560db341ba90a9cbb538eb3
msg340784 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-24 15:24
New changeset 8e91c246e468515b877690e090c73f496552541d by Victor Stinner in branch 'master':
bpo-36710: Add runtime variable to Py_FinalizeEx() (GH-12937)
https://github.com/python/cpython/commit/8e91c246e468515b877690e090c73f496552541d
msg340789 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-24 16:23
New changeset 43125224d6da5febb34101ebfd36536d791d68cd by Victor Stinner in branch 'master':
bpo-36710: Add runtime variable to Py_InitializeEx() (GH-12939)
https://github.com/python/cpython/commit/43125224d6da5febb34101ebfd36536d791d68cd
msg340842 - (view) Author: Jeroen Demeyer (jdemeyer) * Date: 2019-04-25 14:49
I don't really understand the rationale for these changes. What's wrong with the global variable _PyRuntime?

What's the long-term goal for _PyRuntime? If you can't get rid of all occurrences of _PyRuntime, how does it help to get rid of *some* occurences?
msg340845 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-25 15:03
> I don't really understand the rationale for these changes. What's wrong with the global variable _PyRuntime?
> What's the long-term goal for _PyRuntime? If you can't get rid of all occurrences of _PyRuntime, how does it help to get rid of *some* occurences?

The long term goal is to support multiple interpreter instances per process:

Eric Snow's PEP 554 "Multiple Interpreters in the Stdlib"
https://www.python.org/dev/peps/pep-0554/

Right now, there is a single instance of _PyRuntimeState: _PyRuntime. I would be interested to have one _PyRuntimeState per interpreter.

Maybe _PyRuntimeState should be reworked in the meanwhile. Right now, it's a strange beast: it contains things which are set before Python initialization and things which are set after. It contains C types and Python objects. Maybe some parts should be moved into PyInterpreterState or even PyThreadState. I don't know at this point. It takes time to look at each individual structure field...

Anyway, more generally, IMHO it's a bad practice to rely on a global variable. Python runtime should be "stateless".

The current implementation of CPython leaks dozens of *Python* objects at exit. For example, I started to work on this issue while working on https://bugzilla.redhat.com/show_bug.cgi?id=1696322 : Python doesn't clear 2 warnings variables at exit. When I looked into this issue, I also noticed that _PyRuntime.gc.garbage remains *alive* after Py_Finalize().

That's plain wrong: *all* Python objects must be cleared by Py_Finalize(). Two interpreters must *not* share any Python object.

Well, the PEP should better explain the rationale than me :-)

--

When I wrote PR 12934, I noticed that even getting the current thread state rely on _PyRuntime:

#define _PyThreadState_GET() \
    ((PyThreadState*)_Py_atomic_load_relaxed(&_PyRuntime.gilstate.tstate_current))

That's wrong in the case of ceval.c: it should be possible to run _PyEval_EvalFrameDefault() twice at the same time in two threads using two "isolated" interpreters.

Well, PR 12934 doesn't fix all issues. It's just one small step towards "stateless" runtime and the even more long term of having one GIL per *interpeter*, rather than a single GIL per *process*.
msg340860 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-25 20:14
I created bpo-36724: Clear _PyRuntime at exit.
msg340871 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-25 23:53
New changeset 10c8e6af910e3a26e59f913a3c1e4830ca71b1af by Victor Stinner in branch 'master':
bpo-36710: Add runtime variable in pystate.c (GH-12956)
https://github.com/python/cpython/commit/10c8e6af910e3a26e59f913a3c1e4830ca71b1af
msg340873 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-26 00:32
New changeset 9db0324712f6982d89620b420f507a6aa5da152f by Victor Stinner in branch 'master':
bpo-36710: Add runtime parameter in gcmodule.c (GH-12958)
https://github.com/python/cpython/commit/9db0324712f6982d89620b420f507a6aa5da152f
msg340880 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-26 03:48
New changeset 99e69d44f499625786a2e6461a954adcd0037d69 by Victor Stinner in branch 'master':
bpo-36710: Fix compiler warning on PyThreadState_Delete() (GH-12962)
https://github.com/python/cpython/commit/99e69d44f499625786a2e6461a954adcd0037d69
msg340915 - (view) Author: Jeroen Demeyer (jdemeyer) * Date: 2019-04-26 12:32
> The long term goal is to support multiple interpreter instances per process:
> Eric Snow's PEP 554 "Multiple Interpreters in the Stdlib"
> https://www.python.org/dev/peps/pep-0554/

Sorry, but I don't see the relation between this issue and PEP 554. It seems to me that the PEP is about making subinterpreters available from pure Python (instead of only at the C level). It doesn't say anything about the *implementation* of subinterpreters, which is what this issue is about.

So I'm still missing the bigger picture where this issue fits in.

> The current implementation of CPython leaks dozens of *Python* objects at exit.

That may be an issue to be fixed, but again I don't see the relation with this issue.
msg340930 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-04-26 16:21
Jeroen Demeyer:
> Sorry, but I don't see the relation between this issue and PEP 554.

The long term plan for PEP 554 is to support having one GIL per interpreter for best performances. The GIL lives in _PyRuntime.

It's not just about the GIL. Currently, the gc module stores its state into _PyRuntime. It's wrong to share a single gc state between two interpreters: each interpreter should have its own "namespace" completely isolated from the other namespaces. For example, _PyRuntime.gc.garbage is a Python list: each interpreter should have its own list.

My PR 12934 is only a first step to prepare ceval.c for that.

Said differently, if I understood correctly, each interpreter must have its own _PyRuntime instance.

Maybe tomorrow, we will keep a single _PyRuntime instance, *but* my work is needed to identify the relationship between the current implementation of Python and _PyRuntime.
msg340941 - (view) Author: Jeroen Demeyer (jdemeyer) * Date: 2019-04-26 20:57
So what's the relation between _PyRuntime and PyInterpreterState? If the latter is a structure per interpreter, what's the point of also making the former per interpreter? It would be better to move data from _PyRuntime to PyInterpreterState.
msg340942 - (view) Author: Jeroen Demeyer (jdemeyer) * Date: 2019-04-26 21:03
> It's wrong to share a single gc state between two interpreters

And what's your solution for that? I'm not asking for a complete ready-to-implement answer, but at least a basic idea. Otherwise it's impossible for me to judge whether your PR 12934 helps with that or not.

Basically I would like to see something like PEP 579 but for this problem.
msg340945 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-04-26 21:46
I don't think this change is the right way to go (yet), but something related might be.  First, let's be clear on the status quo for CPython.  (This has gotten long, but I want to be clear.)


Status Quo
============

For simplicity sake, let's say nearly all the code operates relative to the 3 levels of runtime state:

* global      - _PyRuntimeState
* interpreter - PyInterpreterState
* thread      - PyThreadState

Furthermore, there are 3 groups of functions in the C-API:

* context-sensitive   - operate relative to the current Python thread
* runtime-dependent   - operate relative to some part of the runtime state, regardless of thread
* runtime-independent - have nothing to do with CPython's runtime state

Most of the C-API is context-sensitive.  A small portion is runtime-dependent.  A handful of functions are runtime-independent (effectively otherwise stateless helper functions that only happen to be part of the C-API).

Each context-sensitive function relies on there being a "runtime context" it can use relative to the current OS thread.  That context consists of the current (i.e. active) PyThreadState, the corresponding PyInterpreterState, and the global _PyRuntimeState.  That context is derived from data in TSS (see caveats below).  This group includes most of the C-API.

Each runtime-dependent function operates against one or more runtime state target, regardless of the current thread context (or even if there isn't one).  The target state (e.g. PyInterpreterState) is always passed explicitly.  Again, this is only a small portion of the C-API.

Caveats:
* for context-sensitive functions, we get the global runtime state from the global C variable (_PyRuntime) rather than via the implicit thread context
* for some of the runtime-dependent functions that target _PyRuntimeState, we rely on the global C variable

All of this is the pattern we use currently.  Using TSS to identify the implicit runtime context has certain benefits and costs:

benefits:
* sticking with the status quo means no backward incompatibility for existing C-extension code
* easier to distinguish the context-sensitive functions from the runtime-dependent ones
* (debatable) callers don't have to track, nor pass through, an extra argument

costs:
* extra complexity in keeping TSS correct
* makes the C-API bigger (extra macros, etc.)


Alternative
=============

For every context-sensitive function we could add a new first parameter, "context", that provides the runtime context to use.  That would be something like this:

struct {
    PyThreadState *tstate;
    ...
} PyRuntimeContext;

The interpreter state and global runtime state would still be accessible via the same indirection we have now.

Taking this alternative would eliminate the previous costs.  Having a consistent "PyRuntimeContext *context" first parameter would maintain the easy distinction from runtime-dependent functions.  Asking callers to pass in the context explicitly is probably better regardless.  As to backward compatibility, we could maintain a shim to bridge between the old way and the new.


About the C-global _PyRuntime
==============================

Currently the global runtime state (_PyRuntimeState) is stored in a static global C variable, _PyRuntime.  I added it at the time I consolidated many of the existing C globals into a single struct.  Having a C global makes it easy to do the wrong thing, so it may be good to do something else.

That would mean allocating a _PyRuntimeState on the heap early in startup and pass that around where needed.  I expect that would not have any meaningful performance penalty.  It would probably also simplify some of the code we currently use to manage _PyRuntime correctly.

As a bonus, this would be important if we decided that multiple-runtimes-per-process were a desirable thing.  That's a neat idea, though I don't see a need currently.  So on its own it's not really a justification for dropping a static _PyRuntime. :)  However, I think the other reasons are enough.


Conclusions
====================

This issue has a specific objective that I think is premature.  We have an existing pattern and we should stick with that until we decide to change to a new pattern.  That said, a few things should get corrected and we should investigate alternative patterns for the context-sensitive C-API.

As to getting rid of the _PyRuntime global variable in favor of putting it on the heap, I'm not opposed.  However, doing so should probably be handled in a separate issue.

Here are my thoughts on actionable items:

1. look for a better pattern for the context-sensitive C-API
2. clearly document which of the 3 groups each C-API function belongs to
3. add a "runtime" field to the PyInterpreterState pointing to the parent _PyRuntimeState
4. (maybe) add a _PyRuntimeState_GET() macro, a la PyThreadState_GET()
5. for context-sensitive C-API that uses the global runtime state, get it from the current PyInterpreterState
6. for runtime-dependent C-API that targets the global runtime state, ensure the _PyRuntimeState is always an explicit parameter
7. (maybe) drop _PyRuntime and create a _PyRuntimeState on the heap during startup to pass around
msg340946 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-04-26 21:48
FWIW, PEP 554 is part of a larger project that I've been working on (slowly) for several years now. [1]  The concrete objective is to leverage subinterpreters as the mechanism by which we can achieve multi-core parallelism in Python code.  Moving the GIL (and some other parts of _PyRuntimeState, as Victor indicated) down to per-interpreter state is essential to that.

However, I don't thing making _PyRuntime a per-interpreter thing is right.  The runtime holds the set of interpreters, as well as any state state shared by the interpreters.

Also, to be clear, the status quo is not a problem for me, so make sure I'm not used as the justification for the change (thoughtful as that is of Victor). :)


[1] https://github.com/ericsnowcurrently/multi-core-python
msg340971 - (view) Author: Jeroen Demeyer (jdemeyer) * Date: 2019-04-27 07:21
Changing *every* C API function to include a state parameter looks very cumbersome. Another alternative would be to store the interpreter state in every Python object (or every class, that would be sufficient). That way, you would only need to pass context to C API functions which do not take a Python object as argument.
msg340986 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-04-27 14:35
Changing every API to take the context parameter would bring us into alignment with the JavaScript VMs.

I'm working on a project that embeds a few of these, as well as Python, and our thread management is much worse than their context parameter. Though I'm of course very sympathetic to the compatibility argument (but then the shims would just load the context from TSS and pass it around, so they're not too bad).

Eric's breakdown of context scopes seems spot on, and it means that we only really need the thread state to be passed around. The few places that would be satisfied by runtime state now (GIL, GC) should become interpreter state, which is most easily found from a thread state anyway.

Runtime state should eventually probably become runtime configuration (those settings we need to create interpreters) and a minimum amount of state to track live interpreters. I see no reason to pass it around anywhere other than interpreter creation, and as a transitional step toward that goal it should be accessible through the active interpreter state.
msg340996 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2019-04-27 15:59
FWIW, I don't mean to side-track this issue.  If we want to have any further discussion about broader solutions then let's take this to capi-sig.  In fact, I've started a thread there.  I'd post the link, but I think it got stuck in moderation. :)
msg341305 - (view) Author: Neil Schemenauer (nascheme) * (Python committer) Date: 2019-05-02 19:07
I think there are two questions to answer.  First, do we want to support multiple runtimes per process?  Second, if we do, what is the best way to do that?  Some people would argue that multiple runtimes are not needed or are too hard to do.  Maybe they are correct, I'm not sure.  We should try to get a consensus on that first question.

If we do decide to do it, then we need to answer the second question.  Passing a "context" argument around seems the best solution.  That is how the Java JNI does it.  It sounds like that's how Javascript VMs do it too.  We don't need to get creative.  Look at what other VMs do and copy the best idea.

If we do decide to do it, evolving the codebase and all extension modules is going to be a massive task.  I would imagine that we can have a backwards compatible API layer that uses TSS.  The layer that passes context explicitly would still have to maintain the TSS.  There could be a build option that turns that backwards compatibility on or off.  If off, you would gain some performance advantage because TSS does not have to be kept up-to-date.

My feeling right now that even though this is a massive job, it is the correct thing to do.  CPUs continue to gain cores.  Improving CPython's ability to do multi-threading and multi-processing should be a priority for CPython core developers.
msg341500 - (view) Author: Steve Dower (steve.dower) * (Python committer) Date: 2019-05-06 13:51
I think Neil is right, though I believe we'll have a clear enough internal boundary that we should only rarely have to maintain TSS for the sake of legacy callers. The build option should just turn off the entire legacy API, which would also make it easier to remove one day.
msg342138 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-05-10 21:39
New changeset 09532feeece39d5ba68a0d47115ce1967bfbd58e by Victor Stinner in branch 'master':
bpo-36710: Add 'ceval' local variable to ceval.c (GH-12934)
https://github.com/python/cpython/commit/09532feeece39d5ba68a0d47115ce1967bfbd58e
msg343380 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-05-24 11:44
New changeset b4bdecd0fc9112b60a81fec171bc78bc13f2f59c by Victor Stinner in branch 'master':
bpo-36710: Add tstate parameter in errors.c (GH-13540)
https://github.com/python/cpython/commit/b4bdecd0fc9112b60a81fec171bc78bc13f2f59c
msg343393 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-05-24 15:02
New changeset 438a12dd9d85f463c0bb7bf1505cd87b98b98170 by Victor Stinner in branch 'master':
bpo-36710: Add tstate parameter in ceval.c (GH-13547)
https://github.com/python/cpython/commit/438a12dd9d85f463c0bb7bf1505cd87b98b98170
msg345537 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-13 20:41
New changeset 838f26402de82640698c38ea9d2be65c6cf780d6 by Victor Stinner in branch 'master':
bpo-36710: Pass explicitly tstate in sysmodule.c (GH-14060)
https://github.com/python/cpython/commit/838f26402de82640698c38ea9d2be65c6cf780d6
msg345538 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-13 20:42
I wrote my notes on this issue there:
https://pythoncapi.readthedocs.io/runtime.html
msg346016 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-19 00:54
New changeset 0a28f8d379544eee897979da0ce99f0b449b49dd by Victor Stinner in branch 'master':
bpo-36710: Add tstate parameter in import.c (GH-14218)
https://github.com/python/cpython/commit/0a28f8d379544eee897979da0ce99f0b449b49dd
msg346030 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-19 08:36
New changeset 987a0dcfa1302df6c1ed8cf14762dc18628e3f33 by Victor Stinner in branch 'master':
bpo-36710: Remove PyImport_Cleanup() function (GH-14221)
https://github.com/python/cpython/commit/987a0dcfa1302df6c1ed8cf14762dc18628e3f33
msg346088 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-06-19 22:05
New changeset b45d259bdda1de2b2d369458a9ad2e4d6f750687 by Victor Stinner in branch 'master':
bpo-36710: Use tstate in pylifecycle.c (GH-14249)
https://github.com/python/cpython/commit/b45d259bdda1de2b2d369458a9ad2e4d6f750687
History
Date User Action Args
2019-06-19 22:05:27vstinnersetmessages: + msg346088
2019-06-19 21:57:31vstinnersetpull_requests: + pull_request14082
2019-06-19 21:16:04vstinnersetpull_requests: + pull_request14081
2019-06-19 08:36:25vstinnersetmessages: + msg346030
2019-06-19 01:01:03vstinnersetpull_requests: + pull_request14059
2019-06-19 00:54:42vstinnersetmessages: + msg346016
2019-06-19 00:39:09vstinnersetpull_requests: + pull_request14056
2019-06-13 20:42:49vstinnersetmessages: + msg345538
2019-06-13 20:41:47vstinnersetmessages: + msg345537
2019-06-13 16:15:02vstinnersetpull_requests: + pull_request13922
2019-05-24 15:02:03vstinnersetmessages: + msg343393
2019-05-24 13:16:28vstinnersetpull_requests: + pull_request13459
2019-05-24 11:44:26vstinnersetmessages: + msg343380
2019-05-24 08:57:37vstinnersetpull_requests: + pull_request13454
2019-05-10 21:39:12vstinnersetmessages: + msg342138
2019-05-06 13:51:58steve.dowersetmessages: + msg341500
2019-05-02 19:07:42naschemesetnosy: + nascheme
messages: + msg341305
2019-04-27 15:59:38eric.snowsetmessages: + msg340996
2019-04-27 14:35:33steve.dowersetmessages: + msg340986
2019-04-27 07:21:53jdemeyersetmessages: + msg340971
2019-04-26 21:48:28eric.snowsetmessages: + msg340946
2019-04-26 21:46:45eric.snowsetnosy: + ncoghlan, steve.dower
messages: + msg340945
2019-04-26 21:03:51jdemeyersetmessages: + msg340942
2019-04-26 20:57:31jdemeyersetmessages: + msg340941
2019-04-26 16:21:23vstinnersetmessages: + msg340930
2019-04-26 12:32:55jdemeyersetmessages: + msg340915
2019-04-26 03:48:54vstinnersetmessages: + msg340880
2019-04-26 03:25:10vstinnersetpull_requests: + pull_request12888
2019-04-26 00:32:05vstinnersetmessages: + msg340873
2019-04-25 23:53:21vstinnersetmessages: + msg340871
2019-04-25 23:52:09vstinnersetpull_requests: + pull_request12884
2019-04-25 23:38:27vstinnersetpull_requests: + pull_request12882
2019-04-25 20:14:17vstinnersetmessages: + msg340860
2019-04-25 15:03:25vstinnersetmessages: + msg340845
2019-04-25 14:49:40jdemeyersetnosy: + jdemeyer
messages: + msg340842
2019-04-24 17:20:23eric.snowsetnosy: + eric.snow
2019-04-24 16:23:57vstinnersetmessages: + msg340789
2019-04-24 15:28:18vstinnersetpull_requests: + pull_request12863
2019-04-24 15:24:06vstinnersetmessages: + msg340784
2019-04-24 15:14:42vstinnersetmessages: + msg340779
2019-04-24 14:58:47vstinnersetpull_requests: + pull_request12861
2019-04-24 14:50:13vstinnersetpull_requests: + pull_request12860
2019-04-24 14:47:50vstinnersetmessages: + msg340775
2019-04-24 14:32:39vstinnersetpull_requests: + pull_request12859
2019-04-24 14:02:02vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request12858
2019-04-24 12:58:00vstinnercreate