gh-122417: Implement per-thread heap type refcounts #122418
Conversation
The free-threaded build partially stores heap type reference counts in distributed manner in per-thread arrays. This avoids reference count contention when creating or destroying instances. Co-authored-by: Ken Jin <[email protected]>
colesbury
requested review from
a team,
ericsnowcurrently,
markshannon and
erlend-aasland
as code owners
Include/internal/pycore_object.h
Outdated
| PyHeapTypeObject *ht = (PyHeapTypeObject *)type; | ||
|
|
||
| // Unsigned comparison so that `ht_id=-1` is treated as out-of-bounds. | ||
| Py_ssize_t ht_id = ht->_ht_id; |
There was a problem hiding this comment.
Nit comment:
Can we extract those two the common static inline function as
get_heap_type_id(ht);too nit, please ignore it.- check_heap_type_id_assigend(ht_id, size); or heap_type_id_assigend(ht_id, size);
It will be helpful to understand what's happening in here for people like me :)
There was a problem hiding this comment.
By the way, you can ignore this, but some people may need to understand the mechanism when they have to write similar features. At that time it will be helpful to verify ht_id
There was a problem hiding this comment.
I don't see a great way of refactoring out this check -- I think a function name that accurately describes the check will be awkwardly long. It's not just that the heap type has an assigned id (i.e., ht_id != -1), but also that the thread state has space for it its per-thread array (i.e., ht_id < size).
Include/internal/pycore_object.h
Outdated
| _PyThreadStateImpl *tstate = (_PyThreadStateImpl *)_PyThreadState_GET(); | ||
| PyHeapTypeObject *ht = (PyHeapTypeObject *)type; | ||
|
|
||
| // Unsigned comparison so that `ht_id=-1` is treated as out-of-bounds. |
There was a problem hiding this comment.
Or just simply
| // Unsigned comparison so that `ht_id=-1` is treated as out-of-bounds. | |
| // Unsigned comparison so that `ht_id=-1` which indicates no thread refcounting is treated as out-of-bounds. |
There was a problem hiding this comment.
I've updated the comment and moved the "fast path" to the "if" branch instead of the "else"
Include/cpython/object.h
Outdated
| @@ -270,6 +270,9 @@ typedef struct _heaptypeobject { | |||
| PyObject *ht_module; | |||
| char *_ht_tpname; // Storage for "tp_name"; see PyType_FromModuleAndSpec | |||
| struct _specialization_cache _spec_cache; // For use by the specializer. | |||
| #ifdef Py_GIL_DISABLED | |||
| Py_ssize_t _ht_id; // ID used for thread-local refcounting | |||
There was a problem hiding this comment.
This seems excessive if you are reusing them. Maybe use a 32 bit number?
There was a problem hiding this comment.
Also, _ht_id is not the clearest name. unique_id perhaps (since we know this is a heap type, the ht prefix adds little).
There was a problem hiding this comment.
More than 2^31-1 heap types seems unlikely, but not implausible. It also doesn't save any space, at least not for now, due to padding and alignment.
|
When you're done making the requested changes, leave the comment: |
| #ifndef Py_GIL_DISABLED | ||
| Py_DECREF(type); | ||
| #else | ||
| if (!_PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) { |
There was a problem hiding this comment.
| if (!_PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE)) { | |
| if (_Py_IsImmortal(type)) { |
Immortal heap types are allowed.
There was a problem hiding this comment.
Immortal heap types are fine. They either go through the Py_DECREF(), which is a no-op, or do some unnecessary work with the per-thread refcounts, which is also fine.
The assumption in the assert is that static types are immortal. We already assert that elsewhere (in _Py_NewReference).
We could also write:
if (_Py_IsImmortal(type)) {
return;
}
assert(_PyType_HasFeature(type, Py_TPFLAGS_HEAPTYPE));That seems a bit less robust to me in cases where the assumptions are violated, but also fine.
There was a problem hiding this comment.
Either works. I think they equivalent in terms of checking assumptions.
|
@markshannon, I've used macros in the default build and renamed Let me know what you think about the guards in I have made the requested changes; please review again. |
|
Thanks for making the requested changes! @corona10, @markshannon: please review the changes made to this pull request. |
) The free-threaded build partially stores heap type reference counts in distributed manner in per-thread arrays. This avoids reference count contention when creating or destroying instances. Co-authored-by: Ken Jin <[email protected]>
) The free-threaded build partially stores heap type reference counts in distributed manner in per-thread arrays. This avoids reference count contention when creating or destroying instances. Co-authored-by: Ken Jin <[email protected]>
The free-threaded build partially stores heap type reference counts in distributed manner in per-thread arrays. This avoids reference count contention when creating or destroying instances of heap types.