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

spzeros and speye are really slow #22110

Closed
jebej opened this issue May 28, 2017 · 9 comments
Closed

spzeros and speye are really slow #22110

jebej opened this issue May 28, 2017 · 9 comments
Labels
performance Must go faster sparse Sparse arrays

Comments

@jebej
Copy link
Contributor

jebej commented May 28, 2017

I searched and couldn't find any open issue on this, which surprised me.

Not too sure what the problem is with the code in Base, but if I write my own spzeros, it is much faster (the same thing happens with speye):

julia> using BenchmarkTools

julia> @benchmark spzeros(5,5)
BenchmarkTools.Trial:
  memory estimate:  1.11 KiB
  allocs estimate:  18
  --------------
  minimum time:     9.330 μs (0.00% GC)
  median time:      9.953 μs (0.00% GC)
  mean time:        10.215 μs (0.00% GC)
  maximum time:     60.647 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1

julia> function my_spzeros(m,n)
           ((m < 0) || (n < 0)) && throw(ArgumentError("invalid Array dimensions"))
           rowval = Vector{Int}(0)
           colptr = ones(Int,n+1)
           nzval  = Vector{Float64}(0)
           return SparseMatrixCSC(m,n,colptr,rowval,nzval)
       end
my_spzeros (generic function with 1 method)

julia> @benchmark my_spzeros(5,5)
BenchmarkTools.Trial:
  memory estimate:  336 bytes
  allocs estimate:  4
  --------------
  minimum time:     88.951 ns (0.00% GC)
  median time:      97.654 ns (0.00% GC)
  mean time:        117.422 ns (13.87% GC)
  maximum time:     1.565 μs (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     965
@nalimilan
Copy link
Member

Good catch. This is because spzeros(5, 5) ends up calling spzeros(Float64, Int, 5, 5), and the two first arguments to this method are not specialized on. Changing the signature of the method from

myspzeros(Tv::Type, Ti::Type, m::Integer, n::Integer)

to:

myspzeros(::Type{Tv}, ::Type{Ti}, m::Integer, n::Integer) where {Ti, Tv}

fixes the problem.

Would you make a pull request to make this change?

@jebej
Copy link
Contributor Author

jebej commented May 28, 2017

It seems to be an issue with having the type arguments being function calls.

I have actually noticed this problem in my own code. In this function, if I just call ptrace in the type constructor performance drops by a factor of 20.

@jebej
Copy link
Contributor Author

jebej commented May 28, 2017

Hm, that does make sense. What problem am I seeing with my code then?

@jebej
Copy link
Contributor Author

jebej commented May 28, 2017

Just to be extra clear what I am doing is replacing

function ptrace::Density, out)
    res = ptrace(full(ρ),out,dims(ρ))
    return Density(res[1],res[2])
end

with

function ptrace::Density, out)
    return Density(ptrace(full(ρ),out,dims(ρ))...)
end

@KristofferC
Copy link
Member

Splatting penalty. Not related to the sparse issue, see: #13359.

@jebej
Copy link
Contributor Author

jebej commented May 28, 2017

I see. Is that penalty also expected if I was doing:

function ptrace::Density, out)
    res = ptrace(full(ρ),out,dims(ρ))
    return Density(res...)
end

This last form is slower than writing out res[1],res[2], but by 20ns 6ns, not microseconds.
Edit: it's pretty much the same after trying a few times.

@KristofferC
Copy link
Member

I am surprised there is a difference between the two.

@jebej
Copy link
Contributor Author

jebej commented May 28, 2017

With the example in the docstrings, I get:

Density(res[1],res[2])

julia> @benchmark ptrace($ρ,2)
BenchmarkTools.Trial:
  memory estimate:  592 bytes
  allocs estimate:  7
  --------------
  minimum time:     283.362 ns (0.00% GC)
  median time:      300.149 ns (0.00% GC)
  mean time:        358.231 ns (14.33% GC)
  maximum time:     7.924 μs (94.38% GC)
  --------------
  samples:          10000
  evals/sample:     315

Density(ptrace(full(ρ),out,dims(ρ))...)

julia> @benchmark ptrace($ρ,2)
BenchmarkTools.Trial:
  memory estimate:  608 bytes
  allocs estimate:  8
  --------------
  minimum time:     6.531 μs (0.00% GC)
  median time:      6.842 μs (0.00% GC)
  mean time:        7.013 μs (0.69% GC)
  maximum time:     501.158 μs (96.20% GC)
  --------------
  samples:          10000
  evals/sample:     5

Density(res...)

julia> @benchmark ptrace($ρ,2)
BenchmarkTools.Trial:
  memory estimate:  592 bytes
  allocs estimate:  7
  --------------
  minimum time:     283.185 ns (0.00% GC)
  median time:      306.042 ns (0.00% GC)
  mean time:        365.921 ns (14.18% GC)
  maximum time:     9.788 μs (95.12% GC)
  --------------
  samples:          10000
  evals/sample:     313

@jebej
Copy link
Contributor Author

jebej commented May 28, 2017

So if I understand correctly, the issue only occurs when splatting directly on a function call?

@kshyatt kshyatt added performance Must go faster sparse Sparse arrays labels May 29, 2017
tkelman pushed a commit that referenced this issue Jun 3, 2017
* Fix type sparse type parameter (fixes #22110)

* No brackets for singular type param

(cherry picked from commit 7b22adb)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Must go faster sparse Sparse arrays
Projects
None yet
Development

No branches or pull requests

4 participants