-
Notifications
You must be signed in to change notification settings - Fork 109
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
replace splat with Tuple
#513
Conversation
benchmarked to be faster on julia 1.9.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, most of these look like optimizations that should be done. IIUC, the 30% performance improvement you've reported was only for the first step of replacing the splats with Tuple
calls? What extra benefit to you get from the other optimizations?
I get a small (~3ms) speedup on a 50x50x50 array for using |
Co-authored-by: Martin Holters <[email protected]>
Co-authored-by: Martin Holters <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #513 +/- ##
==========================================
+ Coverage 97.29% 97.46% +0.17%
==========================================
Files 18 18
Lines 3105 3080 -25
==========================================
- Hits 3021 3002 -19
+ Misses 84 78 -6 ☔ View full report in Codecov by Sentry. |
I did a bit of benchmarking: # Julia 1.0.5
julia> @btime unwrap(A; dims=1:2) setup=A=rand(500,500);
154.448 ms (250021 allocations: 53.34 MiB) #master
153.964 ms (250014 allocations: 53.34 MiB) #PR
julia> @btime unwrap(A; dims=1:3) setup=A=rand(50,50,50);
118.314 ms (125025 allocations: 43.18 MiB) #master
104.837 ms (235608 allocations: 49.93 MiB) #PR # Julia 1.6.7
julia> @btime unwrap(A; dims=1:2) setup=A=rand(500,500);
216.799 ms (2246030 allocations: 121.87 MiB) #master
148.143 ms (250024 allocations: 53.34 MiB) #PR
julia> @btime unwrap(A; dims=1:3) setup=A=rand(50,50,50);
111.778 ms (125034 allocations: 43.18 MiB) #master
96.549 ms (125026 allocations: 43.18 MiB) #PR # Julia 1.9.4
julia> @btime unwrap(A; dims=1:2) setup=A=rand(500,500);
208.574 ms (2246032 allocations: 121.87 MiB) #master
143.879 ms (250026 allocations: 53.34 MiB) #PR
julia> @btime unwrap(A; dims=1:3) setup=A=rand(50,50,50);
110.831 ms (125036 allocations: 43.18 MiB) #master
97.857 ms (125028 allocations: 43.18 MiB) #PR So interestingly, between Julia v1.0.5 and v1.6.7, we've lost quite some performance for the 2d case which this PR successfully recovers almost without negative side-effects. There is a significant increase in the allocations for the 3d case on Julia v1.0.5, but a) it's a bit faster nevertheless and b) I don't think we should worry too much about Julia older than 1.6. |
Co-authored-by: Martin Holters <[email protected]> NTuple (2) Co-authored-by: Martin Holters <[email protected]> NTuple (3) Co-authored-by: Martin Holters <[email protected]> copyto! (1) Co-authored-by: Martin Holters <[email protected]> NTuple (4) Co-authored-by: Martin Holters <[email protected]> NTuple (5) Co-authored-by: Martin Holters <[email protected]> fill! (1) Co-authored-by: Martin Holters <[email protected]> fill! (2) Co-authored-by: Martin Holters <[email protected]>
Co-authored-by: Martin Holters <[email protected]>
For a random matrix
M
of size (500, 500), allocations measured forunwrap(M; dims=1:2)
drops from 2.2M+ to 250k, with a small speedup of about 30% on Windows, Julia 1.9.This is achieved by changing every instance of splatting a vector into
CartesianIndex{N}
toTuple
.