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

Apply InitialOptimizations more consistently in sorting #47946

Merged
merged 3 commits into from
Dec 22, 2022

Conversation

LilithHafner
Copy link
Member

#47383 revised how initial optimizations (special cases for eltypes that are unions with missing and/or a concrete IEEEFloat or Bool and very small input) are implemented and dispatched to. I accidentally dropped the initial optimizations in that PR for cases where the user specifies alg=QuickSort, alg=MergeSort, or alg=InsertionSort. I also dropped those optimizations for partialsort and paritalsort!. Thanks @vtjnash for catching these regressions here

This PR applies InitialOptimizations to achieve better performance and avoid regressions.

julia> @btime sort(rand(50_000); alg=QuickSort);
  2.705 ms (2 allocations: 390.67 KiB) => 3.733 ms (4 allocations: 781.34 KiB) => 2.652 ms (4 allocations: 781.34 KiB)

julia> @btime sortperm(vcat([missing], rand(50_000)));
  6.221 ms (8 allocations: 1.19 MiB) => 6.785 ms (12 allocations: 1.57 MiB) => 3.525 ms (12 allocations: 1.57 MiB)

julia> VERSION
v"1.8.3" => v"1.10.0-DEV.164" => PR

Also fixes a dispatch bug in ::MissingOptimization and debugs code that was previously unreachable due to that dispatch bug.

@LilithHafner LilithHafner added performance Must go faster bugfix This change fixes an existing bug sorting Put things in order backport 1.9 Change should be backported to release-1.9 labels Dec 21, 2022
@LilithHafner
Copy link
Member Author

@nanosoldier runbenchmarks("sort", vs=":master")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - no performance regressions were detected. A full report can be found here.

@KristofferC
Copy link
Member

@nanosoldier runbenchmarks("sort", vs=":release-1.8")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@KristofferC
Copy link
Member

Performance now looks equal to 1.8 but allocates 3x more. Is that expected?

@LilithHafner
Copy link
Member Author

LilithHafner commented Dec 21, 2022

The allocations in sortperm! should be zero but there is a type instability that caused about 16 bytes and 1 allocation in 1.8 and now causes 48 bytes and 3 allocations. The nanosoldier benchmark is basically this:

julia> list = rand(50_000);

julia> ix = collect(1:length(list));

julia> @benchmark sortperm!(x, $list; alg = InsertionSort, rev = true) setup=(x = copy($ix))
BenchmarkTools.Trial: 5 samples with 1 evaluation.
 Range (min  max):  1.068 s    1.144 s  ┊ GC (min  max): 0.00%  0.00%
 Time  (median):     1.087 s              ┊ GC (median):    0.00%
 Time  (mean ± σ):   1.091 s ± 30.354 ms  ┊ GC (mean ± σ):  0.00% ± 0.00%

  ▁  ▁          █                                         ▁  
  █▁▁█▁▁▁▁▁▁▁▁▁▁█▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁▁█ ▁
  1.07 s         Histogram: frequency by time        1.14 s <

 Memory estimate: 48 bytes, allocs estimate: 3.

See issue #47949

@LilithHafner
Copy link
Member Author

The performance diff across this PR seems reasonable under JuliaCI/BaseBenchmarks.jl#305 as well

@KristofferC
Copy link
Member

Alright. This should be good to go then?

@LilithHafner LilithHafner merged commit 12e679c into master Dec 22, 2022
@LilithHafner LilithHafner deleted the apply-initial-sorting-optimizations branch December 22, 2022 11:44
KristofferC pushed a commit that referenced this pull request Dec 27, 2022
…tch bug (#47946)

* Apply InitialOptimizations by default in several cases when it was previously present

* fixup for MissingOptimization

* fix stability in the sortperm union with missing case

(cherry picked from commit 12e679c)
@KristofferC KristofferC mentioned this pull request Jan 10, 2023
41 tasks
@KristofferC KristofferC removed the backport 1.9 Change should be backported to release-1.9 label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug performance Must go faster sorting Put things in order
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants