Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Consolidate stateful C globals under a single struct. #75043

Closed
ericsnowcurrently opened this issue Jul 5, 2017 · 20 comments
Closed

Consolidate stateful C globals under a single struct. #75043

ericsnowcurrently opened this issue Jul 5, 2017 · 20 comments
Assignees
@ericsnowcurrently
Labels
3.7 (EOL) end of life type-feature A feature request or enhancement

Comments

@ericsnowcurrently
Copy link
Member

BPO 30860
Nosy @ncoghlan, @pitrou, @vstinner, @benjaminp, @jkloth, @ericsnowcurrently, @serhiy-storchaka, @1st1
PRs
  • bpo-30860: Consolidate stateful runtime globals. #2594
  • Revert "bpo-30860: Consolidate stateful runtime globals." #3379
  • bpo-30860: Consolidate stateful runtime globals. #3397
  • bpo-30860: Move windows.h include out of internal/*.h. #3458
  • bpo-30860: Add Include/internal/ in "make tags" #3498
  • bpo-30860: Fix deadcode in obmalloc.c #3499
  • bpo-30860: Fix a refleak. #3506
  • bpo-30860: Always provide serialno. #3507
  • bpo-30860: Fix a refleak. #3567
  • bpo-32096: Remove obj and mem from _PyRuntime #4532
    		•

    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 = 'https://github.com/ericsnowcurrently'
closed_at = <Date 2017-09-14.23:57:39.144>
created_at = <Date 2017-07-05.20:30:30.280>
labels = ['type-feature', '3.7']
title = 'Consolidate stateful C globals under a single struct.'
updated_at = <Date 2017-11-24.11:09:30.985>
user = 'https://github.com/ericsnowcurrently'

    bugs.python.org fields:

    activity = <Date 2017-11-24.11:09:30.985>
actor = 'vstinner'
assignee = 'eric.snow'
closed = True
closed_date = <Date 2017-09-14.23:57:39.144>
closer = 'eric.snow'
components = []
creation = <Date 2017-07-05.20:30:30.280>
creator = 'eric.snow'
dependencies = []
files = []
hgrepos = []
issue_num = 30860
keywords = ['patch']
message_count = 20.0
messages = ['297780', '297782', '297794', '297848', '297851', '301428', '301435', '301436', '301438', '301599', '301679', '301735', '301884', '301885', '301927', '301931', '301935', '302153', '302216', '306888']
nosy_count = 8.0
nosy_names = ['ncoghlan', 'pitrou', 'vstinner', 'benjamin.peterson', 'jkloth', 'eric.snow', 'serhiy.storchaka', 'yselivanov']
pr_nums = ['2594', '3379', '3397', '3458', '3498', '3499', '3506', '3507', '3567', '4532']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue30860'
versions = ['Python 3.7']
    @ericsnowcurrently
    Copy link
    Member Author

    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.

    @ericsnowcurrently ericsnowcurrently added the 3.7 (EOL) end of life label Jul 5, 2017
    @ericsnowcurrently ericsnowcurrently self-assigned this Jul 5, 2017
    @ericsnowcurrently ericsnowcurrently added the type-feature A feature request or enhancement label Jul 5, 2017
    @ericsnowcurrently
    Copy link
    Member Author

    One thing I'd like to also try is to use a freelist embedded in _PyRuntimeState for the PyInterpreterState and PyThreadState linked lists.

    @serhiy-storchaka
    Copy link
    Member

    See also bpo-29881.

    @pitrou
    Copy link
    Member

    pitrou commented Jul 6, 2017

    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.

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Jul 7, 2017

    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)

    @ericsnowcurrently
    Copy link
    Member Author

    New changeset 76d5abc by Eric Snow in branch 'master':
    bpo-30860: Consolidate stateful runtime globals. (bpo-2594)
    76d5abc

    @vstinner
    Copy link
    Member

    vstinner commented Sep 6, 2017

    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

    @benjaminp
    Copy link
    Contributor

    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/...

    @ericsnowcurrently
    Copy link
    Member Author

    New changeset 05351c1 by Eric Snow in branch 'master':
    Revert "bpo-30860: Consolidate stateful runtime globals." (bpo-3379)
    05351c1

    @ncoghlan
    Copy link
    Contributor

    ncoghlan commented Sep 7, 2017

    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.

    @ericsnowcurrently
    Copy link
    Member Author

    New changeset 2ebc5ce by Eric Snow in branch 'master':
    bpo-30860: Consolidate stateful runtime globals. (bpo-3397)
    2ebc5ce

    @ericsnowcurrently
    Copy link
    Member Author

    FYI, the merged code introduced a bunch of new warnings on Windows. I'm looking into it.

    @vstinner
    Copy link
    Member

    The commit 2ebc5ce introduced reference leaks: see bpo-31420 which tracks them.

    @vstinner
    Copy link
    Member

    New changeset 4866957 by Victor Stinner in branch 'master':
    bpo-30860: Add Include/internal/ in "make tags" (bpo-3498)
    4866957

    @ericsnowcurrently
    Copy link
    Member Author

    New changeset ba6d5d1 by Eric Snow in branch 'master':
    bpo-30860: Always provide serialno. (bpo-3507)
    ba6d5d1

    @vstinner
    Copy link
    Member

    New changeset 8728018 by Victor Stinner (Eric Snow) in branch 'master':
    bpo-30860: Fix a refleak. (bpo-3506)
    8728018

    @ericsnowcurrently
    Copy link
    Member Author

    New changeset fc1bf87 by Eric Snow in branch 'master':
    bpo-30860: Move windows.h include out of internal/*.h. (bpo-3458)
    fc1bf87

    @ericsnowcurrently
    Copy link
    Member Author

    New changeset dae0276 by Eric Snow in branch 'master':
    bpo-30860: Fix a refleak. (bpo-3567)
    dae0276

    @vstinner
    Copy link
    Member

    New changeset ccb3c76 by Victor Stinner in branch 'master':
    bpo-30860: Fix deadcode in obmalloc.c (bpo-3499)
    ccb3c76

    @vstinner
    Copy link
    Member

    New changeset 9e87e77 by Victor Stinner in branch 'master':
    bpo-32096: Remove obj and mem from _PyRuntime (bpo-4532)
    9e87e77

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees

    @ericsnowcurrently ericsnowcurrently

    Labels
    3.7 (EOL) end of life type-feature A feature request or enhancement
    Projects
    None yet
    Milestone
    No milestone
    Development

    No branches or pull requests
    6 participants
    @vstinner @benjaminp @ncoghlan @ericsnowcurrently @pitrou @serhiy-storchaka