gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API #113584

corona10 · 2023-12-30T14:56:41Z

Issue: Use per-thread freelists in --disable-gil builds #111968

…T API

corona10 · 2023-12-30T15:03:55Z

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

Modules/gcmodule.c

corona10 · 2023-12-31T04:32:12Z

Modules/gcmodule.c

@@ -1062,20 +1062,38 @@ delete_garbage(PyThreadState *tstate, GCState *gcstate,
    }
 }

+static void
+clear_freelists(PyFreeListState *state)
+{


Following functions will be moved here in the end.

_PyTuple_ClearFreeList

_PyFloat_ClearFreeList

_PyDict_ClearFreeList

_PyAsyncGen_ClearFreeLists

_PyContext_ClearFreeList

corona10 · 2023-12-31T05:09:59Z

Python/pylifecycle.c

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

 */

+static void
+finalize_free_lists(PyFreeListState *state)


Following functions will be moved here in the end.

_PyDict_Fini

_PyTuple_Fini

_PySlice_Fini

_PyFloat_Fini

_PyContext_Fini

Modules/gcmodule.c

corona10 · 2023-12-31T05:57:39Z

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

Makefile.pre.in

corona10 · 2024-01-06T00:45:34Z

@colesbury @pablogsal PTAL

pablogsal · 2024-01-08T16:18:20Z

Include/internal/pycore_gc.h

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


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.

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.

pablogsal · 2024-01-08T16:23:43Z

Python/gc_free_threading.c

+    _PyAsyncGen_ClearFreeLists(interp);
+    _PyContext_ClearFreeList(interp);
+
+    HEAD_LOCK(&_PyRuntime);


This locks interpreters creation, but not thread creation, no? Is it possible that this races with thread creation/destruction? I am missing something?

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

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

colesbury · 2024-01-08T17:28:20Z

Python/gc_free_threading.c

+    _PyAsyncGen_ClearFreeLists(interp);
+    _PyContext_ClearFreeList(interp);
+
+    HEAD_LOCK(&_PyRuntime);


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

colesbury · 2024-01-08T17:51:21Z

Python/pystate.c

+#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);


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.

@colesbury We may need to change the assertion of

cpython/Objects/listobject.c

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.

colesbury · 2024-01-08T18:01:40Z

Include/internal/pycore_gc.h

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


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 · 2024-01-09T01:52:44Z

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 added the skip news label Dec 30, 2023

bedevere-app bot mentioned this pull request Dec 30, 2023

Use per-thread freelists in --disable-gil builds #111968

Open

corona10 added DO-NOT-MERGE topic-free-threading labels Dec 30, 2023

pythongh-111968: Introduce _Py_freelist_state and _PyFreeListState_GE…

cd4863c

…T API

corona10 force-pushed the gh-111968 branch from 6acf43d to cd4863c Compare December 30, 2023 15:00

corona10 added 3 commits December 31, 2023 00:06

nit

333873d

Remove comment

8b7e261

Update _PyList_Fini

dd0afa2

corona10 removed the DO-NOT-MERGE label Dec 30, 2023

corona10 requested review from DinoV and colesbury December 30, 2023 15:47

corona10 marked this pull request as ready for review December 30, 2023 15:47

corona10 requested a review from pablogsal as a code owner December 30, 2023 15:47

bedevere-app bot added the awaiting core review label Dec 30, 2023

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 changed the title ~~gh-111968: Introduce Py_freelist_state and _PyFreeListState_GET API~~ gh-111968: Introduce PyFreeListState and _PyFreeListState_GET API Dec 30, 2023

corona10 commented Dec 31, 2023

View reviewed changes

Modules/gcmodule.c Outdated Show resolved Hide resolved

corona10 added 2 commits December 31, 2023 13:28

nit refactor

f0d55de

fix

5e8e3b4

corona10 commented Dec 31, 2023

View reviewed changes

Update finalize step

7aa956a

corona10 commented Dec 31, 2023

View reviewed changes

Modules/gcmodule.c Outdated Show resolved Hide resolved

pep7

908ef13

Update

0096383

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 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 added 4 commits January 6, 2024 08:29

nit

458aadb

nit

b105bda

fix

18e9216

nit

5bb8d3f

corona10 changed the title ~~[WIP] gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API~~ gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API Jan 5, 2024

erlend-aasland reviewed Jan 5, 2024

View reviewed changes

Makefile.pre.in Outdated Show resolved Hide resolved

corona10 added 2 commits January 6, 2024 09:21

fix

2811a12

fix compiler warn

a5f494d

corona10 added 3 commits January 6, 2024 09:54

nit

837ae60

fix

08c8613

Adjust comment

b5eb472

corona10 force-pushed the gh-111968 branch from 3dc6ee5 to b5eb472 Compare January 6, 2024 04:31

2 tabs, not 8 spaces

0d8dc3d

pablogsal reviewed Jan 8, 2024

View reviewed changes

colesbury reviewed Jan 8, 2024

View reviewed changes

corona10 added 5 commits January 9, 2024 09:18

Rename to _PyGC_ClearAllFreeLists

fe859e1

Pass is_finalization to _PyList_ClearFreeList

c284061

fix

a8e39b3

fix

99ac375

fix

0c331c7

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 changed the title ~~[WIP] gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API~~ gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API Jan 9, 2024

corona10 requested a review from colesbury January 9, 2024 15:21

colesbury approved these changes Jan 9, 2024

View reviewed changes

corona10 merged commit 57bdc6c into python:main Jan 9, 2024
30 checks passed

bedevere-app bot removed the awaiting core review label Jan 9, 2024

corona10 deleted the gh-111968 branch January 9, 2024 23:04

gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API #113584

gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API #113584

corona10 commented Dec 30, 2023 •

edited by bedevere-app bot

corona10 commented Dec 30, 2023 •

edited

corona10 Dec 31, 2023 •

edited

corona10 Dec 31, 2023

corona10 commented Dec 31, 2023

corona10 commented Jan 6, 2024

pablogsal Jan 8, 2024

colesbury Jan 8, 2024

pablogsal Jan 8, 2024

colesbury Jan 8, 2024

colesbury left a comment

colesbury Jan 8, 2024

colesbury Jan 8, 2024

corona10 Jan 9, 2024

colesbury Jan 8, 2024

corona10 commented Jan 9, 2024

	/* We serialize concurrent creation to protect global state. */
	HEAD_LOCK(runtime);

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

gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API #113584

gh-111968: Introduce _PyFreeListState and _PyFreeListState_GET API #113584

Conversation

corona10 commented Dec 30, 2023 • edited by bedevere-app bot

corona10 commented Dec 30, 2023 • edited

corona10 Dec 31, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corona10 commented Dec 31, 2023

corona10 commented Jan 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colesbury left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

corona10 commented Jan 9, 2024

corona10 commented Dec 30, 2023 •

edited by bedevere-app bot

corona10 commented Dec 30, 2023 •

edited

corona10 Dec 31, 2023 •

edited