Repository navigation
MAINT: Make most DTypes (not np.dtype) HeapTypes - #31364
Conversation
|
@ngoldbaum free-threaded crash in `macOS tests / Accelerate - macos_x86_64 - 3.14t (pull_request). Presumably unrelated (let's see if it reproduces). EDIT, no it's reliable. This happened before in #30375 except that there it didn't actually segfault. Here it starts to actually segfault. DetailsEDIT: My impression is that its OKish to skip the test, but after it's done and we have nightlies, we either re-open the Python issue or make a new one -- I would hope this is Python. |
seberg
changed the title
np.dtype) proper HeapTypesnp.dtype) proper HeapTypes
seberg
changed the title
np.dtype) proper HeapTypesnp.dtype) proper HeapTypes
seberg
changed the title
np.dtype) proper HeapTypesnp.dtype) HeapTypes
|
Any thoughts about benchmarks? |
|
Very cool, BTW. |
|
I would think it doesn't make a real difference. The main difference I could see would be related to them being implicitly immortal previously and now they are only explicitly immortal. I suppose there may be some other minor differences between heap and static types, but I don't think for the most part. I ran some benchmarks ( Details |
|
Seems something is off with a release note
I wonder if heap types are forcing a different memory layout and causing some cache misses. These are consistent over a few runs and recompilations? |
Yeah, I'll open a PR. I thought it was just a missing rebase first, but I think I truly forgot to add this function to the docs probably... And it didn't register because I thought there was a general CI failure that also hit the circle CI build. |
I don't really think so, but of course it might change allocation patterns in the grand scheme? The allocation size didn't change (they were heap-type sized also before, we just didn't use the heaptype part of it) and in practice we also heap allocated them before as well... The changes in those benchmarks are consistent yes, but I think we had +consistent random shifts in performance before in many PRs that are completely unrelated? One other thing I could imagine (since I can't find |
|
@kumaraditya303 can you try taking a look at this PR sometime in the next week if you have a chance? |
|
Converting a static type to heap type is technically backwards incompatible because if any third party code defines static type which subclasses from |
|
Yeah which is why np.dtype itself cannot be converted :(. But it is only valid to subclass the abstract types in principle. In practice, I think we can get away with it now hopefully. (I.e. quaddtype doesn't and there are no big ones, and even if, it is unlikely they subclass). |
Are you sure? Why wouldn't this work as long as |
|
ahh, I see, it is an actual check in CPython. But why is that? Is there a theoretical barrier? The basic_size and slots could all be made to handle this case, no? Edit: the discussion in the issue that led to adding this check gave no rational for it. The opposite is true: Cython needed to flip the HEAP_TYPE flag when calling PyType_Ready on some types to avoid this check. There was a request to review this for CPython3.10 but it got lost. |
Static types are immutable whereas heap types are mutable, so if this is allowed then you can have a static type whose attribute change even though it is marked as immutable which is problematic. I think the specializing interpreter takes advantage of this and has performance benefit as well. |
Hmmm, but if this is the main reason then setting |
|
Well, I just removed the immortalization since it seems hard and python devs feel that deferred refcounting should be enough and these types are already deferred ref-counted anyway. EDIT: Maybe Python relaxes the immortalization logic, but I think we can follow up either way (I might also just make a PR there). |
|
Can we now simplify that |
Hmmmm, I stil made them immutable, so we still would have to add |
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
Overall looks good. There are some errors (mostly in error paths) that Claude spotted, see comments below.
| * Fill DTypeMeta information that varies between DTypes, any variable | ||
| * type information would need to be set before PyType_Ready(). | ||
| */ | ||
| dtype_class->singleton = (PyArray_Descr *)descr; |
There was a problem hiding this comment.
Pre-existing issue, but shouldn't this be an owned reference? The dealloc path does a DECREF and there's no corresponding INCREF:
There was a problem hiding this comment.
This mutates a type after creating it, which violates the IMMUTABLE flag that is set on the type. Now that you're creating every type with a spec, you can trivially add this as a special-case inside dtypemeta_wrap_legacy_descriptor when you create type_slots.
There was a problem hiding this comment.
This is still getting mutated - was that intentional?
There was a problem hiding this comment.
ack, no just forgot to move it...
This makes all DTypes HeapTypes. For the builtin ones (and non-user created ones) we also mark them all as immortal as it might help a bit with refcounting on threaded execution. Because it is IMO nonsense to limit that to free-threaded, added a helper and use it throughout. (Claude was used with guidance to execute what I wanted.)
This reverts commit 84c2eef.
…gh (and the case for types)
ngoldbaum
left a comment
ngoldbaum
left a comment
There was a problem hiding this comment.
Just one comment, I'm going to send a separate PR in for the singleton ownership issues, I think.
There was a problem hiding this comment.
This is still getting mutated - was that intentional?
kumaraditya303
left a comment
kumaraditya303
left a comment
There was a problem hiding this comment.
I read through the C changes and didn't find any issues. LGTM
|
I opened seberg#53 with a couple fixes. |
There was a problem hiding this comment.
OK, I did one last round of review and I'm pretty sure all the C changes are OK now.
Sorry to have a couple last requests but I think the heap type inheritance issue might be worth pausing to think over? Up to you though, I'm hitting approve if you want to merge as-is and iterate on these issues.
IMO this needs a release note describing the nature of the change before hitting the merge button. The note should target extension module authors and should note that dtypes can now be dynamically allocated heap types and refcounting assumptions around static lifetimes might be invalidated.
I think the way we are setting a docstring on heap types is incorrect with this PR:
numpy/numpy/_core/src/multiarray/compiled_base.c
Lines 1503 to 1522 in 6a1cef1
For a heap type that actually ends up garbage collected, tp_doc can be free'd, and us setting tp_doc manually there becomes incorrect.
Claude found that on a debug build of Python, both type("X", (np.dtype,), {}) and type("X", (type(np.dtype("float64")),), {}) segfault on this PR, with this stack trace:
Details
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
* frame #0: 0x0000000000000000
frame #1: 0x00000001001e5b88 python`type_new_get_bases + 532
frame #2: 0x00000001001dab10 python`type_new + 424
frame #3: 0x00000001001d9f30 python`type_call + 624
frame #4: 0x00000001000ee020 python`_PyObject_MakeTpCall + 644
frame #5: 0x00000001001dad4c python`type_vectorcall + 180
frame #6: 0x00000001000eda68 python`_PyObject_VectorcallTstate + 316
frame #7: 0x00000001000eec80 python`PyObject_Vectorcall + 60
frame #8: 0x00000001002f9360 python`_TAIL_CALL_CALL + 2624
frame #9: 0x00000001002e54dc python`_PyEval_EvalFrameDefault + 536
frame #10: 0x00000001002e4fb4 python`_PyEval_EvalFrame + 60
frame #11: 0x00000001002e4efc python`_PyEval_Vector + 552
frame #12: 0x00000001002e4c7c python`PyEval_EvalCode + 228
frame #13: 0x00000001003f3bf0 python`run_eval_code_obj + 212
frame #14: 0x00000001003f3900 python`run_mod + 680
frame #15: 0x00000001003f19f4 python`_PyRun_StringFlagsWithName + 228
frame #16: 0x00000001003f184c python`_PyRun_SimpleStringFlagsWithName + 196
frame #17: 0x0000000100434e18 python`pymain_run_command + 288
frame #18: 0x00000001004345e0 python`pymain_run_python + 656
frame #19: 0x0000000100434304 python`Py_RunMain + 36
frame #20: 0x00000001004347d8 python`pymain_main + 100
frame #21: 0x000000010043482c python`Py_BytesMain + 60
frame #22: 0x0000000100004024 python`main + 36
frame #23: 0x0000000187ac7e00 dyld`start + 6992
On a normal build of Python and on NumPy main with both builds, you get TypeError: Preliminary-API: Cannot subclass DType..
Claude suggests the following fix, which I can neither confirm no deny:
Suggested fix (needs a build to confirm — I reasoned it through CPython's
type_new/type_call but didn't compile it): drop
Py_TPFLAGS_DISALLOW_INSTANTIATION and restore tp_init = dtypemeta_init (the
raising one). Leave tp_new inherited (== type_new), which still satisfies
PyType_FromMetaclass's "tp_new must be NULL or type_new" restriction. Then:
- type("X",(np.dtype,),{}) → type_new builds the candidate with
metatype=_DTypeMeta (no NULL call), then type_call invokes _DTypeMeta.tp_init →
clean TypeError.
- type("X",(Float64DType,),{}) → Float64DType has no Py_TPFLAGS_BASETYPE → "not
an acceptable base type" before tp_init is even reached.
- Caveat for that fix: type_new now briefly builds a PyArray_DTypeMeta with
dt_slots == NULL before tp_init rejects it, and it's GC-tracked in that window.
dtypemeta_dealloc already tolerates NULL dt_slots (the ownership-fix commit),
but dtypemeta_traverse (dtypemeta.c:511) does not —
Py_VISIT(NPY_DT_SLOTS(type)->castingimpls) would deref NULL. Guard it: if
(NPY_DT_SLOTS(type) != NULL).
Yes, but the analysis just bitterly wrong/omitting things. This is a Python bug and I'll open one: Python must raise an error when
Sure, let me add a very brief note. Extension dtype authors won't notice this, IMO. It really is only user-DType authors who now indeed could get refcounting wrong when it previously may not have mattered (they actually work with the types, nobody else does). |
|
Thanks @seberg! |
This makes all DTypes HeapTypes. I would mark them as immortal, but Python isn't making it easy without private API use so assume the (already enabled) deferred ref-counting is enough.
Because it is IMO nonsense to limit that to free-threaded, added a helper and use it throughout.
(Claude was used with guidance to execute what I wanted.)
Summary of changes:
PyType_FromMetaclassfor all dynamically created dtypes:We make sure to also make the immortal, and keep them immutable for now.add_newdocis changed to work with mutable heap-types where one can set__doc__. Making all of these immutable means it doesn't matter, but thought I'd just keep it.__doc__available in the dict), but now with immutable it isn't actually used.So there is a potential small improvement here.
_PyArray_MapPyTypeToDTypefor the other dtypes is just moved out for now, didn't seem clearer (and the order didn't work out).Overall, I think this is all very straight-forward actually. I thought there might be something tricky, but the trickiest thing was that all DTypes need to be moved if we move the abstract ones for subclassing.