Repository navigation
[subinterpreters] Move _PyRuntimeState.ceval to PyInterpreterState #84693
Description
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-05-15.01:34:28.794>
created_at = <Date 2020-05-05.12:56:32.971>
labels = ['expert-subinterpreters', '3.9']
title = '[subinterpreters] Move _PyRuntimeState.ceval to PyInterpreterState'
updated_at = <Date 2020-05-15.01:34:28.793>
user = 'https://github.com/vstinner'bugs.python.org fields:
activity = <Date 2020-05-15.01:34:28.793>
actor = 'vstinner'
assignee = 'none'
closed = True
closed_date = <Date 2020-05-15.01:34:28.794>
closer = 'vstinner'
components = ['Subinterpreters']
creation = <Date 2020-05-05.12:56:32.971>
creator = 'vstinner'
dependencies = []
files = []
hgrepos = []
issue_num = 40513
keywords = ['patch']
message_count = 17.0
messages = ['368137', '368139', '368147', '368150', '368151', '368153', '368162', '368165', '368168', '368169', '368174', '368178', '368179', '368190', '368194', '368196', '368909']
nosy_count = 3.0
nosy_names = ['vstinner', 'eric.snow', 'shihai1991']
pr_nums = ['19924', '19927', '19929', '19932', '19942', '19943', '19944']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = None
url = 'https://bugs.python.org/issue40513'
versions = ['Python 3.9']
Activity
vstinner commentedon May 5, 2020
To get one GIL per interpreter (bpo-40512), remaining _PyRuntimeState.ceval members should be moved to PyInterpreterState.ceval. The work started in bpo-39984 with ceval "pending calls" and ceval "eval breaker".
There are 4 remaining fields:
The most complex part will be to move the "gil" member: bpo-40512 lists all sub-tasks which must be completed before being able to have one GIL per interpreter without crashing.
vstinner commentedon May 5, 2020
See also: ericsnowcurrently/multi-core-python#34
vstinner commentedon May 5, 2020
New changeset 4e01946 by Victor Stinner in branch 'master':
bpo-40513: Per-interpreter signals pending (GH-19924)
4e01946
vstinner commentedon May 5, 2020
Python/ceval.c also has "int _Py_CheckRecursionLimit = Py_DEFAULT_RECURSION_LIMIT;". _Py_CheckRecursiveCall() has this comment:
I converted Py_EnterRecursiveCall() and Py_LeaveRecursiveCall() macros into opaque functions in bpo-38644: commit f4b1e3d.
I don't think that the "ABI backwards-compatibility" rationale is correct. These macros never worked in the limited C API: they accessed PyThreadState.recursion_depth and PyThreadState.overflowed members, whereas the PyThreadState structure is opaque in the limited C API (on purpose, see also bpo-39947).
I think that it is now safe to simply remove _Py_CheckRecursionLimit.
vstinner commentedon May 5, 2020
New changeset 0b1e330 by Victor Stinner in branch 'master':
bpo-40513: Per-interpreter gil_drop_request (GH-19927)
0b1e330
vstinner commentedon May 5, 2020
For signals_pending, see also: bpo-40010 "Inefficient signal handling in multithreaded applications" which added _Py_ThreadCanHandleSignals() and _Py_ThreadCanHandlePendingCalls() functions.
vstinner commentedon May 5, 2020
New changeset 4e30ed3 by Victor Stinner in branch 'master':
bpo-40513: Per-interpreter recursion_limit (GH-19929)
4e30ed3
ericsnowcurrently commentedon May 5, 2020
FWIW, I think it would make sense to keep "signals_pending" under _PyRuntimeState rather than moving it to PyInterpreterState.
Signals are only handled by the main interpreter (in its main thread). Even though "signals_pending" is useful to only one interpreter in the runtime, making it per-interpreter sends the wrong message that it is significant at that level. This may be confusing to readers of the code.
At the very least there should be a clear comment with the field in Include/internal/pycore_interp.h explaining how it is only used by the main interpreter and why we made it per-interpreter anyway.
ericsnowcurrently commentedon May 5, 2020
From a user perspective, does it make sense to have a different recursion_limit per interpreter? I don't see a problem with it. However, does it make sense to also keep a global value that we default to when a per-interpreter value is not set? I think it might.
I suppose a bigger question is what users will expect the recursion limit (AKA "sys.getrecursionlimit()") to be for a newly created subinterpreter. Will it be some global default? Will it be the value from the parent interpreter? I'd go with a global default, which would imply that the default value should be stored under _PyRuntimeState like we had it (but still keep the actual per-interpreter field for the actual value).
FWIW, the existing docs don't really block either approach.
ericsnowcurrently commentedon May 5, 2020
If this issue covers the GIL (which it seems to) then I'd expect _PyRuntimeState.gilstate to be handled here too.
vstinner commentedon May 5, 2020
New changeset 299b8c6 by Victor Stinner in branch 'master':
Revert "bpo-40513: Per-interpreter signals pending (GH-19924)" (GH-19932)
299b8c6
vstinner commentedon May 5, 2020
Oh right. I forgot about the details. I reverted my change, but this time I added a comment to prevent me to write the same change in 6 months :-)
vstinner commentedon May 5, 2020
Yes, I think so. Someone might use a lower limit to run a template rendering engine or "unsafe" code. Well, it's not hard to find random usage for interpreters with a configuration different than the main interpreter.
vstinner commentedon May 5, 2020
New changeset 0dd5e7a by Victor Stinner in branch 'master':
bpo-40513: new_interpreter() init GIL earlier (GH-19942)
0dd5e7a
vstinner commentedon May 5, 2020
New changeset 7be4e35 by Victor Stinner in branch 'master':
bpo-40513: Per-interpreter GIL (GH-19943)
7be4e35
vstinner commentedon May 5, 2020
New changeset fb2c7c4 by Victor Stinner in branch 'master':
bpo-40513: _xxsubinterpreters.run_string() releases the GIL (GH-19944)
fb2c7c4
[-]Move _PyRuntimeState.ceval to PyInterpreterState[/-][+][subinterpreters] Move _PyRuntimeState.ceval to PyInterpreterState[/+][-]Move _PyRuntimeState.ceval to PyInterpreterState[/-][+][subinterpreters] Move _PyRuntimeState.ceval to PyInterpreterState[/+]vstinner commentedon May 15, 2020
Well, the remaining field which should be moved is the GIL lock itself. It will likely be the last thing to do in bpo-40512. I consider that the work is done in this issue and so I close it.