bpo-36854: Move GC runtime state from _PyRuntimeState to PyInterpreterState. - #13219
bpo-36854: Move GC runtime state from _PyRuntimeState to PyInterpreterState.#13219ericsnowcurrently wants to merge 3 commits into
Conversation
Sorry, something went wrong.
vstinner
left a comment
There was a problem hiding this comment.
I like what I see: -) But I have some comments on the actual implementation.
Sorry, something went wrong.
| // This will unblock any joining threads. | ||
| tstate->on_delete(tstate->on_delete_data); | ||
| tstate->on_delete = NULL; | ||
| tstate->on_delete_data = NULL; |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| filename, lineno, "_PyObject_GC_TRACK"); | ||
|
|
||
| PyGC_Head *last = (PyGC_Head*)(_PyRuntime.gc.generation0->_gc_prev); | ||
| PyInterpreterState *interp = _PyInterpreterState_GET_UNSAFE(); |
There was a problem hiding this comment.
Would it be possible to use a "struct _gc_runtime_state *state" variable instead?
struct _gc_runtime_state *state = &_PyInterpreterState_GET_UNSAFE()->gc;
Sorry, something went wrong.
| /*[clinic end generated code: output=45a427e9dce9155c input=81ac4940ca579707]*/ | ||
| { | ||
| _PyRuntime.gc.enabled = 1; | ||
| PyInterpreterState *interp = _PyInterpreterState_GET_UNSAFE(); |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| { | ||
| while (_PyRuntime.gc.trash_delete_later) { | ||
| PyObject *op = _PyRuntime.gc.trash_delete_later; | ||
| PyInterpreterState *interp = _PyInterpreterState_GET_UNSAFE(); |
There was a problem hiding this comment.
Again, a "state" variable would be more appropriate here.
Sorry, something went wrong.
| } | ||
| *interp_p = interp; | ||
|
|
||
| _PyGC_Initialize(&interp->gc); |
There was a problem hiding this comment.
Hum, maybe _PyGC_Initialize() should take interp argument instead, to be consistent with other _PyGC functions.
Sorry, something went wrong.
|
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again. |
Sorry, something went wrong.
|
@ericsnowcurrently, were you wanting to land this for 3.8? |
Sorry, something went wrong.
|
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 ;-) |
Sorry, something went wrong.
|
Thanks for doing it!!! |
Sorry, something went wrong.
You're welcome. |
Sorry, something went wrong.
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