[mypyc] Make attribute access memory safe on free-threaded builds#21705
[mypyc] Make attribute access memory safe on free-threaded builds#21705JukkaL wants to merge 10 commits into
Conversation
|
cc @msullivan |
| if (value != NULL) { | ||
| _PyObject_SetMaybeWeakref(value); | ||
| } |
There was a problem hiding this comment.
I think that this should not be required for correctness anymore. It looks like the original implementation of GetAttrRef had a loop on _Py_TryIncrefCompare, which requires that maybe-weakref be set in order to succeed, but the current version calls _Py_NewRefWithLock, which does not require that, so we should be good. (I think that change was made so that init wouldn't need to call _PyObject_SetMaybeWeakref, but it also means set shouldn't need to either.)
| if (_Py_TryIncrefCompare(field, v)) { | ||
| return v; | ||
| } | ||
| Py_BEGIN_CRITICAL_SECTION(self); |
There was a problem hiding this comment.
I don't think that the Py_BEGIN_CRITICAL_SECTION can possibly be doing anything useful here, since the writer doesn't take one.
The slow paths for list/dict reads that use Py_BEGIN_CRITICAL_SECTION rely on the write side taking a critical section.
| if (_Py_TryIncrefCompare(field, v)) { | ||
| return v; | ||
| } |
There was a problem hiding this comment.
I don't think that we need to use _Py_TryIncrefCompare or do a re-load of the field, with the current implementation. I think we should be able to use just _Py_TryIncRefShared.
In particular, I think that the dict/list paths that rely on validating that the values haven't changed are a different code path than this one in which the objects do actually get freed but the allocator makes sure they don't get reused for different kinds of objects, so the refcounts stay refcounts.
We're doing a different approach based on _PyObject_XDecRefDelayed, so I don't think that the reloading-and-checking helps any.
2 participants
WIP
This causes a ~6% slowdown in self check when using 3.14t.
Work on mypyc/mypyc#1203.