classification
Title: [subinterpreters] Move _PyRuntimeState.ceval to PyInterpreterState
Type: Stage: resolved
Components: Subinterpreters Versions: Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: eric.snow, shihai1991, vstinner
Priority: normal Keywords: patch

Created on 2020-05-05 12:56 by vstinner, last changed 2020-05-15 01:34 by vstinner. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 19924 merged vstinner, 2020-05-05 12:58
PR 19927 merged vstinner, 2020-05-05 13:52
PR 19929 merged vstinner, 2020-05-05 14:31
PR 19932 merged vstinner, 2020-05-05 15:04
PR 19942 merged vstinner, 2020-05-05 17:53
PR 19943 merged vstinner, 2020-05-05 18:01
PR 19944 merged vstinner, 2020-05-05 18:09
Messages (17)
msg368137 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-05 12:56
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:

struct _ceval_runtime_state {
    int recursion_limit;
    /* Request for dropping the GIL */
    _Py_atomic_int gil_drop_request;
    /* Request for checking signals. */
    _Py_atomic_int signals_pending;
    struct _gil_runtime_state gil;
};

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.
msg368139 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-05 12:57
See also: https://github.com/ericsnowcurrently/multi-core-python/issues/34
msg368147 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-05 13:43
New changeset 4e01946cafca0cf49f796c3118e0d65237bcad69 by Victor Stinner in branch 'master':
bpo-40513: Per-interpreter signals pending (GH-19924)
https://github.com/python/cpython/commit/4e01946cafca0cf49f796c3118e0d65237bcad69
msg368150 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-05 13:59
Python/ceval.c also has "int _Py_CheckRecursionLimit = Py_DEFAULT_RECURSION_LIMIT;". _Py_CheckRecursiveCall() has this comment:

    /* Needed for ABI backwards-compatibility (see bpo-31857) */
    _Py_CheckRecursionLimit = recursion_limit;

I converted Py_EnterRecursiveCall() and Py_LeaveRecursiveCall() macros into opaque functions in bpo-38644: commit f4b1e3d7c64985f5d5b00f6cc9a1c146bbbfd613.

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.
msg368151 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-05 14:14
New changeset 0b1e3307e24b0af45787ab6456535b8346e0239a by Victor Stinner in branch 'master':
bpo-40513: Per-interpreter gil_drop_request (GH-19927)
https://github.com/python/cpython/commit/0b1e3307e24b0af45787ab6456535b8346e0239a
msg368153 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-05 14:34
For signals_pending, see also: bpo-40010 "Inefficient signal handling in multithreaded applications" which added _Py_ThreadCanHandleSignals() and _Py_ThreadCanHandlePendingCalls() functions.
msg368162 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-05 14:52
New changeset 4e30ed3af06ae655f4cb8aad8cba21f341384250 by Victor Stinner in branch 'master':
bpo-40513: Per-interpreter recursion_limit (GH-19929)
https://github.com/python/cpython/commit/4e30ed3af06ae655f4cb8aad8cba21f341384250
msg368165 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2020-05-05 15:11
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.
msg368168 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2020-05-05 15:25
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.
msg368169 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2020-05-05 15:26
If this issue covers the GIL (which it seems to) then I'd expect _PyRuntimeState.gilstate to be handled here too.
msg368174 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-05 15:40
New changeset 299b8c61e9d1a42b929b8deb1b05067876e191e6 by Victor Stinner in branch 'master':
Revert "bpo-40513: Per-interpreter signals pending (GH-19924)" (GH-19932)
https://github.com/python/cpython/commit/299b8c61e9d1a42b929b8deb1b05067876e191e6
msg368178 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-05 16:51
> FWIW, I think it would make sense to keep "signals_pending" under _PyRuntimeState rather than moving it to PyInterpreterState.

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 :-)
msg368179 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-05 16:53
> From a user perspective, does it make sense to have a different recursion_limit per interpreter?

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.
msg368190 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-05 18:16
New changeset 0dd5e7a718997da2026ed64fe054dc36cae4fee7 by Victor Stinner in branch 'master':
bpo-40513: new_interpreter() init GIL earlier (GH-19942)
https://github.com/python/cpython/commit/0dd5e7a718997da2026ed64fe054dc36cae4fee7
msg368194 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-05 18:27
New changeset 7be4e350aadf93c4be5c97b7291d0db2b6bc1dc4 by Victor Stinner in branch 'master':
bpo-40513: Per-interpreter GIL (GH-19943)
https://github.com/python/cpython/commit/7be4e350aadf93c4be5c97b7291d0db2b6bc1dc4
msg368196 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-05 18:33
New changeset fb2c7c4afbab0514352ab0246b0c0cc85d1bba53 by Victor Stinner in branch 'master':
bpo-40513: _xxsubinterpreters.run_string() releases the GIL (GH-19944)
https://github.com/python/cpython/commit/fb2c7c4afbab0514352ab0246b0c0cc85d1bba53
msg368909 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2020-05-15 01:34
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.
History
Date User Action Args
2020-05-15 01:34:28vstinnersetstatus: open -> closed
resolution: fixed
messages: + msg368909

stage: patch review -> resolved
2020-05-15 00:35:40vstinnersetcomponents: + Subinterpreters, - Interpreter Core
title: Move _PyRuntimeState.ceval to PyInterpreterState -> [subinterpreters] Move _PyRuntimeState.ceval to PyInterpreterState
2020-05-05 18:33:10vstinnersetmessages: + msg368196
2020-05-05 18:27:53vstinnersetmessages: + msg368194
2020-05-05 18:16:45vstinnersetmessages: + msg368190
2020-05-05 18:09:44vstinnersetpull_requests: + pull_request19258
2020-05-05 18:01:16vstinnersetpull_requests: + pull_request19257
2020-05-05 17:53:38vstinnersetpull_requests: + pull_request19256
2020-05-05 16:53:16vstinnersetmessages: + msg368179
2020-05-05 16:51:28vstinnersetmessages: + msg368178
2020-05-05 15:40:25vstinnersetmessages: + msg368174
2020-05-05 15:26:56eric.snowsetmessages: + msg368169
2020-05-05 15:25:38eric.snowsetmessages: + msg368168
2020-05-05 15:11:26eric.snowsetnosy: + eric.snow
messages: + msg368165
2020-05-05 15:04:51vstinnersetpull_requests: + pull_request19247
2020-05-05 14:52:58vstinnersetmessages: + msg368162
2020-05-05 14:34:48vstinnersetmessages: + msg368153
2020-05-05 14:31:45vstinnersetpull_requests: + pull_request19244
2020-05-05 14:22:56shihai1991setnosy: + shihai1991
2020-05-05 14:14:39vstinnersetmessages: + msg368151
2020-05-05 13:59:58vstinnersetmessages: + msg368150
2020-05-05 13:52:22vstinnersetpull_requests: + pull_request19242
2020-05-05 13:43:44vstinnersetmessages: + msg368147
2020-05-05 12:58:03vstinnersetkeywords: + patch
stage: patch review
pull_requests: + pull_request19239
2020-05-05 12:57:32vstinnersetmessages: + msg368139
2020-05-05 12:56:32vstinnercreate