Skip to content

Conversation

LilithHafner
Copy link
Contributor

@LilithHafner LilithHafner commented Apr 11, 2022

This PR may increase performance in some cases and should pretty much never pose much of a regression. Perhaps more importantly, it drops a dependency on an undocumented internal method of Julia that may be removed in 1.10 because internals are not subject to SymVer.

In 1.9.0, Julia's permute! method got a bit more efficient. These benchmarks compare use of permute! to permute!!

julia> using BenchmarkTools, Random, Plots

julia> x = 2 .^ (3:26);

julia> GC.gc(); y!! = [(hmer_heights = rand(Int, n); o = randperm(n); @elapsed(Base.permute!!(hmer_heights, o))) for n in x];

julia> GC.gc(); y! = [(hmer_heights = rand(Int, n); o = randperm(n); @elapsed(permute!(hmer_heights, o))) for n in x];

julia> plot(x, y! ./ x, label="permute!"); plot!(x, y!! ./ x, label="permute!!", xlabel="length(hmer_heights)", ylabel="seconds/element", xscale=:log10, yscale=:log10)
Screenshot 2023-05-14 at 5 22 17 PM

In the case of the usage in this package, in cases where permute!! is faster (small inputs or old versions of Julia) this PR may result in a minor performance regression, but never more than 33% because there are already two calls to permute! in this spot. In cases where permute!! is slower, there is not really a cap on how much of an improvement this PR can be. For large inputs on modern versions of Julia, this could easily be a 3x or more improvement because of the poor cache locality properties of permute!!.

(I rewrote this post with the release of 1.9.0)

@LilithHafner LilithHafner marked this pull request as draft April 11, 2022 16:45
@codecov-commenter
Copy link

codecov-commenter commented Apr 11, 2022

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.14 🎉

Comparison is base (8af2507) 95.30% compared to head (ec4a764) 95.44%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
+ Coverage   95.30%   95.44%   +0.14%     
==========================================
  Files          17       18       +1     
  Lines        1213     1448     +235     
==========================================
+ Hits         1156     1382     +226     
- Misses         57       66       +9     
Impacted Files Coverage Δ
src/hclust.jl 94.65% <100.00%> (-0.21%) ⬇️

... and 13 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@LilithHafner LilithHafner marked this pull request as ready for review May 14, 2023 22:27
@LilithHafner
Copy link
Contributor Author

Bumping because now that Julia 1.9 is out, this is a pretty clear performance win in addition to a maintainability win.

@alyst alyst closed this Jun 18, 2023
@alyst alyst reopened this Jun 18, 2023
@alyst alyst merged commit 6021a28 into JuliaStats:master Jun 18, 2023
@LilithHafner LilithHafner deleted the patch-1 branch June 18, 2023 23:32
@alyst alyst mentioned this pull request Jun 30, 2023
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