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
gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API #113584
Conversation
|
Modules/gcmodule.c
Outdated
@@ -1062,20 +1062,38 @@ delete_garbage(PyThreadState *tstate, GCState *gcstate, | |||
} | |||
} | |||
|
|||
static void | |||
clear_freelists(PyFreeListState *state) | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following functions will be moved here in the end.
- _PyTuple_ClearFreeList
- _PyFloat_ClearFreeList
- _PyDict_ClearFreeList
- _PyAsyncGen_ClearFreeLists
- _PyContext_ClearFreeList
Python/pylifecycle.c
Outdated
@@ -1727,6 +1727,11 @@ flush_std_files(void) | |||
|
|||
*/ | |||
|
|||
static void | |||
finalize_free_lists(PyFreeListState *state) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following functions will be moved here in the end.
- _PyDict_Fini
- _PyTuple_Fini
- _PySlice_Fini
- _PyFloat_Fini
- _PyContext_Fini
@pablogsal @colesbury |
@colesbury @pablogsal PTAL |
Include/internal/pycore_gc.h
Outdated
@@ -238,9 +240,11 @@ extern PyObject *_PyGC_GetObjects(PyInterpreterState *interp, Py_ssize_t generat | |||
extern PyObject *_PyGC_GetReferrers(PyInterpreterState *interp, PyObject *objs); | |||
|
|||
// Functions to clear types free lists | |||
extern void _PyGC_ClearFreeList(PyInterpreterState *interp); | |||
extern void _Py_ClearFreeLists(_PyFreeListState *state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hummm, I find this asymmetry a bit odd. Wouldn't be better to keep all functions receiving PyInterpreterState *interp
and then fetching the freelist state from there instead?
That way the code of the caller will be also cleaner and doesn't need to deal with where the freelist is stored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the free-threaded builds, we want to clear the free-lists for a specific thread (not all threads). Passing interp
doesn't provide enough information. In some cases the thread we are clearing is not even the active thread, such as when we call PyThreadState_Clear()
on dead threads after fork.
_PyAsyncGen_ClearFreeLists(interp); | ||
_PyContext_ClearFreeList(interp); | ||
|
||
HEAD_LOCK(&_PyRuntime); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This locks interpreters creation, but not thread creation, no? Is it possible that this races with thread creation/destruction? I am missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part looks okay to me. The HEAD_LOCK()
protects the threads linked list (among other things). Threads acquire the HEAD_LOCK()
when appending (or removing) themselves to that list.
Lines 1370 to 1371 in ace4d7f
/* We serialize concurrent creation to protect global state. */ | |
HEAD_LOCK(runtime); |
Modification of the free-lists themselves is protected by the "stop-the-world" pause (or the GIL), which is not yet implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks good to me.
I find the code organization a bit confusing: we have free list handling split across a lot of files. The declarations of some functions are in headers that don't correspond to the files where those functions are defined. For example, _Py_ClearFreeLists()
is declared in pycore_gc.h
but defined in pystate.c
.
I think it'll be worth doing a refactor/reorganization after more functions are converted to this pattern. (I'm not sure it's worth doing now.)
_PyAsyncGen_ClearFreeLists(interp); | ||
_PyContext_ClearFreeList(interp); | ||
|
||
HEAD_LOCK(&_PyRuntime); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part looks okay to me. The HEAD_LOCK()
protects the threads linked list (among other things). Threads acquire the HEAD_LOCK()
when appending (or removing) themselves to that list.
Lines 1370 to 1371 in ace4d7f
/* We serialize concurrent creation to protect global state. */ | |
HEAD_LOCK(runtime); |
Modification of the free-lists themselves is protected by the "stop-the-world" pause (or the GIL), which is not yet implemented.
Python/pystate.c
Outdated
#ifdef Py_GIL_DISABLED | ||
// Each thread should clear own freelists in free-threading builds. | ||
_PyFreeListState *freelist_state = &((_PyThreadStateImpl*)tstate)->freelist_state; | ||
_Py_ClearFreeLists(freelist_state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we pass something like int is_finalization
as an argument to _Py_ClearFreeLists()
? I am concerned about the issue that _PyList_Fini()
attempts to detect by setting num_free=-1
: allocating objects after the free list is cleared for the final time. We want that error detection here.
Something like:
_Py_ClearFreeLists(freelist_state, 1);
and in the code clearing free lists
_PyList_ClearFreeList(_PyFreeListState *freelist_state, int is_finalization)
{
....
if (is_finalization) {
state->numfree = -1;
}
}
Along those lines, I think we should move the _Py_ClearFreeLists()
call after the on_delete()
call since, on_delete()
can free some Python objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@colesbury We may need to change the assertion of
Lines 366 to 367 in a023bc2
// list_dealloc() must not be called after _PyList_Fini() | |
assert(state->numfree != -1); |
If we mark it at the PyThreadState_Clear phase.
Include/internal/pycore_gc.h
Outdated
@@ -238,9 +240,11 @@ extern PyObject *_PyGC_GetObjects(PyInterpreterState *interp, Py_ssize_t generat | |||
extern PyObject *_PyGC_GetReferrers(PyInterpreterState *interp, PyObject *objs); | |||
|
|||
// Functions to clear types free lists | |||
extern void _PyGC_ClearFreeList(PyInterpreterState *interp); | |||
extern void _Py_ClearFreeLists(_PyFreeListState *state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the free-threaded builds, we want to clear the free-lists for a specific thread (not all threads). Passing interp
doesn't provide enough information. In some cases the thread we are clearing is not even the active thread, such as when we call PyThreadState_Clear()
on dead threads after fork.
It's due to the complex of dependency of headers once we move all domains into pycore_freelist.h, we can clean up the location issue. Let's handle it separately :) |
--disable-gil
builds #111968