Skip to content

/cpython

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

bpo-36854: Move GC runtime state from _PyRuntimeState to PyInterpreterState. #13219

Open
wants to merge 3 commits into
base: master
from
+94 −45

Conversation

Reviewers

@vstinner vstinner

@encukou encukou

@ncoghlan ncoghlan

@warsaw warsaw

@pablogsal pablogsal

Assignees
No one assigned
Labels
CLA signed awaiting changes
Projects
None yet
Milestone
No milestone
5 participants
@ericsnowcurrently @bedevere-bot @csabella @vstinner @the-knights-who-say-ni
@ericsnowcurrently
Copy link
Member

commented May 9, 2019 •
edited by bedevere-bot

Note: as part of this PR, I've made sure that Python threads (created by the threading module) are marked as "deleted" earlier in finalization. That marker is how joining threads get unblocked. It was happening in PyThreadState_Delete() but now it will happen in PyThreadState_Clear(). This is necessary to ensure that the callback gets called before much interpreter/runtime finalization happens. (It could impact daemon threads, but we already can't guarantee behavior for those once finalization starts.)

https://bugs.python.org/issue36854

@ericsnowcurrently ericsnowcurrently requested review from brettcannon, encukou, ncoghlan and warsaw as code owners May 9, 2019

@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 9, 2019

@bedevere-bot bedevere-bot added the awaiting core review label May 9, 2019

@pablogsal pablogsal requested review from vstinner and pablogsal May 10, 2019

@ericsnowcurrently ericsnowcurrently force-pushed the ericsnowcurrently:runtime-interp-gc branch from 4bdd07f to 1607996 May 10, 2019

@ericsnowcurrently ericsnowcurrently referenced this pull request May 10, 2019

Merged

bpo-36818: Add PyInterpreterState.runtime field. #13129

@vstinner

vstinner requested changes May 10, 2019
View changes

Copy link
Member

left a comment

I like what I see: -) But I have some comments on the actual implementation.
Python/pystate.c
// This will unblock any joining threads.
tstate->on_delete(tstate->on_delete_data);
tstate->on_delete = NULL;
tstate->on_delete_data = NULL;

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner May 10, 2019

Member

This change looks to be unrelated. If it's related, I would prefer to see it as a separated PR to prepare that one.

Same comment for other thread changes:

    // All threading module threads are marked as "done" later
    // in PyThreadState_Clear().

and

        // This will unblock any joining threads.
        // We also do this in PyThreadState_Clear(), but do it here to be sure.
Include/internal/pycore_object.h
@@ -38,11 +38,12 @@ static inline void _PyObject_GC_TRACK_impl(const char *filename, int lineno,
"object is in generation which is garbage collected",
filename, lineno, "_PyObject_GC_TRACK");

PyGC_Head *last = (PyGC_Head*)(_PyRuntime.gc.generation0->_gc_prev);
PyInterpreterState *interp = _PyInterpreterState_GET_UNSAFE();

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner May 10, 2019

Member

Would it be possible to use a "struct _gc_runtime_state *state" variable instead?

struct _gc_runtime_state *state = &_PyInterpreterState_GET_UNSAFE()->gc;
Modules/gcmodule.c
@@ -1257,7 +1257,8 @@ static PyObject *
gc_enable_impl(PyObject *module)
/*[clinic end generated code: output=45a427e9dce9155c input=81ac4940ca579707]*/
{
_PyRuntime.gc.enabled = 1;
PyInterpreterState *interp = _PyInterpreterState_GET_UNSAFE();

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner May 10, 2019

Member

Would it be possible to use a "struct _gc_runtime_state *state" variable instead?

struct _gc_runtime_state *state = &_PyInterpreterState_GET_UNSAFE()->gc;

If the line is too lone, maybe add a GET_STATE() macro which returns &_PyInterpreterState_GET_UNSAFE()->gc.

Same comment for the whole file. I would prefer to avoid introducing "PyInterpreterState *interp = _PyInterpreterState_GET_UNSAFE(); " lines if it's only used to get the GC state.
Objects/object.c
@@ -2074,11 +2075,12 @@ _PyTrash_thread_deposit_object(PyObject *op)
void
_PyTrash_destroy_chain(void)
{
while (_PyRuntime.gc.trash_delete_later) {
PyObject *op = _PyRuntime.gc.trash_delete_later;
PyInterpreterState *interp = _PyInterpreterState_GET_UNSAFE();

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner May 10, 2019

Member

Again, a "state" variable would be more appropriate here.
Python/pylifecycle.c
@@ -511,6 +511,8 @@ pycore_create_interpreter(_PyRuntimeState *runtime,
}
*interp_p = interp;

_PyGC_Initialize(&interp->gc);

This comment has been minimized.

Sign in to view
Copy link
@vstinner

vstinner May 10, 2019

Member

Hum, maybe _PyGC_Initialize() should take interp argument instead, to be consistent with other _PyGC functions.

@bedevere-bot bedevere-bot added awaiting changes and removed awaiting core review labels May 10, 2019

@bedevere-bot

This comment has been minimized.

Sign in to view
Copy link

commented May 10, 2019

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
@csabella

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented May 31, 2019

@ericsnowcurrently, were you wanting to land this for 3.8?

@brettcannon brettcannon removed their request for review May 31, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.