gh-133931: Introduce _PyObject_XSetRefDelayed to replace Py_XSETREF #134377
Conversation
Objects/obmalloc.c
Outdated
| _PyObject_XSetRefDelayed(PyObject **ptr, PyObject *value) | ||
| { | ||
| PyObject *old = *ptr; | ||
| FT_ATOMIC_STORE_PTR_RELEASE(*ptr, Py_NewRef(value)); |
There was a problem hiding this comment.
The default build implementation in pycore_pymem.h doesn't include the Py_NewRef().
My preference is to keep the Py_NewRef(value) in the callers, i.e.:
_PyObject_XSetRefDelayed(dictptr, Py_NewRef(value));to match the semantics of Py_XSETREF.
|
Interesting... CI passed in my local but not in the CI.. :( |
|
Ah okay.. |
|
Why do we need Also, what is "delayed"? Either choose a clearer name, or add an explanatory comment in the header file. |
IIUC, the critical section is for the root object to prevent other threads from mutating the child object, and a delayed reference to prevent use-after-free from other threads that reference the child object. |
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
Include/internal/pycore_pymem.h
Outdated
| @@ -100,6 +100,18 @@ static inline void _PyObject_XDecRefDelayed(PyObject *obj) | |||
| } | |||
| #endif | |||
|
|
|||
| #ifdef Py_GIL_DISABLED | |||
| // This is the delayed-free equivalent of Py_XSETREF(), providing the same | |||
There was a problem hiding this comment.
this sounds opposite, _PyObject_XSetRefDelayed is the delayed version whereas Py_XSETREF is the delayed free equivalent
Co-authored-by: Kumar Aditya <kumaraditya@python.org>
Objects/obmalloc.c
Outdated
| _PyObject_XDecRefDelayed(old); | ||
| } | ||
| else { | ||
| Py_DECREF(old); |
There was a problem hiding this comment.
if the object is immortal then decref has no effect
There was a problem hiding this comment.
Yeah but it tracks the stat. I am not sure we need this.
There was a problem hiding this comment.
Lines 375 to 378 in 598aa7c
There was a problem hiding this comment.
But let's remove it, unitil we need this stat explicitly.
Co-authored-by: Victor Stinner <vstinner@python.org>
|
Could someone please explain to me, why Line 7381 in 598aa7c Which one is more correct? |
gen_set_nameandgen_set_qualnamethread-safe in free-threaded builds #133931