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

slightly speed up cartesian indexing #51606

Merged

Conversation

oscardssmith
Copy link
Member

@oscardssmith oscardssmith commented Oct 5, 2023

This is a minor simplification to the multidimensional iteration code that behaves slightly better in the presence of possible overflow (at least according to LLVM. This was found when looking into performance regressions caused by #51319 since that makes Array rely more on the regular multidimensional code rather than being special cased.

This PR does remove/alter a few tests, but they are all tests of iteration when given an invalid iterator state which we specifically document as a thing that does not work.

Also worth noting that @vtjnash is the one who actually made these changes. I'm just separating them (with permission) from the memory PR because it's a lot easier to review a ~10 line change than a 5000 line change.

@oscardssmith oscardssmith added performance Must go faster domain:arrays [a, r, r, a, y, s] domain:iteration Involves iteration or the iteration protocol labels Oct 5, 2023
@oscardssmith oscardssmith force-pushed the speed-up-multidimensional-iterate branch from 0473675 to 3a46ec1 Compare October 5, 2023 20:39
@vtjnash
Copy link
Sponsor Member

vtjnash commented Oct 6, 2023

looks like there are some test problems with it still:

Test Failed at /cache/build/default-amdci4-4/julialang/julia-master/julia-3a46ec16e8/share/julia/test/cartesian.jl:422
  Expression: iterate(I, i) === (CartesianIndex(1, 2), CartesianIndex(1, 2))
   Evaluated: (CartesianIndex(2, 2), CartesianIndex(2, 2)) === (CartesianIndex(1, 2), CartesianIndex(1, 2))
Error in testset cartesian:
Test Failed at /cache/build/default-amdci4-4/julialang/julia-master/julia-3a46ec16e8/share/julia/test/cartesian.jl:426
  Expression: iterate(I, i) === (CartesianIndex(2, 3), CartesianIndex(2, 3))
   Evaluated: (CartesianIndex(-9223372036854775808, 1), CartesianIndex(-9223372036854775808, 1)) === (CartesianIndex(2, 3), CartesianIndex(2, 3))

test/cartesian.jl Outdated Show resolved Hide resolved
test/cartesian.jl Outdated Show resolved Hide resolved
@vtjnash vtjnash added the status:merge me PR is reviewed. Merge when all tests are passing label Oct 6, 2023
@vtjnash vtjnash merged commit 1d6ee7e into JuliaLang:master Oct 6, 2023
6 of 7 checks passed
@oscardssmith oscardssmith deleted the speed-up-multidimensional-iterate branch October 6, 2023 16:30
@oscardssmith oscardssmith removed the status:merge me PR is reviewed. Merge when all tests are passing label Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:arrays [a, r, r, a, y, s] domain:iteration Involves iteration or the iteration protocol performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants