Skip to content

Commit

Permalink
Remove specialized PartialQuickSort{<:Integer} method (#36896)
Browse files Browse the repository at this point in the history
This specialized method is actually considerably slower than the one that also
works for `PartialQuickSort{<:OrdinalRange}`. It had been removed before in
9e8fcd3 due to an inference issue, and reintroduced by c41d5a3 (#31632) once
that issue got fixed, but apparently without benchmarks. It turns out that saving
one comparison per iteration isn't worth it. The gain is especially large when
looking for a value among the largest in the array, as the specialized method always
sorted all values lower than the requested one.
  • Loading branch information
nalimilan authored Sep 30, 2020
1 parent 06ba657 commit 28330a2
Show file tree
Hide file tree
Showing 2 changed files with 49 additions and 38 deletions.
28 changes: 4 additions & 24 deletions base/sort.jl
Original file line number Diff line number Diff line change
Expand Up @@ -650,40 +650,20 @@ function sort!(v::AbstractVector, lo::Integer, hi::Integer, a::MergeSortAlg, o::
return v
end

function sort!(v::AbstractVector, lo::Integer, hi::Integer, a::PartialQuickSort{<:Integer},
function sort!(v::AbstractVector, lo::Integer, hi::Integer, a::PartialQuickSort,
o::Ordering)
@inbounds while lo < hi
hi-lo <= SMALL_THRESHOLD && return sort!(v, lo, hi, SMALL_ALGORITHM, o)
j = partition!(v, lo, hi, o)
if j >= a.k
# we don't need to sort anything bigger than j
hi = j-1
elseif j-lo < hi-j
# recurse on the smaller chunk
# this is necessary to preserve O(log(n))
# stack space in the worst case (rather than O(n))
lo < (j-1) && sort!(v, lo, j-1, a, o)
lo = j+1
else
(j+1) < hi && sort!(v, j+1, hi, a, o)
hi = j-1
end
end
return v
end


function sort!(v::AbstractVector, lo::Integer, hi::Integer, a::PartialQuickSort{T},
o::Ordering) where T<:OrdinalRange
@inbounds while lo < hi
hi-lo <= SMALL_THRESHOLD && return sort!(v, lo, hi, SMALL_ALGORITHM, o)
j = partition!(v, lo, hi, o)

if j <= first(a.k)
lo = j+1
elseif j >= last(a.k)
hi = j-1
else
# recurse on the smaller chunk
# this is necessary to preserve O(log(n))
# stack space in the worst case (rather than O(n))
if j-lo < hi-j
lo < (j-1) && sort!(v, lo, j-1, a, o)
lo = j+1
Expand Down
59 changes: 45 additions & 14 deletions test/sorting.jl
Original file line number Diff line number Diff line change
Expand Up @@ -294,14 +294,15 @@ Base.step(r::ConstantRange) = 0
end

@testset "unstable algorithms" begin
for alg in [QuickSort, PartialQuickSort(length(a))]
b = sort(a, alg=alg)
@test issorted(b)
b = sort(a, alg=alg, rev=true)
@test issorted(b, rev=true)
b = sort(a, alg=alg, by=x->1/x)
@test issorted(b, by=x->1/x)
end
b = sort(a, alg=QuickSort)
@test issorted(b)
@test last(b) == last(sort(a, alg=PartialQuickSort(length(a))))
b = sort(a, alg=QuickSort, rev=true)
@test issorted(b, rev=true)
@test last(b) == last(sort(a, alg=PartialQuickSort(length(a)), rev=true))
b = sort(a, alg=QuickSort, by=x->1/x)
@test issorted(b, by=x->1/x)
@test last(b) == last(sort(a, alg=PartialQuickSort(length(a)), by=x->1/x))
end
end
@testset "insorted" begin
Expand Down Expand Up @@ -362,14 +363,26 @@ end
@testset "PartialQuickSort" begin
a = rand(1:10000, 1000)
# test PartialQuickSort only does a partial sort
let alg = PartialQuickSort(1:div(length(a), 10))
k = alg.k
b = sort(a, alg=alg)
c = sort(a, alg=alg, by=x->1/x)
d = sort(a, alg=alg, rev=true)
@test issorted(b[k])
@test issorted(c[k], by=x->1/x)
@test issorted(d[k], rev=true)
@test !issorted(b)
@test !issorted(c, by=x->1/x)
@test !issorted(d, rev=true)
end
let alg = PartialQuickSort(div(length(a), 10))
k = alg.k
b = sort(a, alg=alg)
c = sort(a, alg=alg, by=x->1/x)
d = sort(a, alg=alg, rev=true)
@test issorted(b[1:k])
@test issorted(c[1:k], by=x->1/x)
@test issorted(d[1:k], rev=true)
@test b[k] == sort(a)[k]
@test c[k] == sort(a, by=x->1/x)[k]
@test d[k] == sort(a, rev=true)[k]
@test !issorted(b)
@test !issorted(c, by=x->1/x)
@test !issorted(d, rev=true)
Expand Down Expand Up @@ -417,7 +430,8 @@ end
# stable algorithms
for alg in [MergeSort]
p = sortperm(v, alg=alg, rev=rev)
@test p == sortperm(float(v), alg=alg, rev=rev)
p2 = sortperm(float(v), alg=alg, rev=rev)
@test p == p2
@test p == pi
s = copy(v)
permute!(s, p)
Expand All @@ -427,9 +441,10 @@ end
end

# unstable algorithms
for alg in [QuickSort, PartialQuickSort(n)]
for alg in [QuickSort, PartialQuickSort(1:n)]
p = sortperm(v, alg=alg, rev=rev)
@test p == sortperm(float(v), alg=alg, rev=rev)
p2 = sortperm(float(v), alg=alg, rev=rev)
@test p == p2
@test isperm(p)
@test v[p] == si
s = copy(v)
Expand All @@ -438,6 +453,22 @@ end
invpermute!(s, p)
@test s == v
end
for alg in [PartialQuickSort(n)]
p = sortperm(v, alg=alg, rev=rev)
p2 = sortperm(float(v), alg=alg, rev=rev)
if n == 0
@test isempty(p) && isempty(p2)
else
@test p[n] == p2[n]
@test v[p][n] == si[n]
@test isperm(p)
s = copy(v)
permute!(s, p)
@test s[n] == si[n]
invpermute!(s, p)
@test s == v
end
end
end

v = randn_with_nans(n,0.1)
Expand Down

4 comments on commit 28330a2

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executing the daily benchmark build, I will reply here when finished:

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt

Please sign in to comment.