Skip to content

gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API#113584

Merged
corona10 merged 54 commits into
python:mainfrom
corona10:gh-111968
Jan 9, 2024
Merged

gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API#113584
corona10 merged 54 commits into
python:mainfrom
corona10:gh-111968

Conversation

@corona10

@corona10 corona10 commented Dec 30, 2023

Copy link
Copy Markdown
Member

@corona10

corona10 commented Dec 30, 2023

Copy link
Copy Markdown
Member Author
  • 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 requested review from DinoV and colesbury December 30, 2023 15:47
@corona10
corona10 marked this pull request as ready for review December 30, 2023 15:47
@corona10
corona10 requested a review from pablogsal as a code owner 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
Comment thread Modules/gcmodule.c Outdated
Comment thread Modules/gcmodule.c Outdated

static void
clear_freelists(PyFreeListState *state)
{

@corona10 corona10 Dec 31, 2023

Copy link
Copy Markdown
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.

  • _PyTuple_ClearFreeList
  • _PyFloat_ClearFreeList
  • _PyDict_ClearFreeList
  • _PyAsyncGen_ClearFreeLists
  • _PyContext_ClearFreeList

Comment thread Python/pylifecycle.c Outdated
*/

static void
finalize_free_lists(PyFreeListState *state)

Copy link
Copy Markdown
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

Comment thread Modules/gcmodule.c Outdated
@corona10

Copy link
Copy Markdown
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
Comment thread Makefile.pre.in Outdated
@corona10

corona10 commented Jan 6, 2024

Copy link
Copy Markdown
Member Author

@colesbury @pablogsal PTAL

Comment thread Include/internal/pycore_gc.h Outdated

// Functions to clear types free lists
extern void _PyGC_ClearFreeList(PyInterpreterState *interp);
extern void _Py_ClearFreeLists(_PyFreeListState *state);

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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.

Comment thread Include/internal/pycore_gc.h Outdated
Comment thread 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
Copy Markdown
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
Copy Markdown
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.

Comment thread Include/internal/pycore_gc.h Outdated

// Functions to clear types free lists
extern void _PyGC_ClearFreeList(PyInterpreterState *interp);
extern void _Py_ClearFreeLists(_PyFreeListState *state);

Copy link
Copy Markdown
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

corona10 commented Jan 9, 2024

Copy link
Copy Markdown
Member Author

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 requested a review from colesbury January 9, 2024 15:21
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.

4 participants