Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix type stability issue with SubDiskArray #139

Merged
merged 1 commit into from
Jan 26, 2024
Merged

Conversation

wkearn
Copy link
Contributor

@wkearn wkearn commented Jan 25, 2024

I ran into a performance issue that I traced to a type instability in size(a::SubDiskArray).

In the existing definition of SubDiskArray

struct SubDiskArray{T,N} <: AbstractDiskArray{T,N}
    v::SubArray{T,N}
end

the child view is only parameterized by two of the five type parameters of SubArray, so it is a field with an abstract type, which leads to the type instability.

There are a few ways to fix this. The way proposed in this pull request is to parameterize by all of the type parameters of SubArray:

struct SubDiskArray{T,N,P,I,L} <: AbstractDiskArray{T,N}
    v::SubArray{T,N,P,I,L}
end

which gives the child array a concrete type and fixes the type inference problem and my performance issue. One could also do something like

struct SubDiskArray{T,N, A <: SubArray{T,N}} <: AbstractDiskArray{T,N}
    v::A
end

or try to put some constraints on P, I, and L from the types of the parent array. I don't think I understand the whole DiskArrays codebase enough to work out the correct form of that last option, though.

I've also added a test that throws an error with the current type instability and does not error with the new type definition.

Fixes type stability bug in size(v::SubDiskArray)
Copy link
Collaborator

@rafaqz rafaqz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The first seems fine to me.

@rafaqz rafaqz merged commit 3689544 into JuliaIO:main Jan 26, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants