-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
GH-135106: Restrict trashcan to GC'ed objects #135682
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.
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
Changes from 1 commit
2d81b6b
f89ddfd
7101e8f
a81bb72
98c9efd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
Restrict the trashcan mechanism to GC'ed objects and untrack them while in | ||
the trashcan to prevent the GC and trashcan mechanisms conflicting. |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -3034,57 +3034,24 @@ Py_ReprLeave(PyObject *obj) | |||
|
||||
/* Trashcan support. */ | ||||
|
||||
#ifndef Py_GIL_DISABLED | ||||
/* We need to store a pointer in the refcount field of | ||||
* an object. It is important that we never store 0 (NULL). | ||||
* It is also important to not make the object appear immortal, | ||||
* or it might be untracked by the cycle GC. */ | ||||
static uintptr_t | ||||
pointer_to_safe_refcount(void *ptr) | ||||
{ | ||||
uintptr_t full = (uintptr_t)ptr; | ||||
assert((full & 3) == 0); | ||||
#if SIZEOF_VOID_P > 4 | ||||
uint32_t refcnt = (uint32_t)full; | ||||
if (refcnt >= (uint32_t)_Py_IMMORTAL_MINIMUM_REFCNT) { | ||||
full = full - ((uintptr_t)_Py_IMMORTAL_MINIMUM_REFCNT) + 1; | ||||
} | ||||
return full + 2; | ||||
#else | ||||
// Make the top two bits 0, so it appears mortal. | ||||
return (full >> 2) + 1; | ||||
#endif | ||||
} | ||||
|
||||
static void * | ||||
safe_refcount_to_pointer(uintptr_t refcnt) | ||||
{ | ||||
#if SIZEOF_VOID_P > 4 | ||||
if (refcnt & 1) { | ||||
refcnt += _Py_IMMORTAL_MINIMUM_REFCNT - 1; | ||||
} | ||||
return (void *)(refcnt - 2); | ||||
#else | ||||
return (void *)((refcnt -1) << 2); | ||||
#endif | ||||
} | ||||
#endif | ||||
|
||||
/* Add op to the gcstate->trash_delete_later list. Called when the current | ||||
* call-stack depth gets large. op must be a currently untracked gc'ed | ||||
* object, with refcount 0. Py_DECREF must already have been called on it. | ||||
* call-stack depth gets large. op must be a gc'ed object, with refcount 0. | ||||
* Py_DECREF must already have been called on it. | ||||
*/ | ||||
void | ||||
_PyTrash_thread_deposit_object(PyThreadState *tstate, PyObject *op) | ||||
{ | ||||
_PyObject_ASSERT(op, Py_REFCNT(op) == 0); | ||||
assert(PyObject_IS_GC(op)); | ||||
int tracked = _PyObject_GC_IS_TRACKED(op); | ||||
if (tracked) { | ||||
_PyObject_GC_UNTRACK(op); | ||||
} | ||||
uintptr_t tagged_ptr = ((uintptr_t)tstate->delete_later) | tracked; | ||||
#ifdef Py_GIL_DISABLED | ||||
op->ob_tid = (uintptr_t)tstate->delete_later; | ||||
op->ob_tid = tagged_ptr; | ||||
#else | ||||
/* Store the delete_later pointer in the refcnt field. */ | ||||
uintptr_t refcnt = pointer_to_safe_refcount(tstate->delete_later); | ||||
*((uintptr_t*)op) = refcnt; | ||||
assert(!_Py_IsImmortal(op)); | ||||
_Py_AS_GC(op)->_gc_next = tagged_ptr; | ||||
#endif | ||||
tstate->delete_later = op; | ||||
} | ||||
|
@@ -3099,25 +3066,28 @@ _PyTrash_thread_destroy_chain(PyThreadState *tstate) | |||
destructor dealloc = Py_TYPE(op)->tp_dealloc; | ||||
|
||||
#ifdef Py_GIL_DISABLED | ||||
tstate->delete_later = (PyObject*) op->ob_tid; | ||||
uintptr_t tagged_ptr = op->ob_tid; | ||||
op->ob_tid = 0; | ||||
_Py_atomic_store_ssize_relaxed(&op->ob_ref_shared, _Py_REF_MERGED); | ||||
#else | ||||
/* Get the delete_later pointer from the refcnt field. | ||||
* See _PyTrash_thread_deposit_object(). */ | ||||
uintptr_t refcnt = *((uintptr_t*)op); | ||||
tstate->delete_later = safe_refcount_to_pointer(refcnt); | ||||
op->ob_refcnt = 0; | ||||
uintptr_t tagged_ptr = _Py_AS_GC(op)->_gc_next; | ||||
_Py_AS_GC(op)->_gc_next = 0; | ||||
#endif | ||||
|
||||
/* Call the deallocator directly. This used to try to | ||||
* fool Py_DECREF into calling it indirectly, but | ||||
* Py_DECREF was already called on this object, and in | ||||
* assorted non-release builds calling Py_DECREF again ends | ||||
* up distorting allocation statistics. | ||||
*/ | ||||
_PyObject_ASSERT(op, Py_REFCNT(op) == 0); | ||||
(*dealloc)(op); | ||||
tstate->delete_later = (PyObject *)(tagged_ptr & ~1); | ||||
if (tagged_ptr & 1) { | ||||
_PyObject_GC_TRACK(op); | ||||
} | ||||
/* It is possible that the object has been accessed through | ||||
* a weak ref, so only free if refcount == 0) */ | ||||
if (Py_REFCNT(op) == 0) { | ||||
/* Call the deallocator directly. This used to try to | ||||
* fool Py_DECREF into calling it indirectly, but | ||||
* Py_DECREF was already called on this object, and in | ||||
* assorted non-release builds calling Py_DECREF again ends | ||||
* up distorting allocation statistics. | ||||
*/ | ||||
(*dealloc)(op); | ||||
} | ||||
} | ||||
} | ||||
|
||||
|
@@ -3186,7 +3156,7 @@ _Py_Dealloc(PyObject *op) | |||
destructor dealloc = type->tp_dealloc; | ||||
PyThreadState *tstate = _PyThreadState_GET(); | ||||
intptr_t margin = _Py_RecursionLimit_GetMargin(tstate); | ||||
if (margin < 2) { | ||||
if (margin < 2 && PyObject_IS_GC(op)) { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you also need to guard the That checks seems a bit more difficult to add efficiently. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Does that matter? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can do it reasonably efficiently with a read of |
||||
_PyTrash_thread_deposit_object(tstate, (PyObject *)op); | ||||
return; | ||||
} | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you already brought this up in the meeting, but I don't think this should be necessary. Weakrefs won't resurrect an object with refcount of zero, which
_PyTrash_thread_deposit_object()
ensures.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll put the assert back.