gh-128100: Atomic dict load in _PyObject_GenericGetAttrWithDict - #128297
gh-128100: Atomic dict load in _PyObject_GenericGetAttrWithDict#128297colesbury merged 4 commits into
Conversation
Sorry, something went wrong.
ZeroIntensity
left a comment
There was a problem hiding this comment.
Thanks for taking a shot at this. I haven't dove too deep into the issue so take my review with a grain of salt.
Sorry, something went wrong.
| } | ||
| else { | ||
| Py_BEGIN_CRITICAL_SECTION(obj); | ||
| #ifdef DISABLE_GIL |
There was a problem hiding this comment.
This should be Py_GIL_DISABLED.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| 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.
Sorry, something went wrong.
| 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.)
Sorry, something went wrong.
colesbury
left a comment
There was a problem hiding this comment.
The change looks good. Let's drop the test for now as it's not catching any TSAN data races even with repeated runs before this change. We can write a better test when we address the second half of the issue.
Sorry, something went wrong.
It wasn't reporting any TSAN failures before the PR.
|
@colesbury Was this supposed to get backported to 3.13? |
Sorry, something went wrong.
|
Thanks @wrongnull for the PR, and @colesbury for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13. |
Sorry, something went wrong.
…ttrWithDict` (pythonGH-128297) (cherry picked from commit 47d2cb8) Co-authored-by: Bogdan Romanyuk <65823030+wrongnull@users.noreply.github.com>
|
GH-129979 is a backport of this pull request to the 3.13 branch. |
Sorry, something went wrong.
This is my very first attempt in free-threading in cpython. So that I would be happy to get some criticism