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

Propagate iteration to parent in OffsetArrays #257

Merged
merged 2 commits into from
Aug 27, 2021

Conversation

jishnub
Copy link
Member

@jishnub jishnub commented Aug 19, 2021

This provides a performance boost, especially for range indices but also to some extent for array indices.

On master

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

julia> v = ones(1000);

julia> @btime $v[$r];
  2.010 μs (1 allocation: 7.94 KiB)

julia> @btime $v[$ro];
  25.848 μs (1 allocation: 7.94 KiB)

julia> @btime $v[$s];
  1.897 μs (1 allocation: 7.94 KiB)

julia> @btime $v[$so];
  3.859 μs (1 allocation: 7.94 KiB)

After this PR

julia> @btime $v[$ro];
  1.679 μs (1 allocation: 7.94 KiB)

julia> @btime $v[$so];
  1.862 μs (1 allocation: 7.94 KiB)

It's interesting that an OffsetArray(::StepRange) index leads to faster indexing than with a StepRange index (although the difference appears smaller on nightly).

@codecov
Copy link

codecov bot commented Aug 19, 2021

Codecov Report

Merging #257 (ee59667) into master (f3fad96) will increase coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #257      +/-   ##
==========================================
+ Coverage   95.41%   95.43%   +0.02%     
==========================================
  Files           5        5              
  Lines         436      438       +2     
==========================================
+ Hits          416      418       +2     
  Misses         20       20              
Impacted Files Coverage Δ
src/OffsetArrays.jl 98.30% <100.00%> (+0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3fad96...ee59667. Read the comment docs.

@timholy
Copy link
Member

timholy commented Aug 19, 2021

It really would be nice not to have to specialize so many things on OffsetArray. The more methods we have, the less generic we are. #255 (comment) seems like it needs a decision before we do things like this.

@jishnub
Copy link
Member Author

jishnub commented Aug 19, 2021

This is unrelated to bounds-checking though, and may be addressed independent of #255. This seems to be a necessary addition at least for ranges, and may be version-limited if Base decides to use traits to check if the argument is range-like in iterate. I can restrict this to OffsetRanges to avoid making the method too broad, if this is a concern.

@timholy
Copy link
Member

timholy commented Aug 19, 2021

My point is that it's better to address these by private methods or traits; we don't want to overload every exported function in Base specifically for OffsetArrays, that would be a nightmare. As much as possible, we should give them the generic interface and then let the generic implementations handle them.

I recognize that may not always be possible, but that should be the goal.

@timholy
Copy link
Member

timholy commented Aug 19, 2021

Another approach: what is the specific source of the poor performance on master? Can we fix it there instead?

@jishnub
Copy link
Member Author

jishnub commented Aug 19, 2021

That might be possible using a trait-based approach similar to #255. Perhaps the source of this difference, at least for OffsetRanges, is that iterate doesn't realize that the argument is range-like and supports rapid iteration using first, step and last without a need to index into the argument. The difference for arrays is a bit harder to track down, perhaps more efficient bounds-checking?

Copy link
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.

In light of JuliaLang/julia#41968 (review), I'm changing my recommendation here.

Sorry it took me a while to get to this. I was consumed by JuliaLang/julia#42016, which if it's not entirely misguided (:question:) should be a very impactful change for the whole ecosystem.

@jishnub jishnub merged commit 0c3d0f6 into JuliaArrays:master Aug 27, 2021
@jishnub jishnub deleted the iterateOR branch August 27, 2021 14:03
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