Skip to content

gh-111968: Use per-thread slice_cache in free-threading #113972

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jan 15, 2024
Merged

gh-111968: Use per-thread slice_cache in free-threading #113972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b05b359
gh-111968: Use per-thread slice_cahce in free-threading
corona10 Jan 12, 2024
ac5cdb8
Address code review
corona10 Jan 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions Include/internal/pycore_freelist.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,10 +59,19 @@ struct _Py_float_state {
#endif
};

struct _Py_slice_state {
#ifdef WITH_FREELISTS
/* Using a cache is very effective since typically only a single slice is
created and then deleted again. */
PySliceObject *slice_cache;
#endif
};

typedef struct _Py_freelist_state {
struct _Py_float_state float_state;
struct _Py_tuple_state tuple_state;
struct _Py_list_state list_state;
struct _Py_slice_state slice_state;
} _PyFreeListState;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erlend-aasland @colesbury
IMO, we should rename _PyFreeListState if we consider slice_cache is not freelist :)
Do you have any good suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's basically a freelist of max-length 1 🤷

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, let's maintain the current naming. (Changing is quite stressful)


#ifdef __cplusplus
Expand Down
1 change: 1 addition & 0 deletions Include/internal/pycore_gc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ extern void _Py_ClearFreeLists(_PyFreeListState *state, int is_finalization);
extern void _PyTuple_ClearFreeList(_PyFreeListState *state, int is_finalization);
extern void _PyFloat_ClearFreeList(_PyFreeListState *state, int is_finalization);
extern void _PyList_ClearFreeList(_PyFreeListState *state, int is_finalization);
extern void _PySlice_ClearCache(_PyFreeListState *state);
extern void _PyDict_ClearFreeList(PyInterpreterState *interp);
extern void _PyAsyncGen_ClearFreeLists(PyInterpreterState *interp);
extern void _PyContext_ClearFreeList(PyInterpreterState *interp);
Expand Down
3 changes: 0 additions & 3 deletions Include/internal/pycore_interp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,9 +187,6 @@ struct _is {
struct _Py_long_state long_state;
struct _dtoa_state dtoa;
struct _py_func_state func_state;
/* Using a cache is very effective since typically only a single slice is
created and then deleted again. */
PySliceObject *slice_cache;

struct _Py_tuple_state tuple;
struct _Py_dict_state dict_state;
Expand Down
2 changes: 1 addition & 1 deletion Include/internal/pycore_sliceobject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ extern "C" {

/* runtime lifecycle */

extern void _PySlice_Fini(PyInterpreterState *);
extern void _PySlice_Fini(_PyFreeListState *);

extern PyObject *
_PyBuildSlice_ConsumeRefs(PyObject *start, PyObject *stop);
Expand Down
26 changes: 15 additions & 11 deletions Objects/sliceobject.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,16 +103,20 @@ PyObject _Py_EllipsisObject = _PyObject_HEAD_INIT(&PyEllipsis_Type);

/* Slice object implementation */


void _PySlice_Fini(PyInterpreterState *interp)
void _PySlice_ClearCache(_PyFreeListState *state)
{
PySliceObject *obj = interp->slice_cache;
PySliceObject *obj = state->slice_state.slice_cache;
if (obj != NULL) {
interp->slice_cache = NULL;
state->slice_state.slice_cache = NULL;
PyObject_GC_Del(obj);
}
}

void _PySlice_Fini(_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.

This is identical to _PySlice_ClearCache. Do we need both functions? If you want to keep both names, maybe have one call the other.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep both names; I don't want to confuse people who work on the default build.

{
_PySlice_ClearCache(state);
}

/* start, stop, and step are python objects with None indicating no
index is present.
*/
Expand All @@ -122,11 +126,11 @@ _PyBuildSlice_Consume2(PyObject *start, PyObject *stop, PyObject *step)
{
assert(start != NULL && stop != NULL && step != NULL);

PyInterpreterState *interp = _PyInterpreterState_GET();
_PyFreeListState *state = _PyFreeListState_GET();
PySliceObject *obj;
if (interp->slice_cache != NULL) {
obj = interp->slice_cache;
interp->slice_cache = NULL;
if (state->slice_state.slice_cache != NULL) {
obj = state->slice_state.slice_cache;
state->slice_state.slice_cache = NULL;
_Py_NewReference((PyObject *)obj);
}
else {
Expand Down Expand Up @@ -354,13 +358,13 @@ Create a slice object. This is used for extended slicing (e.g. a[0:10:2]).");
static void
slice_dealloc(PySliceObject *r)
{
PyInterpreterState *interp = _PyInterpreterState_GET();
_PyFreeListState *state = _PyFreeListState_GET();
_PyObject_GC_UNTRACK(r);
Py_DECREF(r->step);
Py_DECREF(r->start);
Py_DECREF(r->stop);
if (interp->slice_cache == NULL) {
interp->slice_cache = r;
if (state->slice_state.slice_cache == NULL) {
state->slice_state.slice_cache = r;
}
else {
PyObject_GC_Del(r);
Expand Down
4 changes: 1 addition & 3 deletions Python/pylifecycle.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1752,15 +1752,13 @@ finalize_interp_types(PyInterpreterState *interp)
_PyUnicode_ClearInterned(interp);

_PyDict_Fini(interp);

_PySlice_Fini(interp);

_PyUnicode_Fini(interp);

_PyFreeListState *state = _PyFreeListState_GET();
_PyTuple_Fini(state);
_PyList_Fini(state);
_PyFloat_Fini(state);
_PySlice_Fini(state);

#ifdef Py_DEBUG
_PyStaticObjects_CheckRefcnt(interp);
Expand Down
1 change: 1 addition & 0 deletions Python/pystate.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1549,6 +1549,7 @@ PyThreadState_Clear(PyThreadState *tstate)
// Each thread should clear own freelists in free-threading builds.
_PyFreeListState *freelist_state = &((_PyThreadStateImpl*)tstate)->freelist_state;
_Py_ClearFreeLists(freelist_state, 0);
_PySlice_ClearCache(freelist_state);
#endif

_PyThreadState_ClearMimallocHeaps(tstate);
Expand Down