Skip to content

bpo-44032: Defer clearing last thread state until last GC has been run.#26285

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
markshannon merged 2 commits into from
May 24, 2021
Merged

bpo-44032: Defer clearing last thread state until last GC has been run. #26285

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
236beca
Defer clearing last thread state until last GC has been run.
markshannon May 21, 2021
ddad17e
Delay deletion of stack chunks until thread state is deleted.
markshannon May 22, 2021
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
14 changes: 7 additions & 7 deletions Python/pystate.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -897,13 +897,6 @@ PyThreadState_Clear(PyThreadState *tstate)
if (tstate->on_delete != NULL){
tstate->on_delete(tstate->on_delete_data);
Copy link
Contributor

@erlend-aaslanderlend-aaslandMay 22, 2021

Choose a reason for hiding this comment

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

Should this also be moved to tstate_delete_common, or is it unrelated to deleting the data stack?

Copy link
MemberAuthor

@markshannonmarkshannonMay 22, 2021

Choose a reason for hiding this comment

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

I have no idea what it does, so I'm not touching it 🙂

Copy link
Contributor

@erlend-aaslanderlend-aaslandMay 22, 2021•
edited
Loading

Choose a reason for hiding this comment

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

Me neither :) I tried moving it, and it seems to work fine either place. BTW, I found a clue in Include:

cpython/Include/cpython/pystate.h

Lines 134 to 143 in a9e4361

* So instead of holding the lock directly, the tstate holds a weakref to
* the lock: that's the value of on_delete_data below. Decref'ing a
* weakref is harmless.
* on_delete points to _threadmodule.c's static release_sentinel() function.
* After the tstate is unlinked, release_sentinel is called with the
* weakref-to-lock (on_delete_data) argument, and release_sentinel releases
* the indirectly held lock.
*/
void (*on_delete)(void*);
void*on_delete_data;

Copy link
Contributor

@erlend-aaslanderlend-aaslandMay 22, 2021

Choose a reason for hiding this comment

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

Also: https://docs.python.org/3/c-api/init.html#c.PyThreadState_Clear:

"Changed in version 3.9: This function now calls the PyThreadState.on_delete callback. Previously, that happened in PyThreadState_Delete()."

Copy link
Contributor

@erlend-aaslanderlend-aaslandMay 22, 2021•
edited
Loading

Choose a reason for hiding this comment

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

... and here:

cpython/Modules/_threadmodule.c

Lines 1280 to 1329 in a9e4361

release_sentinel(void*wr_raw)
{
PyObject*wr=_PyObject_CAST(wr_raw);
/* Tricky: this function is called when the current thread state
is being deleted. Therefore, only simple C code can safely
execute here. */
PyObject*obj=PyWeakref_GET_OBJECT(wr);
lockobject*lock;
if (obj!=Py_None){
lock= (lockobject*) obj;
if (lock->locked){
PyThread_release_lock(lock->lock_lock);
lock->locked=0;
}
}
/* Deallocating a weakref with a NULL callback only calls
PyObject_GC_Del(), which can't call any Python code. */
Py_DECREF(wr);
}
staticPyObject*
thread__set_sentinel(PyObject*module, PyObject*Py_UNUSED(ignored))
{
PyObject*wr;
PyThreadState*tstate=PyThreadState_Get();
lockobject*lock;
if (tstate->on_delete_data!=NULL){
/* We must support the re-creation of the lock from a
fork()ed child. */
assert(tstate->on_delete==&release_sentinel);
wr= (PyObject*) tstate->on_delete_data;
tstate->on_delete=NULL;
tstate->on_delete_data=NULL;
Py_DECREF(wr);
}
lock=newlockobject(module);
if (lock==NULL)
returnNULL;
/* The lock is owned by whoever called _set_sentinel(), but the weakref
hangs to the thread state. */
wr=PyWeakref_NewRef((PyObject*) lock, NULL);
if (wr==NULL){
Py_DECREF(lock);
returnNULL;
}
tstate->on_delete_data= (void*) wr;
tstate->on_delete=&release_sentinel;
return (PyObject*) lock;
}

Copy link
Contributor

@erlend-aaslanderlend-aaslandMay 22, 2021

Choose a reason for hiding this comment

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

It's added by @vstinner in GH-18296 (issue 39511). I guess he knows if it should stay or follow, then :)

}
_PyStackChunk *chunk = tstate->datastack_chunk;
tstate->datastack_chunk = NULL;
while (chunk != NULL){
_PyStackChunk *prev = chunk->previous;
_PyObject_VirtualFree(chunk, chunk->size);
chunk = prev;
}
}


Expand DownExpand Up@@ -936,6 +929,13 @@ tstate_delete_common(PyThreadState *tstate,
{
PyThread_tss_set(&gilstate->autoTSSkey, NULL);
}
_PyStackChunk *chunk = tstate->datastack_chunk;
tstate->datastack_chunk = NULL;
while (chunk != NULL){
_PyStackChunk *prev = chunk->previous;
_PyObject_VirtualFree(chunk, chunk->size);
chunk = prev;
}
}

static void
Expand Down