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

Dispatch on traits in iterate for AbstractArrays and types that wrap AbstractArrays #41968

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jishnub
Copy link
Contributor

@jishnub jishnub commented Aug 23, 2021

An attempt at using traits in iterate for AbstractArrays (Fixes #41945), which makes iteration for wrapper types (eg. OffsetArrays) use the same iterate method used for the parent array, consequently obtaining the same performance. Array wrapper types may opt-in to the iteration behavior of the parent through the trait IterationStyle, for example as

Base.IterationStyle(A::OffsetArray) = Base.IterationStyle(parent(A))

By default they will fall back to the iterate method for AbstractArrays, which is the present behavior.

A comparison of the performance without using the trait and with it (see #41945 for other examples):

Without defining the trait for OffsetArrays:

julia> r = 1:1:1000; ro = OffsetArray(r);

julia> @btime Float64[i for i in $r];
  1.621 μs (1 allocation: 7.94 KiB)

julia> @btime Float64[i for i in $ro];
  25.306 μs (1 allocation: 7.94 KiB)

With the trait defined:

julia> @btime Float64[i for i in $r];
  1.677 μs (1 allocation: 7.94 KiB)

julia> @btime Float64[i for i in $ro];
  1.609 μs (1 allocation: 7.94 KiB)

I would love some feedback on this as it is a somewhat large change, although there seems to be a significant performance benefit to this.

@jishnub jishnub changed the title Dispatch on traits in iterate Dispatch on traits in iterate for AbstractArrays and types that wrap AbstractArrays Aug 23, 2021
Copy link
Sponsor Member

@timholy timholy left a comment

Choose a reason for hiding this comment

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

I like this, although since iterate relies on indexing your tests might also check OffsetArray with an offset other than 0; iterate(OffsetArray(v, (0,)) and iterate(OffsetArray(v, (1,))) should yield the same sequence of values (all the elements of v).

My main response is to consider whether we can be a bit more indirect. From profiling, to me it seems that bounds-checking accounts for an awful lot of the performance gap. Intervening there, rather than directly in iterate, thus seems likely to benefit a wider range of use cases. #42029 and #42030 are efforts in that direction. Shall we see how that pans out and then consider this later? I'll drop my objection to JuliaArrays/OffsetArrays.jl#257 in the meantime, since I recognize that the path I'm proposing may take a while.

@jishnub
Copy link
Contributor Author

jishnub commented Aug 27, 2021

Fair enough, that seems like a reasonable approach. Best to narrow down the issue before introducing more traits.

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.

[RFC] Dispatch on traits instead of types in iterate for AbstractArrays
2 participants