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

Optimize and improve dispatch to sort* #303

Merged
merged 1 commit into from
Dec 4, 2022

Conversation

LilithHafner
Copy link
Member

@LilithHafner LilithHafner commented Nov 28, 2022

This is motivated by #302 and fixes it for ::AbstractSparseVector but does address the sparse matrix case.

Before:

julia> using SparseArrays, StableRNGs, BenchmarkTools

julia> x = sprand(StableRNG(8), 30, .1)
30-element SparseVector{Float64, Int64} with 2 stored entries:
  [6 ]  =  0.14869
  [25]  =  0.0341937

julia> sort!(x)
30-element SparseVector{Float64, Int64} with 25 stored entries:
  [6 ]  =  0.0
  [7 ]  =  0.0
  [8 ]  =  0.0
  [9 ]  =  0.0
        
  [26]  =  0.0
  [27]  =  0.0
  [28]  =  0.0
  [29]  =  0.0341937
  [30]  =  0.14869

julia> @btime sort(x) setup = (x = sprand(1000, .01));
  782.907 ns (10 allocations: 544 bytes)

julia> @btime sort!(x) setup = (x = sprand(1000, .01));
ERROR: `reinterpret` on sparse arrays is discontinued.
Try reinterpreting the value itself instead.

Stacktrace:
  [1] error(s::String)
    @ Base ./error.jl:35
  [2] reinterpret(#unused#::Type, A::SparseVector{Float64, Int64})
    @ SparseArrays ~/.julia/dev/julia_master/usr/share/julia/stdlib/v1.10/SparseArrays/src/abstractsparse.jl:79
  [3] uint_map!(v::SparseVector{Float64, Int64}, lo::Int64, hi::Int64, order::Base.Sort.Float.Right)
    @ Base.Sort ./sort.jl:1425
  [4] sort!(v::SparseVector{Float64, Int64}, lo::Int64, hi::Int64, ::Base.Sort.AdaptiveSortAlg, o::Base.Sort.Float.Right, t::Nothing)
    @ Base.Sort ./sort.jl:844
  [5] fpsort!(v::SparseVector{Float64, Int64}, a::Base.Sort.AdaptiveSortAlg, o::Base.Order.ForwardOrdering, t::Nothing)
    @ Base.Sort.Float ./sort.jl:1588
  [6] sort!
    @ ./sort.jl:1595 [inlined]
  [7] #sort!#11
    @ ./sort.jl:935 [inlined]
  [8] sort!
    @ ./sort.jl:928 [inlined]
  [9] var"##core#305"(x::SparseVector{Float64, Int64})
    @ Main ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:489
 [10] var"##sample#306"(::Tuple{}, __params::BenchmarkTools.Parameters)
    @ Main ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:495
 [11] _run(b::BenchmarkTools.Benchmark, p::BenchmarkTools.Parameters; verbose::Bool, pad::String, kwargs::Base.Pairs{Symbol, Integer, NTuple{4, Symbol}, NamedTuple{(:samples, :evals, :gctrial, :gcsample), Tuple{Int64, Int64, Bool, Bool}}})
    @ BenchmarkTools ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:99
 [12] #invokelatest#2
    @ ./essentials.jl:818 [inlined]
 [13] invokelatest
    @ ./essentials.jl:813 [inlined]
 [14] #run_result#45
    @ ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:34 [inlined]
 [15] run_result
    @ ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:34 [inlined]
 [16] run(b::BenchmarkTools.Benchmark, p::BenchmarkTools.Parameters; progressid::Nothing, nleaves::Float64, ndone::Float64, kwargs::Base.Pairs{Symbol, Integer, NTuple{5, Symbol}, NamedTuple{(:verbose, :samples, :evals, :gctrial, :gcsample), Tuple{Bool, Int64, Int64, Bool, Bool}}})
    @ BenchmarkTools ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:117
 [17] run (repeats 2 times)
    @ ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:117 [inlined]
 [18] #warmup#54
    @ ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:169 [inlined]
 [19] warmup(item::BenchmarkTools.Benchmark)
    @ BenchmarkTools ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:168
 [20] top-level scope
    @ ~/.julia/packages/BenchmarkTools/0owsb/src/execution.jl:575

After

julia> using SparseArrays, StableRNGs, BenchmarkTools

julia> x = sprand(StableRNG(8), 30, .1)
30-element SparseVector{Float64, Int64} with 2 stored entries:
  [6 ]  =  0.14869
  [25]  =  0.0341937

julia> sort!(x)
30-element SparseVector{Float64, Int64} with 2 stored entries:
  [29]  =  0.0341937
  [30]  =  0.14869

julia> @btime sort(x) setup = (x = sprand(1000, .01));
  161.197 ns (2 allocations: 128 bytes)

julia> @btime sort!(x) setup = (x = sprand(1000, .01));
  26.352 ns (0 allocations: 0 bytes)

Tests are on Julia master.

@codecov-commenter
Copy link

codecov-commenter commented Nov 28, 2022

Codecov Report

Merging #303 (ce70d54) into main (2445435) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #303   +/-   ##
=======================================
  Coverage   93.79%   93.79%           
=======================================
  Files          12       12           
  Lines        7351     7351           
=======================================
  Hits         6895     6895           
  Misses        456      456           
Impacted Files Coverage Δ
src/sparsevector.jl 95.01% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@dkarrasch
Copy link
Member

This is ready to go, isn't it?

@LilithHafner
Copy link
Member Author

Yes; I was waiting for an approving review.

@dkarrasch dkarrasch merged commit 57cbb74 into JuliaSparse:main Dec 4, 2022
@LilithHafner LilithHafner deleted the sort branch December 4, 2022 09:27
dkarrasch added a commit that referenced this pull request Mar 6, 2023
#303 should've been backported as well, before #335.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants