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

Speed up permsort by utilizing stability of the default sorting algorithm #47587

Draft
wants to merge 41 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 36 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
42c70a1
initial functionality
Oct 17, 2022
61e4006
support 5- and 3-argument sort! for backwards compatability
Oct 29, 2022
901182c
test for bug that slipped through test suite
Oct 17, 2022
e032ba6
fix bug
Oct 17, 2022
e6cfee0
make send_to_end more human friendly (and less compiler friendly! int…
Oct 20, 2022
f160582
Give each sorting pass and DEFAULT_STABLE a docstring
Oct 29, 2022
15a4484
add tests and fix typos they unveiled
Oct 30, 2022
d82b090
avoid potential name conflict
Nov 1, 2022
029cbae
switch to custom keyword handling
Nov 1, 2022
d3bdca3
remove InsertionSortAlg and MergeSortAlg
Nov 2, 2022
2232cac
better algorithm display
Nov 2, 2022
a574c7f
stop passing U around
Nov 2, 2022
05de36e
remove lenm1
Nov 6, 2022
70290d6
fix unexpected allocations in Radix Sort
Nov 7, 2022
f06de10
fix doctests? I have no idea how
Nov 7, 2022
38f4512
support and test backwards compatability with packages that depend in…
Nov 9, 2022
383b9d2
Merge branch 'master' into sort-dispatch
Nov 9, 2022
d8ae968
improve extensibility tests
Nov 10, 2022
c633419
overhall scratch space handling
Nov 11, 2022
32a6f54
Merge branch 'master' into sort-dispatch
LilithHafner Nov 14, 2022
a2c2646
Consistency with other constructors
Nov 15, 2022
71e8fa1
Introduce PermUnstable to speed up sortperm
petvana Nov 15, 2022
812c917
Fix a mistake
petvana Nov 15, 2022
e752ea7
pass around even fewer easily computed things in kw to reduce load on…
Nov 18, 2022
15666f2
Merge branch 'master' into sort-dispatch
LilithHafner Nov 18, 2022
04399d9
Merge branch 'sort-dispatch' into pv/PermUnstable-v4
petvana Nov 18, 2022
34621c7
Merge branch 'pv/PermUnstable-v4' into pv/PermUnstable-v5
petvana Dec 5, 2022
7e6f103
Restore the PR
petvana Dec 5, 2022
77b2b08
Rename to PermFast
petvana Dec 5, 2022
36d3ff3
Introduce send_to_end_stable!
petvana Dec 5, 2022
1fe68d9
Fix spacing
petvana Dec 5, 2022
dd1d89b
Fix trailing whitespace
petvana Dec 5, 2022
91c2d2a
Merge branch 'master' into pv/PermUnstable-v4
petvana Dec 5, 2022
20ddeb4
Small fixes
petvana Dec 6, 2022
c14432b
Fix sortperm!
petvana Dec 6, 2022
176d779
Merge branch 'master' into pv/PermUnstable-v4
petvana Dec 13, 2022
a2f9710
Commit suggestion from review
petvana Dec 14, 2022
2d2cf4d
De-duplicate code
petvana Dec 14, 2022
ef8e8eb
Merge branch 'pv/PermUnstable-v4' of github.com:petvana/julia into pv…
petvana Dec 14, 2022
3b972eb
Re-use scratch array
petvana Dec 14, 2022
9b5be34
Merge branch 'master' into pv/PermUnstable-v4
petvana Dec 28, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion base/ordering.jl
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import .Base:

export # not exported by Base
Ordering, Forward, Reverse,
By, Lt, Perm,
By, Lt, Perm, PermFast,
ReverseOrdering, ForwardOrdering,
DirectOrdering,
lt, ord, ordtype
Expand Down Expand Up @@ -106,8 +106,21 @@ struct Perm{O<:Ordering,V<:AbstractVector} <: Ordering
data::V
end

"""
PermFast(order::Ordering, data::AbstractVector)
`Ordering` on the indices of `data` where `i` is less than `j` if `data[i]` is
less than `data[j]` according to `order`. In the case that `data[i]` and
`data[j]` are equal, the ordering is undefined. Thus, it is designed to be
faster than `Perm` when a stable sorting algorithm is used.
"""
struct PermFast{O<:Ordering,V<:AbstractVector} <: Ordering
order::O
data::V
end

ReverseOrdering(by::By) = By(by.by, ReverseOrdering(by.order))
ReverseOrdering(perm::Perm) = Perm(ReverseOrdering(perm.order), perm.data)
ReverseOrdering(perm::PermFast) = PermFast(ReverseOrdering(perm.order), perm.data)

"""
lt(o::Ordering, a, b)
Expand All @@ -125,6 +138,12 @@ lt(o::Lt, a, b) = o.lt(a,b)
(lt(p.order, da, db)::Bool) | (!(lt(p.order, db, da)::Bool) & (a < b))
end

@propagate_inbounds function lt(p::PermFast, a::Integer, b::Integer)
da = p.data[a]
db = p.data[b]
lt(p.order, da, db)::Bool
end

_ord(lt::typeof(isless), by::typeof(identity), order::Ordering) = order
_ord(lt::typeof(isless), by, order::Ordering) = By(by, order)

Expand Down
67 changes: 61 additions & 6 deletions base/sort.jl
Original file line number Diff line number Diff line change
Expand Up @@ -560,15 +560,52 @@ elements that are not
@inline send_to_end!(f::F, v::AbstractVector, ::ReverseOrdering, end_stable=false; lo, hi) where F <: Function =
end_stable ? (send_to_end!(!f, v; lo, hi)+1, hi) : (hi-send_to_end!(f, view(v, hi:-1:lo))+1, hi)

"""
send_to_end_stable!(f::Function, v::AbstractVector; [lo, hi])

Send every element of `v` for which `f` returns `true` to the end of the vector and return
the index of the last element which for which `f` returns `false`.

`send_to_end_stable!(f, v, lo, hi)` is equivalent to `send_to_end_stable!(f, view(v, lo:hi))+lo-1`

Preserves the order of the elements.
"""
function send_to_end_stable!(f::F, v::AbstractVector; lo=firstindex(v), hi=lastindex(v)) where F <: Function
tmp=copy(v)
Copy link
Member

Choose a reason for hiding this comment

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

Ideally, this should utilize the scratch space re-use system, though it this PR is a performance improvement without, then that isn't essential.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've tried to re-use the scratch array, but not sure if i got your @getkw concept right.

offset = 0
@inbounds begin
while lo <= hi
x = tmp[lo]
fx = f(x)::Bool
v[(fx ? hi : lo) - offset] = x
offset += fx
lo += 1
end
end

# This is similar to the partition function
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to have code re-use; the loop here is almost exactly the same as the loops in partition!

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice. However, partition! left a pivot on the place and split to two parts with. Thus it seems challenging.

pivot_index = lo-offset-1
# t[<=pivot_index] <* f(x) = false
# t[>pivot_index] >* f(x) = true

# Make the results stable
reverse!(v, pivot_index+1, hi)
return pivot_index
end

@inline send_to_end_stable!(f::F, v::AbstractVector, ::ForwardOrdering; lo, hi) where F <: Function = (lo, send_to_end_stable!(f, v; lo, hi))
@inline send_to_end_stable!(f::F, v::AbstractVector, ::ReverseOrdering; lo, hi) where F <: Function = (hi-send_to_end_stable!(f, view(v, hi:-1:lo))+1, hi)

function _sort!(v::AbstractVector, a::MissingOptimization, o::Ordering, kw)
@getkw lo hi
if nonmissingtype(eltype(v)) != eltype(v) && o isa DirectOrdering
lo, hi = send_to_end!(ismissing, v, o; lo, hi)
_sort!(WithoutMissingVector(v, unsafe=true), a.next, o, (;kw..., lo, hi))
elseif eltype(v) <: Integer && o isa Perm{DirectOrdering} && nonmissingtype(eltype(o.data)) != eltype(o.data)
elseif eltype(v) <: Integer && (o isa Perm{DirectOrdering} || o isa PermFast{DirectOrdering}) &&
nonmissingtype(eltype(o.data)) != eltype(o.data)
PermT = o isa Perm{DirectOrdering} ? Perm : PermFast
petvana marked this conversation as resolved.
Show resolved Hide resolved
lo, hi = send_to_end!(i -> ismissing(@inbounds o.data[i]), v, o)
_sort!(v, a.next, Perm(o.order, WithoutMissingVector(o.data, unsafe=true)), (;kw..., lo, hi))
_sort!(v, a.next, PermT(o.order, WithoutMissingVector(o.data, unsafe=true)), (;kw..., lo, hi))
else
_sort!(v, a.next, o, kw)
end
Expand Down Expand Up @@ -603,7 +640,7 @@ function _sort!(v::AbstractVector, a::IEEEFloatOptimization, o::Ordering, kw)
j = send_to_end!(x -> after_zero(o, x), v; lo, hi)
scratch = _sort!(iv, a.next, Reverse, (;kw..., lo, hi=j))
if scratch === nothing # Union split
_sort!(iv, a.next, Forward, (;kw..., lo=j+1, hi, scratch))
_sort!(iv, a.next, Forward, (;kw..., lo=j+1, hi, nothing))
else
_sort!(iv, a.next, Forward, (;kw..., lo=j+1, hi, scratch))
Copy link
Member Author

Choose a reason for hiding this comment

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

@LilithHafner Btw, is scratch type-stable here, i.e., can compiler infer that scratch cannot be Nothing?

Copy link
Member

Choose a reason for hiding this comment

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

I recall that removing the if statement entirely introduced dynamic dispatch. IICU, all of type inference is an implementation detail, so I'm not totally sure, but I believe that we need an if statement to force union splitting though it works well whether we use _sort!(..., nothing) or _sort!(..., scratch) because the compiler can determine the type of scratch at compile time..

end
Expand All @@ -613,10 +650,20 @@ function _sort!(v::AbstractVector, a::IEEEFloatOptimization, o::Ordering, kw)
j = send_to_end!(i -> after_zero(o.order, @inbounds o.data[i]), v; lo, hi)
scratch = _sort!(v, a.next, Perm(Reverse, ip), (;kw..., lo, hi=j))
if scratch === nothing # Union split
_sort!(v, a.next, Perm(Forward, ip), (;kw..., lo=j+1, hi, scratch))
_sort!(v, a.next, Perm(Forward, ip), (;kw..., lo=j+1, hi, nothing))
else
_sort!(v, a.next, Perm(Forward, ip), (;kw..., lo=j+1, hi, scratch))
end
elseif eltype(v) <: Integer && o isa PermFast && o.order isa DirectOrdering && is_concrete_IEEEFloat(eltype(o.data))
lo, hi = send_to_end_stable!(i -> isnan(@inbounds o.data[i]), v, o.order; lo, hi)
ip = reinterpret(UIntType(eltype(o.data)), o.data)
j = send_to_end_stable!(i -> after_zero(o.order, @inbounds o.data[i]), v; lo, hi)
scratch = _sort!(v, a.next, PermFast(Reverse, ip), (;kw..., lo, hi=j))
if scratch === nothing # Union split
_sort!(v, a.next, PermFast(Forward, ip), (;kw..., lo=j+1, hi, nothing))
else
_sort!(v, a.next, PermFast(Forward, ip), (;kw..., lo=j+1, hi, scratch))
end
petvana marked this conversation as resolved.
Show resolved Hide resolved
else
_sort!(v, a.next, o, kw)
end
Expand Down Expand Up @@ -1558,7 +1605,11 @@ function sortperm(A::AbstractArray;
end
end
ix = copymutable(LinearIndices(A))
sort!(ix; alg, order = Perm(ordr, vec(A)), scratch, dims...)
if alg == DEFAULT_STABLE
sort!(ix; alg, order = PermFast(ordr, vec(A)), scratch, dims...)
else
sort!(ix; alg, order = Perm(ordr, vec(A)), scratch, dims...)
end
end


Expand Down Expand Up @@ -1615,7 +1666,11 @@ function sortperm!(ix::AbstractArray{T}, A::AbstractArray;
if !initialized
ix .= LinearIndices(A)
end
sort!(ix; alg, order = Perm(ord(lt, by, rev, order), vec(A)), scratch, dims...)
if alg == DEFAULT_STABLE
sort!(ix; alg, order = PermFast(ord(lt, by, rev, order), vec(A)), scratch, dims...)
else
sort!(ix; alg, order = Perm(ord(lt, by, rev, order), vec(A)), scratch, dims...)
end
end

# sortperm for vectors of few unique integers
Expand Down