gh-128100: Atomic dict load in _PyObject_GenericGetAttrWithDict #128297
Conversation
Objects/object.c
Outdated
| } | ||
| else { | ||
| Py_BEGIN_CRITICAL_SECTION(obj); | ||
| #ifdef DISABLE_GIL |
There was a problem hiding this comment.
This should be Py_GIL_DISABLED.
Objects/object.c
Outdated
| else { | ||
| Py_BEGIN_CRITICAL_SECTION(obj); | ||
| #ifdef DISABLE_GIL | ||
| PyObject **dictptr = _Py_atomic_load_ptr(_PyObject_ComputedDictPointer(obj)); |
There was a problem hiding this comment.
This is the wrong thing to load atomically--dereferencing it should be the atomic operation. In fact, this doesn't do what you think it does: _Py_atomic_load* takes a pointer, and the thing that it points to gets atomically loaded and returned.
Objects/object.c
Outdated
| dict = (PyObject *)_PyObject_GetManagedDict(obj); | ||
| } | ||
| else { | ||
| Py_BEGIN_CRITICAL_SECTION(obj); |
There was a problem hiding this comment.
In general, we don't need to lock something if we're going to use one of the _Py_atomic APIs, and vice versa.
| PyObject **dictptr = _PyObject_ComputedDictPointer(obj); | ||
| #endif | ||
| if (dictptr) { | ||
| dict = *dictptr; |
There was a problem hiding this comment.
Here is where the atomic load should be. (And, as Sam said in the issue, we want _Py_atomic_load_ptr_acquire instead of plain old _Py_atomic_load_ptr.)
It wasn't reporting any TSAN failures before the PR.
|
@colesbury Was this supposed to get backported to 3.13? |
|
Thanks @wrongnull for the PR, and @colesbury for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
…ttrWithDict` (pythonGH-128297) (cherry picked from commit 47d2cb8) Co-authored-by: Bogdan Romanyuk <[email protected]>
|
GH-129979 is a backport of this pull request to the 3.13 branch. |
…AttrWithDict` (GH-128297) (GH-129979) (cherry picked from commit 47d2cb8) Co-authored-by: Bogdan Romanyuk <[email protected]>
3 participants
This is my very first attempt in free-threading in cpython. So that I would be happy to get some criticism