◐ Shell
reader mode source ↗
Skip to content

bpo-30860: Consolidate stateful runtime globals.#2594

Merged
ericsnowcurrently merged 78 commits into
python:masterfrom
ericsnowcurrently:interp-globals
Sep 6, 2017
Merged

bpo-30860: Consolidate stateful runtime globals.#2594
ericsnowcurrently merged 78 commits into
python:masterfrom
ericsnowcurrently:interp-globals

Conversation

@ericsnowcurrently

@ericsnowcurrently ericsnowcurrently commented Jul 5, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member
  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

Other globals are excluded (see globals.txt and check-c-globals.py).

https://bugs.python.org/issue30860

@ericsnowcurrently ericsnowcurrently requested a review from 1st1 July 5, 2017 21:10
@1st1

1st1 commented Jul 5, 2017

Copy link
Copy Markdown
Member

An obvious nit: there are a lot of lines that are longer than 79 chars. Those will need to be fixed per PEP 7.

@ericsnowcurrently

ericsnowcurrently commented Jul 6, 2017

Copy link
Copy Markdown
Member Author

FYI, there are test failures in test_capi, test_warnings, and test_tracemalloc. I'll address those tomorrow, but I expect the fixes won't require many changes.

ncoghlan
ncoghlan reviewed Jul 6, 2017
View reviewed changes

@ncoghlan ncoghlan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

I still like the general idea, but the way the struct definitions are currently handling Py_BUILD_CORE doesn't look right to me.

@ericsnowcurrently ericsnowcurrently force-pushed the interp-globals branch 5 times, most recently from 4d9153f to d2d58ef Compare July 11, 2017 20:33
@ericsnowcurrently

ericsnowcurrently commented Jul 12, 2017

Copy link
Copy Markdown
Member Author

@zooba, any thoughts on the Windows build failure here? I'm sure it's something trivial.

@zooba

zooba commented Jul 14, 2017

Copy link
Copy Markdown
Member

@ericsnowcurrently Not sure that uint is a standard C type - I thought they were uint#_t, where # is the bit length. unsigned int or uint32_t should be okay.

@ericsnowcurrently

ericsnowcurrently commented Jul 18, 2017

Copy link
Copy Markdown
Member Author

FYI, the _Py_CheckRecursionLimit change causes a performance regression on the pickle_list and pickle_dict benchmarks. When I get a chance I'll look into it.

@ericsnowcurrently ericsnowcurrently requested a review from a team September 5, 2017 20:53
@ericsnowcurrently ericsnowcurrently requested a review from a team as a code owner September 5, 2017 20:53
@the-knights-who-say-ni

the-knights-who-say-ni commented Sep 5, 2017

Copy link
Copy Markdown

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

73 hidden items Load more…
gpshead
gpshead approved these changes Sep 5, 2017
View reviewed changes

@gpshead gpshead left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hide comment

overall, awesome refactoring and great analysis of the global state!

@ericsnowcurrently ericsnowcurrently merged commit 76d5abc into python:master Sep 6, 2017
@ericsnowcurrently ericsnowcurrently deleted the interp-globals branch September 6, 2017 01:26
@ericsnowcurrently ericsnowcurrently restored the interp-globals branch September 6, 2017 03:59
ericsnowcurrently added a commit that referenced this pull request Sep 6, 2017
@ericsnowcurrently
Revert "bpo-30860: Consolidate stateful runtime globals. (#2594)" …
a086c10 
This reverts commit 76d5abc.
@ericsnowcurrently ericsnowcurrently mentioned this pull request Sep 6, 2017
Closed
ericsnowcurrently added a commit to ericsnowcurrently/cpython that referenced this pull request Sep 6, 2017
@ericsnowcurrently
Revert "bpo-30860: Consolidate stateful runtime globals. (python#2594)" …
baaacc1 
This reverts commit 76d5abc.
This was referenced Sep 6, 2017
Merged
Merged
GadgetSteve pushed a commit to GadgetSteve/cpython that referenced this pull request Sep 10, 2017
@ericsnowcurrently @GadgetSteve
bpo-30860: Consolidate stateful runtime globals. (python#2594) …
79267a1 
* group the (stateful) runtime globals into various topical structs
* consolidate the topical structs under a single top-level _PyRuntimeState struct
* add a check-c-globals.py script that helps identify runtime globals

Other globals are excluded (see globals.txt and check-c-globals.py).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ncoghlan ncoghlan ncoghlan left review comments

@zooba zooba zooba left review comments

@gpshead gpshead gpshead approved these changes

@1st1 1st1 Awaiting requested review from 1st1

@rhettinger rhettinger Awaiting requested review from rhettinger

@skrah skrah Awaiting requested review from skrah

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@ericsnowcurrently @1st1 @zooba @the-knights-who-say-ni @gpshead @ncoghlan @Mariatta @bedevere-bot