Skip to content
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

Merged
merged 54 commits into from Jan 9, 2024

Conversation

corona10
Copy link
Member

@corona10 corona10 commented Dec 30, 2023

@corona10
Copy link
Member Author

corona10 commented Dec 30, 2023

  • This PR introduces _PyFreeListState and a _PyFreeListState_GET function to seamlessly manage free-lists for 2 versions of CPython (default / free-threading).
    • Runtime authors do not need to consider where the free list is positioned. (Except for gc author ;) )
  • Zero impact change on the default CPython build
    • It will be managed as per-interpreter free list as same as before.
  • This PR is modified commit 88ec919 of @colesbury
  • Only apply for the free-list of list as the minimal work (the rest of things will be changed with separated PRs).

@corona10 corona10 marked this pull request as ready for review December 30, 2023 15:47
@corona10 corona10 changed the title gh-111968: Introduce _Py_freelist_state and _PyFreeListState_GET API gh-111968: Introduce Py_freelist_state and _PyFreeListState_GET API Dec 30, 2023
@corona10 corona10 changed the title gh-111968: Introduce Py_freelist_state and _PyFreeListState_GET API gh-111968: Introduce PyFreeListState and _PyFreeListState_GET API Dec 30, 2023
Modules/gcmodule.c Outdated Show resolved Hide resolved
@@ -1062,20 +1062,38 @@ delete_garbage(PyThreadState *tstate, GCState *gcstate,
}
}

static void
clear_freelists(PyFreeListState *state)
{
Copy link
Member Author

@corona10 corona10 Dec 31, 2023

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

@@ -1727,6 +1727,11 @@ flush_std_files(void)

*/

static void
finalize_free_lists(PyFreeListState *state)
Copy link
Member Author

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

Modules/gcmodule.c Outdated Show resolved Hide resolved
@corona10
Copy link
Member Author

@pablogsal @colesbury
This PR is now ready to be reviewed :)
Please take a look, and happy new free-threaded year!!

@corona10 corona10 changed the title gh-111968: Introduce PyFreeListState and _PyFreeListState_GET API [WIP] gh-111968: Introduce PyFreeListState and _PyFreeListState_GET API Jan 2, 2024
@corona10 corona10 changed the title [WIP] gh-111968: Introduce PyFreeListState and _PyFreeListState_GET API [WIP] gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API Jan 2, 2024
@corona10 corona10 changed the title [WIP] gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API Jan 5, 2024
Makefile.pre.in Outdated Show resolved Hide resolved
@corona10
Copy link
Member Author

corona10 commented Jan 6, 2024

@colesbury @pablogsal PTAL

@@ -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);
Copy link
Member

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.

Copy link
Contributor

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);
Copy link
Member

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?

Copy link
Contributor

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.

cpython/Python/pystate.c

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.

Copy link
Contributor

@colesbury colesbury left a 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);
Copy link
Contributor

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.

cpython/Python/pystate.c

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.

Include/internal/pycore_gc.h Outdated Show resolved Hide resolved
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);
Copy link
Contributor

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.

Copy link
Member Author

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

// list_dealloc() must not be called after _PyList_Fini()
assert(state->numfree != -1);

If we mark it at the PyThreadState_Clear phase.

@@ -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);
Copy link
Contributor

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.

@corona10 corona10 changed the title gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API [WIP] gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API Jan 9, 2024
@corona10 corona10 changed the title [WIP] gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API Jan 9, 2024
@corona10
Copy link
Member Author

corona10 commented Jan 9, 2024

For example, _Py_ClearFreeLists() is declared in pycore_gc.h but defined in pystate.c.

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 :)

@corona10 corona10 merged commit 57bdc6c into python:main Jan 9, 2024
30 checks passed
@corona10 corona10 deleted the gh-111968 branch January 9, 2024 23:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants