MAINT: Make most DTypes (not np.dtype) HeapTypes#31364
Open
seberg wants to merge 3 commits into
Open
Conversation
Member
Author
|
@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. |
np.dtype) proper HeapTypesnp.dtype) proper HeapTypes
np.dtype) proper HeapTypesnp.dtype) proper HeapTypes
np.dtype) proper HeapTypesnp.dtype) HeapTypes
Member
|
Any thoughts about benchmarks? |
Member
|
Very cool, BTW. |
Member
Author
|
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 |
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.)
Member
|
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? |
Member
Author
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. |
Member
Author
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.)
Summary of changes:
PyType_FromMetaclassfor all dynamically created dtypes: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.
PyUnstable_SetImmortalwas only used for free-threading. It makes sense on all Python as all Python can be threaded. So addedNpyUnstable_SetImmortalto set it whenver we can.(There could be places where we really do it as a work-around on free-threading, but I don't think we are there yet.)
_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.