Skip to content

gh-111968: Use per-thread freelists for float in free-threading#113886

Merged
corona10 merged 4 commits into
python:mainfrom
corona10:gh-111968-float
Jan 10, 2024
Merged

gh-111968: Use per-thread freelists for float in free-threading#113886
corona10 merged 4 commits into
python:mainfrom
corona10:gh-111968-float

Conversation

@corona10

@corona10 corona10 commented Jan 10, 2024

Copy link
Copy Markdown
Member

Comment thread Python/gc_gil.c
_PyGC_ClearAllFreeLists(PyInterpreterState *interp)
{
_PyTuple_ClearFreeList(interp);
_PyFloat_ClearFreeList(interp);

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.

Integrated with _Py_ClearFreeLists

_PyGC_ClearAllFreeLists(PyInterpreterState *interp)
{
_PyTuple_ClearFreeList(interp);
_PyFloat_ClearFreeList(interp);

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.

Integrated with _Py_ClearFreeLists

};

typedef struct _Py_freelist_state {
struct _Py_float_state float_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.

Why not use the same naming scheme? The list freelist state member is named list.

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.

Because float is reserved name?

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.

If we decide to change the naming schme, list should be changed. WDYT?

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.

I think that the naming of a struct and its members is an important part of API design. Having a consistent scheme makes it easier to read and review code. I would suggest use a naming scheme for the freelist struct and its members. Up to you :)

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.

Totally agree, I will create a seperate PR :)

Comment thread Include/internal/pycore_freelist.h Outdated
Comment thread Objects/floatobject.c
}
state->free_list = NULL;
state->numfree = 0;
if (is_finalization) {

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.

Previously, we only set num_free to -1 during finalization in debug builds. With this change, we set it in non-debug builds too. I think that's fine (and simpler), but we should better handle the -1 case in PyFloat_ExactDealloc in case assertions are not enabled.

I'd suggest adding changing the condition in PyFloat_ExactDealloc to something like if (state->num_free >= PyFloat_MAXFREELIST || state->num_free < 0) so that num_free=-1 disables the free-list.

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.

Updated!

@corona10
corona10 requested a review from colesbury January 10, 2024 15:09

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

lgtm

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.

3 participants