From 70d9aef37dbc2bebabf6cba5e1ce77d46bbedbd8 Mon Sep 17 00:00:00 2001 From: Lilith Hafner Date: Wed, 21 Dec 2022 23:07:24 +0800 Subject: [PATCH 1/8] add strange hacks to avoid compiler heuristics? --- base/sort.jl | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/base/sort.jl b/base/sort.jl index 1266da8a8c9df..1a2536340fac1 100644 --- a/base/sort.jl +++ b/base/sort.jl @@ -1572,7 +1572,7 @@ function sortperm(A::AbstractArray; end end end - ix = copymutable(LinearIndices(A)) + ix = unsafe_copymutable_LinearIndices(A) sort!(ix; alg, order = Perm(ordr, vec(A)), scratch, dims...) end @@ -1625,14 +1625,21 @@ function sortperm!(ix::AbstractArray{T}, A::AbstractArray; scratch::Union{Vector{T}, Nothing}=nothing, dims...) where T <: Integer #to optionally specify dims argument (typeof(A) <: AbstractVector) == (:dims in keys(dims)) && throw(ArgumentError("Dims argument incorrect for type $(typeof(A))")) - axes(ix) == axes(A) || throw(ArgumentError("index array must have the same size/axes as the source array, $(axes(ix)) != $(axes(A))")) + axes(ix) == axes(A) || @noinline throw_sortperm_axis_mismatch_error(ix, A) if !initialized - ix .= LinearIndices(A) + very_unsafe_copyto!(ix, LinearIndices(A)) end sort!(ix; alg, order = Perm(ord(lt, by, rev, order), vec(A)), scratch, dims...) end +# TODO stop using these three hacks +# but check performance, especially unexpected allocations, when removing +Base.@assume_effects :nothrow very_unsafe_copyto!(a, b) = a .= b +Base.@assume_effects :nothrow unsafe_copymutable_LinearIndices(A) = copymutable(LinearIndices(A)) +throw_sortperm_axis_mismatch_error(ix, A) = + throw(ArgumentError("index array must have the same size/axes as the source array, $(axes(ix)) != $(axes(A))")) + # sortperm for vectors of few unique integers function sortperm_int_range(x::Vector{<:Integer}, rangelen, minval) offs = 1 - minval From 62a5729c7dcbfc88fbd8dd119b0a7447e594b333 Mon Sep 17 00:00:00 2001 From: Lilith Hafner Date: Wed, 21 Dec 2022 23:51:49 +0800 Subject: [PATCH 2/8] add allocation tests --- test/sorting.jl | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/sorting.jl b/test/sorting.jl index eb5020547c789..abe9a94436588 100644 --- a/test/sorting.jl +++ b/test/sorting.jl @@ -918,6 +918,25 @@ end @test bsqs() === bsqs(missing, missing, InsertionSort) end +function test_allocs() + v = rand(10) + i = randperm(length(v)) + @test 1 == @allocations sort(v) + @test 0 == @allocations sortperm!(i, v) + @test 0 == @allocations sort!(i) + @test_broken 0 == @allocations sortperm!(i, v, rev=true) + @test 0 == @allocations sortperm!(i, v, order=Base.Reverse) + @test 1 == @allocations sortperm(v) + @test 1 == @allocations sortperm(i, by=sqrt) + @test 0 == @allocations sort!(v, lt=(a, b) -> hash(a) < hash(b)) + @test 0 == @allocations sort!(i, rev=false) + rand!(i) + @test 0 == @allocations sort!(i, order=Base.Reverse) +end +@testset "Small calls do not unnecessarily allocate" begin + test_allocs() +end + # This testset is at the end of the file because it is slow. @testset "searchsorted" begin numTypes = [ Int8, Int16, Int32, Int64, Int128, From 3ae8d9c0699923eee652d9a53f3de6e60fe0fddf Mon Sep 17 00:00:00 2001 From: Lilith Hafner Date: Thu, 22 Dec 2022 08:52:38 -0600 Subject: [PATCH 3/8] elimiate allocations in the rev::Bool case as well --- base/sort.jl | 21 ++++++++++++++++----- test/sorting.jl | 4 +++- 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/base/sort.jl b/base/sort.jl index 1a2536340fac1..c02669b9aafe0 100644 --- a/base/sort.jl +++ b/base/sort.jl @@ -1560,8 +1560,14 @@ function sortperm(A::AbstractArray; order::Ordering=Forward, scratch::Union{Vector{<:Integer}, Nothing}=nothing, dims...) #to optionally specify dims argument - ordr = ord(lt,by,rev,order) - if ordr === Forward && isa(A,Vector) && eltype(A)<:Integer + if rev === true + _sortperm(A; alg, order=ord(lt, by, true, order), scratch, dims...) + else + _sortperm(A; alg, order=ord(lt, by, nothing, order), scratch, dims...) + end +end +function _sortperm(A::AbstractArray; alg, order, scratch, dims...) + if order === Forward && isa(A,Vector) && eltype(A)<:Integer n = length(A) if n > 1 min, max = extrema(A) @@ -1573,7 +1579,7 @@ function sortperm(A::AbstractArray; end end ix = unsafe_copymutable_LinearIndices(A) - sort!(ix; alg, order = Perm(ordr, vec(A)), scratch, dims...) + sort!(ix; alg, order = Perm(order, vec(A)), scratch, dims...) end @@ -1615,7 +1621,7 @@ julia> sortperm!(p, A; dims=2); p 2 4 ``` """ -function sortperm!(ix::AbstractArray{T}, A::AbstractArray; +@inline function sortperm!(ix::AbstractArray{T}, A::AbstractArray; alg::Algorithm=DEFAULT_UNSTABLE, lt=isless, by=identity, @@ -1630,7 +1636,12 @@ function sortperm!(ix::AbstractArray{T}, A::AbstractArray; if !initialized very_unsafe_copyto!(ix, LinearIndices(A)) end - sort!(ix; alg, order = Perm(ord(lt, by, rev, order), vec(A)), scratch, dims...) + + if rev === true + sort!(ix; alg, order=Perm(ord(lt, by, true, order), vec(A)), scratch, dims...) + else + sort!(ix; alg, order=Perm(ord(lt, by, nothing, order), vec(A)), scratch, dims...) + end end # TODO stop using these three hacks diff --git a/test/sorting.jl b/test/sorting.jl index abe9a94436588..2f92b276e701d 100644 --- a/test/sorting.jl +++ b/test/sorting.jl @@ -924,7 +924,9 @@ function test_allocs() @test 1 == @allocations sort(v) @test 0 == @allocations sortperm!(i, v) @test 0 == @allocations sort!(i) - @test_broken 0 == @allocations sortperm!(i, v, rev=true) + @test 0 == @allocations sortperm!(i, v, rev=true) + @test 1 == @allocations sortperm(i, v, rev=true) + @test 1 == @allocations sortperm(i, v, rev=false) @test 0 == @allocations sortperm!(i, v, order=Base.Reverse) @test 1 == @allocations sortperm(v) @test 1 == @allocations sortperm(i, by=sqrt) From f06d19753f94f2776afb68688b63e0bfb8af90b7 Mon Sep 17 00:00:00 2001 From: Lilith Hafner Date: Thu, 22 Dec 2022 14:31:37 -0600 Subject: [PATCH 4/8] compile before allocation test --- test/sorting.jl | 1 + 1 file changed, 1 insertion(+) diff --git a/test/sorting.jl b/test/sorting.jl index 2f92b276e701d..7706f6a8073bc 100644 --- a/test/sorting.jl +++ b/test/sorting.jl @@ -931,6 +931,7 @@ function test_allocs() @test 1 == @allocations sortperm(v) @test 1 == @allocations sortperm(i, by=sqrt) @test 0 == @allocations sort!(v, lt=(a, b) -> hash(a) < hash(b)) + sort!(Int[], rev=false) # compile @test 0 == @allocations sort!(i, rev=false) rand!(i) @test 0 == @allocations sort!(i, order=Base.Reverse) From 4191e27f71ee7a32e5e6b2a932603ce341070ed1 Mon Sep 17 00:00:00 2001 From: Lilith Hafner Date: Thu, 22 Dec 2022 17:32:43 -0600 Subject: [PATCH 5/8] fixup: fix typo in added tests --- test/sorting.jl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/sorting.jl b/test/sorting.jl index 7706f6a8073bc..c4e324a61cde9 100644 --- a/test/sorting.jl +++ b/test/sorting.jl @@ -925,8 +925,8 @@ function test_allocs() @test 0 == @allocations sortperm!(i, v) @test 0 == @allocations sort!(i) @test 0 == @allocations sortperm!(i, v, rev=true) - @test 1 == @allocations sortperm(i, v, rev=true) - @test 1 == @allocations sortperm(i, v, rev=false) + @test 1 == @allocations sortperm(v, rev=true) + @test 1 == @allocations sortperm(v, rev=false) @test 0 == @allocations sortperm!(i, v, order=Base.Reverse) @test 1 == @allocations sortperm(v) @test 1 == @allocations sortperm(i, by=sqrt) From 9616d059815045737ea5521fcf5f4df624eb21a1 Mon Sep 17 00:00:00 2001 From: Lilith Orion Hafner Date: Tue, 27 Dec 2022 10:23:19 -0600 Subject: [PATCH 6/8] Remove the hacks Thanks @N5N3 for pointing out that they are unnecessary --- base/sort.jl | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/base/sort.jl b/base/sort.jl index c02669b9aafe0..06403e33303d1 100644 --- a/base/sort.jl +++ b/base/sort.jl @@ -1578,7 +1578,7 @@ function _sortperm(A::AbstractArray; alg, order, scratch, dims...) end end end - ix = unsafe_copymutable_LinearIndices(A) + ix = copymutable(LinearIndices(A)) sort!(ix; alg, order = Perm(order, vec(A)), scratch, dims...) end @@ -1631,10 +1631,10 @@ julia> sortperm!(p, A; dims=2); p scratch::Union{Vector{T}, Nothing}=nothing, dims...) where T <: Integer #to optionally specify dims argument (typeof(A) <: AbstractVector) == (:dims in keys(dims)) && throw(ArgumentError("Dims argument incorrect for type $(typeof(A))")) - axes(ix) == axes(A) || @noinline throw_sortperm_axis_mismatch_error(ix, A) + axes(ix) == axes(A) || throw(ArgumentError("index array must have the same size/axes as the source array, $(axes(ix)) != $(axes(A))")) if !initialized - very_unsafe_copyto!(ix, LinearIndices(A)) + @noinline copyto!(ix, LinearIndices(A)) end if rev === true @@ -1644,13 +1644,6 @@ julia> sortperm!(p, A; dims=2); p end end -# TODO stop using these three hacks -# but check performance, especially unexpected allocations, when removing -Base.@assume_effects :nothrow very_unsafe_copyto!(a, b) = a .= b -Base.@assume_effects :nothrow unsafe_copymutable_LinearIndices(A) = copymutable(LinearIndices(A)) -throw_sortperm_axis_mismatch_error(ix, A) = - throw(ArgumentError("index array must have the same size/axes as the source array, $(axes(ix)) != $(axes(A))")) - # sortperm for vectors of few unique integers function sortperm_int_range(x::Vector{<:Integer}, rangelen, minval) offs = 1 - minval From da6d39deafd4b68ed9f8a75a021584f688c08c07 Mon Sep 17 00:00:00 2001 From: Lilith Hafner Date: Tue, 27 Dec 2022 10:32:11 -0600 Subject: [PATCH 7/8] revert more superfluous changes --- base/sort.jl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/base/sort.jl b/base/sort.jl index 06403e33303d1..64bbacbb363b1 100644 --- a/base/sort.jl +++ b/base/sort.jl @@ -1621,7 +1621,7 @@ julia> sortperm!(p, A; dims=2); p 2 4 ``` """ -@inline function sortperm!(ix::AbstractArray{T}, A::AbstractArray; +function sortperm!(ix::AbstractArray{T}, A::AbstractArray; alg::Algorithm=DEFAULT_UNSTABLE, lt=isless, by=identity, @@ -1634,7 +1634,7 @@ julia> sortperm!(p, A; dims=2); p axes(ix) == axes(A) || throw(ArgumentError("index array must have the same size/axes as the source array, $(axes(ix)) != $(axes(A))")) if !initialized - @noinline copyto!(ix, LinearIndices(A)) + ix .= LinearIndexes(A) end if rev === true From 2a98fbbc94d836430a99c102cd9fb8182610e654 Mon Sep 17 00:00:00 2001 From: Lilith Hafner Date: Tue, 27 Dec 2022 10:42:14 -0600 Subject: [PATCH 8/8] fixup & put back `@inline` because it is sometimes necessary for performace --- base/sort.jl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/base/sort.jl b/base/sort.jl index 64bbacbb363b1..669d2d97b2ac1 100644 --- a/base/sort.jl +++ b/base/sort.jl @@ -1621,7 +1621,7 @@ julia> sortperm!(p, A; dims=2); p 2 4 ``` """ -function sortperm!(ix::AbstractArray{T}, A::AbstractArray; +@inline function sortperm!(ix::AbstractArray{T}, A::AbstractArray; alg::Algorithm=DEFAULT_UNSTABLE, lt=isless, by=identity, @@ -1634,7 +1634,7 @@ function sortperm!(ix::AbstractArray{T}, A::AbstractArray; axes(ix) == axes(A) || throw(ArgumentError("index array must have the same size/axes as the source array, $(axes(ix)) != $(axes(A))")) if !initialized - ix .= LinearIndexes(A) + ix .= LinearIndices(A) end if rev === true