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

Inline start(::CartesianRange) #15775

Merged
merged 1 commit into from
Apr 6, 2016

Conversation

mbauman
Copy link
Sponsor Member

@mbauman mbauman commented Apr 5, 2016

Or, alternatively: "Look ma! No CartesianRanges!"

This dramatically simplifies the generated code for iteration over CartesianRanges -- in fact, no references to CartesianRange appear in the LLVM IR in many cases with this commit. While it does simplify the code in #9080, it does not solve the performance problem there (I see no difference). It does, however, speed up copy(::SubArray) by 1.3 - 1.6x:

julia> A = sub(reshape(1:5^3,5,5,5), 1:2:5, :, 1:2:5);

julia> @benchmark copy!(similar(A), A) # current master
================ Benchmark Results ========================
     Time per evaluation: 232.69 ns [227.97 ns, 237.42 ns]
Proportion of time in GC: 0.00% [0.00%, 0.00%]
        Memory allocated: 0.00 bytes
   Number of allocations: 0 allocations
       Number of samples: 4301
   Number of evaluations: 120601
         R² of OLS model: 0.953
 Time spent benchmarking: 5.53 s

julia> @benchmark copy!(similar(A), A) # this PR
================ Benchmark Results ========================
     Time per evaluation: 168.91 ns [165.67 ns, 172.14 ns]
Proportion of time in GC: 0.00% [0.00%, 0.00%]
        Memory allocated: 0.00 bytes
   Number of allocations: 0 allocations
       Number of samples: 4601
   Number of evaluations: 160601
         R² of OLS model: 0.955
 Time spent benchmarking: 5.33 s

Comparing this to non-scalar indexing, you can see there's still room for improvement, even after this commit:

julia> @benchmark Base._unsafe_getindex!(similar(A), A.parent, A.indexes[1], A.indexes[2], A.indexes[3])
================ Benchmark Results ========================
     Time per evaluation: 115.75 ns [113.43 ns, 118.06 ns]
Proportion of time in GC: 0.00% [0.00%, 0.00%]
        Memory allocated: 0.00 bytes
   Number of allocations: 0 allocations
       Number of samples: 4501
   Number of evaluations: 146001
         R² of OLS model: 0.952
 Time spent benchmarking: 5.22 s

Or, alternatively: "Look ma! No CartesianRanges!"

This dramatically simplifies the generated code for iteration over CartesianRanges -- in fact, no references to CartesianRange appear in the LLVM IR with this commit. While it does simplify the code in JuliaLang#9080, it does not solve the performance problem there (I see no difference).  It does, however, speed up `copy(::SubArray)` by 1.3 - 1.6x:

```jl
julia> A = sub(reshape(1:5^3,5,5,5), 1:2:5, :, 1:2:5);

julia> @benchmark copy!(similar(A), A) # current master
================ Benchmark Results ========================
     Time per evaluation: 232.69 ns [227.97 ns, 237.42 ns]
Proportion of time in GC: 0.00% [0.00%, 0.00%]
        Memory allocated: 0.00 bytes
   Number of allocations: 0 allocations
       Number of samples: 4301
   Number of evaluations: 120601
         R² of OLS model: 0.953
 Time spent benchmarking: 5.53 s

julia> @benchmark copy!(similar(A), A) # this PR
================ Benchmark Results ========================
     Time per evaluation: 168.91 ns [165.67 ns, 172.14 ns]
Proportion of time in GC: 0.00% [0.00%, 0.00%]
        Memory allocated: 0.00 bytes
   Number of allocations: 0 allocations
       Number of samples: 4601
   Number of evaluations: 160601
         R² of OLS model: 0.955
 Time spent benchmarking: 5.33 s
```
@timholy
Copy link
Sponsor Member

timholy commented Apr 6, 2016

👍. Initially counterintuitive, since start should not by itself be terribly performance-sensitive, but of course inlining it gives LLVM the chance to analyze the whole block. Perhaps a reminder that it might make sense, if possible, to grant LLVM more authority to make some of its own inlining decisions above and beyond julia's.

@vtjnash vtjnash merged commit 95607ed into JuliaLang:master Apr 6, 2016
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.

3 participants