Skip to content

gh-129236: Use stackpointer in free threaded GC #129240

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 5 commits into from
Jan 29, 2025
Merged

gh-129236: Use stackpointer in free threaded GC #129240

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1ec055b
gh-129236: Use `stackpointer` in free threaded GC
colesbury Jan 23, 2025
fcaee80
Remove additional frame initialization
colesbury Jan 24, 2025
ca32c7b
Update Python/gc_free_threading.c
colesbury Jan 24, 2025
1c8a2e3
Update comment
colesbury Jan 24, 2025
57a34c2
Update comment and logic in mark_heap_visitor
colesbury Jan 24, 2025
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
7 changes: 0 additions & 7 deletions Include/internal/pycore_frame.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,13 +159,6 @@ static inline void _PyFrame_Copy(_PyInterpreterFrame *src, _PyInterpreterFrame *
// Don't leave a dangling pointer to the old frame when creating generators
// and coroutines:
dest->previous = NULL;

#ifdef Py_GIL_DISABLED
PyCodeObject *co = _PyFrame_GetCode(dest);
for (int i = stacktop; i < co->co_nlocalsplus + co->co_stacksize; i++) {
dest->localsplus[i] = PyStackRef_NULL;
}
#endif
}

#ifdef Py_GIL_DISABLED
Expand Down
78 changes: 50 additions & 28 deletions Python/gc_free_threading.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ struct collection_state {
PyInterpreterState *interp;
GCState *gcstate;
_PyGC_Reason reason;
// GH-129236: If we see a an active frame without a valid stack pointer,
// we can't collect objects with deferred references because we may not
// see all references.
int skip_deferred_objects;
Py_ssize_t collected;
Py_ssize_t uncollectable;
Py_ssize_t long_lived_total;
Expand Down Expand Up @@ -413,9 +417,6 @@ gc_visit_heaps(PyInterpreterState *interp, mi_block_visit_fun *visitor,
static inline void
gc_visit_stackref(_PyStackRef stackref)
{
// Note: we MUST check that it is deferred before checking the rest.
// Otherwise we might read into invalid memory due to non-deferred references
// being dead already.
if (PyStackRef_IsDeferred(stackref) && !PyStackRef_IsNull(stackref)) {
PyObject *obj = PyStackRef_AsPyObjectBorrow(stackref);
if (_PyObject_GC_IS_TRACKED(obj) && !gc_is_frozen(obj)) {
Expand All @@ -426,20 +427,26 @@ gc_visit_stackref(_PyStackRef stackref)

// Add 1 to the gc_refs for every deferred reference on each thread's stack.
static void
gc_visit_thread_stacks(PyInterpreterState *interp)
gc_visit_thread_stacks(PyInterpreterState *interp, struct collection_state *state)
{
_Py_FOR_EACH_TSTATE_BEGIN(interp, p) {
for (_PyInterpreterFrame *f = p->current_frame; f != NULL; f = f->previous) {
PyObject *executable = PyStackRef_AsPyObjectBorrow(f->f_executable);
if (executable == NULL || !PyCode_Check(executable)) {
if (f->owner >= FRAME_OWNED_BY_INTERPRETER) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also updated this to match the condition in Python/gc.c.

continue;
}

_PyStackRef *top = f->stackpointer;
if (top == NULL) {
// GH-129236: The stackpointer may be NULL. Skip this frame
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know the cases when stackpointer can be NULL here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see it can be because of PyStackRef_CLOSE, perhaps add that here too or hint to check gc_visit_thread_stacks_mark_alive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the comment.

// and don't collect objects with deferred references.
state->skip_deferred_objects = 1;
continue;
}

PyCodeObject *co = (PyCodeObject *)executable;
int max_stack = co->co_nlocalsplus + co->co_stacksize;
gc_visit_stackref(f->f_executable);
for (int i = 0; i < max_stack; i++) {
gc_visit_stackref(f->localsplus[i]);
while (top != f->localsplus) {
--top;
gc_visit_stackref(*top);
}
}
}
Expand Down Expand Up @@ -519,10 +526,7 @@ gc_abort_mark_alive(PyInterpreterState *interp,
static int
gc_visit_stackref_mark_alive(_PyObjectStack *stack, _PyStackRef stackref)
{
// Note: we MUST check that it is deferred before checking the rest.
// Otherwise we might read into invalid memory due to non-deferred references
// being dead already.
if (PyStackRef_IsDeferred(stackref) && !PyStackRef_IsNull(stackref)) {
if (!PyStackRef_IsNull(stackref)) {
PyObject *op = PyStackRef_AsPyObjectBorrow(stackref);
if (mark_alive_stack_push(op, stack) < 0) {
return -1;
Expand All @@ -534,27 +538,37 @@ gc_visit_stackref_mark_alive(_PyObjectStack *stack, _PyStackRef stackref)
static int
gc_visit_thread_stacks_mark_alive(PyInterpreterState *interp, _PyObjectStack *stack)
{
int err = 0;
_Py_FOR_EACH_TSTATE_BEGIN(interp, p) {
for (_PyInterpreterFrame *f = p->current_frame; f != NULL; f = f->previous) {
PyObject *executable = PyStackRef_AsPyObjectBorrow(f->f_executable);
if (executable == NULL || !PyCode_Check(executable)) {
if (f->owner >= FRAME_OWNED_BY_INTERPRETER) {
continue;
}

PyCodeObject *co = (PyCodeObject *)executable;
int max_stack = co->co_nlocalsplus + co->co_stacksize;
if (f->stackpointer == NULL) {
// GH-129236: The stackpointer may be NULL in cases where
// the GC is run during a PyStackRef_CLOSE() call. Skip this
// frame for now.
continue;
}

_PyStackRef *top = f->stackpointer;
if (gc_visit_stackref_mark_alive(stack, f->f_executable) < 0) {
return -1;
err = -1;
goto exit;
}
for (int i = 0; i < max_stack; i++) {
if (gc_visit_stackref_mark_alive(stack, f->localsplus[i]) < 0) {
return -1;
while (top != f->localsplus) {
--top;
if (gc_visit_stackref_mark_alive(stack, *top) < 0) {
err = -1;
goto exit;
}
}
}
}
exit:
_Py_FOR_EACH_TSTATE_END(interp);
return 0;
return err;
}
#endif // GC_MARK_ALIVE_STACKS
#endif // GC_ENABLE_MARK_ALIVE
Expand Down Expand Up @@ -789,14 +803,22 @@ mark_heap_visitor(const mi_heap_t *heap, const mi_heap_area_t *area,
return true;
}

if (gc_is_alive(op)) {
_PyObject_ASSERT_WITH_MSG(op, gc_get_refs(op) >= 0,
"refcount is too small");

if (gc_is_alive(op) || !gc_is_unreachable(op)) {
// Object was already marked as reachable.
return true;
}

_PyObject_ASSERT_WITH_MSG(op, gc_get_refs(op) >= 0,
"refcount is too small");
// GH-129236: Hack to avoid collect objects with deferred references
// if we see a frame without a valid stack pointer.
struct collection_state *state = (struct collection_state *)args;
if (state->skip_deferred_objects && _PyObject_HasDeferredRefcount(op)) {
gc_add_refs(op, 1);
}

if (gc_is_unreachable(op) && gc_get_refs(op) != 0) {
if (gc_get_refs(op) != 0) {
// Object is reachable but currently marked as unreachable.
// Mark it as reachable and traverse its pointers to find
// any other object that may be directly reachable from it.
Expand Down Expand Up @@ -985,7 +1007,7 @@ deduce_unreachable_heap(PyInterpreterState *interp,
#endif

// Visit the thread stacks to account for any deferred references.
gc_visit_thread_stacks(interp);
gc_visit_thread_stacks(interp, state);

// Transitively mark reachable objects by clearing the
// _PyGC_BITS_UNREACHABLE flag.
Expand Down
Loading