[mypyc] Make native attribute writes faster with free threading - #21748
Conversation
|
@msullivan This is a potentially faster way to implement memory safe attribute access. |
371864e to
54f5644
Compare
|
I measured the impact to self-check, and on Linux/AMD Ryzen, this improves performance by about 2.7%, and on macOS/M1 Pro performance improves by about 2.3%. In microbenchmarks performance impact is all over the place and on Apple Silicon they were slower on average, but I trust the self check benchmark more. |
msullivan
left a comment
msullivan
left a comment
There was a problem hiding this comment.
I think this all seems right, and it gives better behavior too (eager destruction).
| # The attribute is known to be previously undefined (NULL) and self | ||
| # can't have leaked yet, so there is no old value to reclaim and no | ||
| # competing writer; a relaxed store suffices (self's later publication | ||
| # provides the release barrier -- see CPy_InitAttrRef). |
There was a problem hiding this comment.
I'm not sure so much comment is useful when just calling the helpers
| // The reload is deliberately relaxed, where CPython 3.14 uses a consume load in the | ||
| // equivalent locked read (_PyObject_TryGetInstanceAttribute): acquiring the owner's | ||
| // critical section already pairs with CPy_SetAttrRef's release store, and a value | ||
| // published by CPy_InitAttrRef is ordered by the publication of the owner itself | ||
| // (see CPy_InitAttrRef). |
There was a problem hiding this comment.
This comment is wrong: acquiring the critical section pairs with the "end critical section" from the writer, not with the release store. I think substantively it is fine that it is correct.
I need to go check why _PyObject_TryGetInstanceAttribute is using consume though...
Consume doesn't really exist though; it looks like in cpython it is "acquire under TSan and relaxed in real life"... I'm a little unsure why they felt it was worth bothering with it...
| PyObject *CPy_GetAttrRefSlow(PyObject *v, PyObject *owner, PyObject **field) { | ||
| if (_Py_TryIncRefShared(v)) { | ||
| return v; | ||
| if (v == (PyObject *)_Py_atomic_load_ptr(field)) { |
There was a problem hiding this comment.
We could use _Py_TryIncrefCompare instead of doing this comparison ourselves, which might be cleaner... but it would duplicate the _Py_TryIncrefFast check I suppose
| // CPy_SetAttrRef free the old value immediately instead of deferring it. Both were | ||
| // verified against CPython 3.14 and depend on interpreter internals, so they must | ||
| // be rechecked when adding support for a new Python version: | ||
| // - Reading the header of a freed object cannot fault. All three object heaps | ||
| // set 'page_use_qsbr' (see Python/pystate.c), so a freed block's page is not | ||
| // unmapped or handed to another size class while any thread is attached. | ||
| // - _Py_TryIncrefFast cannot succeed on stale memory. The current thread does | ||
| // not allocate between the field load and the try-incref, so the block cannot | ||
| // have been reused for an object owned by this thread; a freed block reads | ||
| // ob_tid as 0 or as mimalloc's free-list pointer (which overwrites only the | ||
| // first word, i.e. ob_tid) and ob_ref_local as 0, so neither the | ||
| // owned-by-this-thread test nor the immortal test can fire. | ||
| // If the block was reused for a live object at the same address, that object is | ||
| // either the field's current value (so returning it is correct) or the field | ||
| // validation fails and the provisional reference is dropped again, leaving its | ||
| // refcount unchanged. ob_ref_shared of a freed block is 0 or _Py_REF_MERGED, so | ||
| // _Py_TryIncRefShared fails on it and the reader falls into the locked path. |
There was a problem hiding this comment.
I think this can all be trimmed a bunch here. I think this is kind of the fundamental invariant of QSBR in cpython: any pointer to a PyObject will keep being a valid pointer to a PyObject header at least, so trying to do an incref on it will be safe.
| // arbitrary destructor does not run while the owner is locked -- the equivalent in | ||
| // CPython 3.14 (store_instance_attr_lock_held) decrefs with the lock still held. |
There was a problem hiding this comment.
hmmmm; I wonder why the cpython equiv does this
| // Caveat (based on CPython 3.14 internals): this takes 'owner->ob_mutex', which for | ||
| // a native class with a built-in base is the same mutex CPython uses for that | ||
| // container's own critical sections, and CPython runs arbitrary Python code under it | ||
| // (e.g. _PyDict_SetItem_LockHeld calls __hash__/__eq__ while holding CS(dict)). | ||
| // Re-entering a critical section on the same object is only free when the held | ||
| // section is the top-most one; with an unrelated section still active in between, | ||
| // the thread parks, detaches, has this mutex released by | ||
| // _PyCriticalSection_SuspendAll, then re-locked by _PyCriticalSection_Resume, and | ||
| // parks again. Reaching that needs a callback under | ||
| // two nested critical sections to assign an attribute of the outer object; the | ||
| // helpers here can't cause it on their own, since they only ever hold this one | ||
| // mutex (so there is also no lock-ordering deadlock). |
There was a problem hiding this comment.
this is all exciting but maybe irrelevant; nothing to be done about it
# Conflicts: # mypyc/codegen/emitfunc.py
2 participants
This makes safe native attribute access faster in free-threaded builds by using optimistic reference acquisition, with field validation when needed, for reads. An object-level lock is used only as a fallback during reads and when assigning to a mutable attribute, except during initialization before self can escape. Final and thread-confined generator attributes still use fast direct access. Most attribute reads remain lock-free.
Together, optimistic reference acquisition, field validation, CPython’s protection of stale object headers, and the shared fallback lock ensure that readers either obtain a valid reference or reload the value under the lock. This lets writers decref the old value immediately after releasing the lock instead of deferring the decref itself through QSBR.
I measured the impact to self-check, and on Linux/AMD Ryzen, this improves performance by about 2.7%, and on macOS/M1 Pro performance improves by about 2.3%. In microbenchmarks performance impact is all over the place and on Apple Silicon they were slower on average, but I trust the self check benchmark more.