bpo-36854: Move GC runtime state from _PyRuntimeState to PyInterpreterState. by ericsnowcurrently · Pull Request #13219 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation11 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

Choose a reason for hiding this comment

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

I like what I see: -) But I have some comments on the actual implementation.

// This will unblock any joining threads.
tstate->on_delete(tstate->on_delete_data);
tstate->on_delete = NULL;
tstate->on_delete_data = NULL;

Choose a reason for hiding this comment

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

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.

@@ -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();

Choose a reason for hiding this comment

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

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

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

@@ -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();

Choose a reason for hiding this comment

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

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.

@@ -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();

Choose a reason for hiding this comment

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

Again, a "state" variable would be more appropriate here.

@@ -511,6 +511,8 @@ pycore_create_interpreter(_PyRuntimeState *runtime,
}
*interp_p = interp;

_PyGC_Initialize(&interp->gc);

Choose a reason for hiding this comment

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

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

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

I wrote a different change, PR #17287, that I just merged. So I close this old PR. Thanks anyway Eric for working on this, my work is partially based on yours ;-)

Thanks for doing it!!!

You're welcome.