classification
Title: Consolidate stateful C globals under a single struct.
Type: enhancement Stage: resolved
Components: Versions: Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: eric.snow Nosy List: benjamin.peterson, eric.snow, jkloth, ncoghlan, pitrou, serhiy.storchaka, vstinner, yselivanov
Priority: normal Keywords: patch

Created on 2017-07-05 20:30 by eric.snow, last changed 2017-09-14 23:57 by eric.snow. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 2594 merged eric.snow, 2017-07-05 21:10
PR 3379 merged eric.snow, 2017-09-06 04:14
PR 3397 merged eric.snow, 2017-09-06 22:31
PR 3458 merged eric.snow, 2017-09-08 17:37
PR 3498 merged vstinner, 2017-09-11 15:04
PR 3499 merged vstinner, 2017-09-11 15:09
PR 3506 merged eric.snow, 2017-09-11 22:54
PR 3507 merged eric.snow, 2017-09-11 23:08
PR 3567 merged eric.snow, 2017-09-14 07:15
Messages (19)
msg297780 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-07-05 20:30
CPython's C code makes extensive use of global variables.  The globals fall into one of several categories:

* (effectively) constants (incl. static types)
* freelists, caches, and counters
* used exclusively in main or in REPL
* process-global state
* module state
* Python runtime state

Those in the last category are not explicitly organized nor easily discoverable.  Among other things, this has an impact on efforts that change the runtime (e.g. my multi-core Python project).  To improve the situation I'd like to do the following:

1. group the (stateful) runtime globals into various topical structs
2. consolidate the topical structs under a single top-level _PyRuntimeState struct
3. add a check-c-globals.py script that helps identify runtime globals

One side effect of consolidating these globals is a significant performance improvement of CPython.  Presumably this is due to the caching behavior of a single struct vs. that of dozens of separate globals.

I have a patch and will put up a PR momentarily.
msg297782 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-07-05 21:12
One thing I'd like to also try is to use a freelist embedded in _PyRuntimeState for the PyInterpreterState and PyThreadState linked lists.
msg297794 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2017-07-06 03:42
See also issue29881.
msg297848 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2017-07-06 22:08
After looking at the PR, I'm a bit skeptical about this.  Suddenly a lot of things which are implementation details get moved to the public include files, which makes things more confusing from my POV.  I also don't know why all globals should be consolidated in a single place (it is not a widespread policy in other projects AFAIK, but I may be mistaken).

Perhaps it would be more reasonable to consolidate the globals in each .c file independently, without trying to join all of them together in one place nor expose them in the public include files.
msg297851 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-07-07 01:59
The core motivation driving the original refactoring was to better understand and consolidate our runtime state in order to clarify what the GIL is actually protecting (aside from the reference counts). That then turned out to have surprising performance benefits (presumably by way of loading all of this state permanently into the CPU cache and then keeping it there).

The changes to the public include files arise more from the fact we don't currently have a common place to put internal CPython headers for declarations we want to share across compilation modules.

I agree the latter is confusing, and it could be worth having a separate "Include/internal/CPython.h" header that outright failed the build if you attempted to include it without Py_BUILD_CORE set.

(This also ties into my comments on the PR, where I believe the current approach of mixing public and private state in the same structs will cause problems with compilers generating incorrect field offsets in extension modules and embedding applications)
msg301428 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-09-06 01:26
New changeset 76d5abc8684bac4f2fc7cccfe2cd940923357351 by Eric Snow in branch 'master':
bpo-30860: Consolidate stateful runtime globals. (#2594)
https://github.com/python/cpython/commit/76d5abc8684bac4f2fc7cccfe2cd940923357351
msg301435 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-06 03:06
Sadly, Windows doesn't like much your big change.

Just one example:

     2>C:\buildbot.python.org\3.x.kloth-win64\build\Include\Python-ast.h(563): warning C4005: 'Yield': macro redefinition [C:\buildbot.python.org\3.x.kloth-win64\build\PCbuild\pythoncore.vcxproj]
         C:\Program Files (x86)\Windows Kits\8.1\Include\um\winbase.h(97): note: see previous definition of 'Yield'
         bltinmodule.c

http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/954/steps/compile/logs/stdio
msg301436 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2017-09-06 03:37
I'm also a bit skeptical about the header design in this PR. We should not to have a monolithic _Python.h header, and it should not be sometimes included in Python.h. Rather, code that needs the internal headers should include them directly. In fact, this PR regresses that by moving all the internal headers into Include/...
msg301438 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-09-06 04:43
New changeset 05351c1bd8b70d1878527762174cdaaba3572395 by Eric Snow in branch 'master':
Revert "bpo-30860: Consolidate stateful runtime globals." (#3379)
https://github.com/python/cpython/commit/05351c1bd8b70d1878527762174cdaaba3572395
msg301599 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2017-09-07 17:58
Regarding the accidental exposure of _Py_CheckRecursionLimit, the main active usage of the stable ABI that we're aware of is Riverbank's C/C++ binding generator for PyQt: http://pyqt.sourceforge.net/Docs/sip4/directives.html#directive-%Module (see the use_limited_api option for the linked directive)

I checked with Phil Thomson (sip's maintainer), and the bindings sip generates don't use either of the public macros that access this nominally private value.
msg301679 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-09-08 05:51
New changeset 2ebc5ce42a8a9e047e790aefbf9a94811569b2b6 by Eric Snow in branch 'master':
bpo-30860: Consolidate stateful runtime globals. (#3397)
https://github.com/python/cpython/commit/2ebc5ce42a8a9e047e790aefbf9a94811569b2b6
msg301735 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-09-08 21:27
FYI, the merged code introduced a bunch of new warnings on Windows.  I'm looking into it.
msg301884 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-11 16:31
The commit 2ebc5ce42a8a9e047e790aefbf9a94811569b2b6 introduced reference leaks: see bpo-31420 which tracks them.
msg301885 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-11 16:34
New changeset 4866957e86594935ec2e6434b6e470ebeb0c79b4 by Victor Stinner in branch 'master':
bpo-30860: Add Include/internal/ in "make tags" (#3498)
https://github.com/python/cpython/commit/4866957e86594935ec2e6434b6e470ebeb0c79b4
msg301927 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-09-12 00:02
New changeset ba6d5d1defd7a281c8c8804e4b4cfd7370886236 by Eric Snow in branch 'master':
bpo-30860: Always provide serialno. (#3507)
https://github.com/python/cpython/commit/ba6d5d1defd7a281c8c8804e4b4cfd7370886236
msg301931 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-12 00:59
New changeset 8728018624f257c7cfe44014742ae46134047f49 by Victor Stinner (Eric Snow) in branch 'master':
bpo-30860: Fix a refleak. (#3506)
https://github.com/python/cpython/commit/8728018624f257c7cfe44014742ae46134047f49
msg301935 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-09-12 01:30
New changeset fc1bf872e9d31f3e837f686210f94e57ad3d6582 by Eric Snow in branch 'master':
bpo-30860: Move windows.h include out of internal/*.h. (#3458)
https://github.com/python/cpython/commit/fc1bf872e9d31f3e837f686210f94e57ad3d6582
msg302153 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2017-09-14 07:35
New changeset dae0276bb6bc7281d59fb0b8f1aab31634ee80dc by Eric Snow in branch 'master':
bpo-30860: Fix a refleak. (#3567)
https://github.com/python/cpython/commit/dae0276bb6bc7281d59fb0b8f1aab31634ee80dc
msg302216 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2017-09-14 21:48
New changeset ccb3c7654cc3d031fb93bf443a6ef9cfb11f6b43 by Victor Stinner in branch 'master':
bpo-30860: Fix deadcode in obmalloc.c (#3499)
https://github.com/python/cpython/commit/ccb3c7654cc3d031fb93bf443a6ef9cfb11f6b43
History
Date User Action Args
2017-09-14 23:57:39eric.snowsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2017-09-14 21:48:39vstinnersetmessages: + msg302216
2017-09-14 07:36:00eric.snowsetmessages: + msg302153
2017-09-14 07:15:42eric.snowsetpull_requests: + pull_request3556
2017-09-12 01:30:46eric.snowsetmessages: + msg301935
2017-09-12 00:59:24vstinnersetmessages: + msg301931
2017-09-12 00:02:29eric.snowsetmessages: + msg301927
2017-09-11 23:08:44eric.snowsetpull_requests: + pull_request3501
2017-09-11 22:54:11eric.snowsetpull_requests: + pull_request3499
2017-09-11 16:34:38vstinnersetmessages: + msg301885
2017-09-11 16:31:25vstinnersetmessages: + msg301884
2017-09-11 15:09:18vstinnersetpull_requests: + pull_request3493
2017-09-11 15:04:08vstinnersetpull_requests: + pull_request3492
2017-09-08 21:27:12eric.snowsetmessages: + msg301735
2017-09-08 17:37:06eric.snowsetkeywords: + patch
pull_requests: + pull_request3452
2017-09-08 05:51:30eric.snowsetmessages: + msg301679
2017-09-07 17:58:21ncoghlansetmessages: + msg301599
2017-09-06 22:31:03eric.snowsetpull_requests: + pull_request3403
2017-09-06 04:43:11eric.snowsetmessages: + msg301438
2017-09-06 04:22:25eric.snowsetpull_requests: - pull_request3387
2017-09-06 04:14:37eric.snowsetpull_requests: + pull_request3388
2017-09-06 04:08:50eric.snowsetpull_requests: + pull_request3387
2017-09-06 03:37:55benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg301436
2017-09-06 03:06:58vstinnersetmessages: + msg301435
2017-09-06 01:26:18eric.snowsetmessages: + msg301428
2017-07-07 01:59:22ncoghlansetmessages: + msg297851
2017-07-06 22:08:39pitrousetnosy: + pitrou
messages: + msg297848
2017-07-06 13:54:01jklothsetnosy: + jkloth
2017-07-06 03:42:10serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg297794
2017-07-05 21:12:49eric.snowsetmessages: + msg297782
2017-07-05 21:10:39eric.snowsetpull_requests: + pull_request2665
2017-07-05 20:30:30eric.snowcreate