sparse findnext findprev hash performance improved

[KlausC]

Specialized version of several find functions and hash.
To demonstrate the difference, hash is called for different sparse matrices. hash is calling findprev multiple times.

Before - the time is growing with decreasing fill ratio.

julia> n = 10^5;
julia> A = sprand(rng, n, n, 10^8/n^2);
julia> @btime hash(A)
  129.376 ms (1 allocation: 16 bytes)
0x4c30d5f8ae24e7c4

julia> A = sprand(rng, n, n, 10^7/n^2);
julia> @btime hash(A)
  788.578 ms (1 allocation: 16 bytes)
0x7143db6e266eb8d8

julia> A = sprand(rng, n, n, 10^6/n^2);
julia> @btime hash(A)
  5.574 s (1 allocation: 16 bytes)
0x1ff7a9421112310d

julia> A = sprand(rng, n, n, 10^5/n^2);
julia> @btime hash(A)
  37.960 s (1 allocation: 16 bytes)
0x1083a7abc61aa2d2

After - the time is decreasing with decreasing fill ratio and generally lower.

julia> n = 10^5;
julia> A = sprand(rng, n, n, 10^8/n^2);
julia> @btime hash(A)
  29.570 ms (1 allocation: 16 bytes)
0x4c30d5f8ae24e7c4

julia> A = sprand(rng, n, n, 10^7/n^2);
julia> @btime hash(A)
  17.184 ms (1 allocation: 16 bytes)
0x7143db6e266eb8d8

julia> A = sprand(rng, n, n, 10^6/n^2);
julia> @btime hash(A)
  10.305 ms (1 allocation: 16 bytes)
0x1ff7a9421112310d

julia> A = sprand(rng, n, n, 10^5/n^2);
julia> @btime hash(A)
  8.319 ms (1 allocation: 16 bytes)
0x1083a7abc61aa2d2

julia> A = sprand(rng, n, n, 10^4/n^2);
julia> @btime hash(A)
  1.415 ms (1 allocation: 16 bytes)
0x02799c131c82e605

[KlausC]

AppVeyor fault seems unrelated.

[ViralBShah]

@KlausC All your PRs seem to have a lot of commits that seem to be unrelated and clearly don't show up when I merge.

I wonder if you are doing something differently with git, which produces these massive commit histories and messages. Perhaps someone knows what the issue is.

[yuyichao]

He just always use merge to sync with master and never rebased.

[KlausC]

@yuyichao That is right. I would like to insert a git rebase, if I'd know how to do that.
That is my current workstream:

git checkout master
git fetch upstream
git checkout master
git merge upstream/master
git branch newbranch
git checkout newbranch
... edit ...
git push ...

[ViralBShah]

When you are ready to submit the PR, you may want to do git rebase master on your branch. Not sure if that is sufficient to fix this. You can try it here and force push to your branch.

[mauro3]

The workflow should be something like:

git checkout master
git pull # now your master is the same as upstream master.  Don't edit your master.
git checkout -b krc/newbranch
# work work

# if you need to get changes from upstream/master (only when there are conflicts):
git fetch upstream
git rebase upstream/master

Once done with a PR it is often nice to condense the PR into one or several important commits (each commit should pass the tests). For this use git rebase -i 123 where 123 is the sha of the commit just before you started work (if I recall correctly).

To get out of your current affliction, it's probably easiest if you copy your stdlib/SparseArrays/src/sparsematrix.jl to somewhere, then do:

cp stdlib/SparseArrays/src/sparsematrix.jl /tmp/
git branch krc/findnext-backup # make a backup-branch in case something goes haywire
git fetch upstream/master
git reset --hard upstream/master # now your krc/findnext branch is identical to upstream/master
                                 # (be careful with reset --hard, it destroys stuff...)
cp /tmp/sparsematrix.jl stdlib/SparseArrays/src/sparsematrix.jl
git commit -am "..."
git push -f # fixes what's here on github

[KlausC]

@mauro3 , thank you very much! git looks clean now. I will change my work flow according to your recommendation.

@ViralBShah, rebase master alone did not fix it:

$ git rebase master
Current branch krc/findnext is up to date.

[ViralBShah]

Thanks @KlausC. This looks so much cleaner. And thanks everyone for the help here.

[ViralBShah]

Can we have some tests for the sparse cases for findprev/next/etc?

Also, while it basically looks good to me, it would be good to get an extra pair of eyes on this PR.

[KlausC]

Can we have some tests for the sparse cases for findprev/next/etc?

I thought, the existing tests might be sufficient:
hash: ( especially the critical -0.0 case is included)

julia/test/hashing.jl

Lines 126 to 147 in 10141d3

	# various stored zeros patterns
	sparse([1], [1], [0]), sparse([1], [1], [-0.0]),
	sparse([1, 2], [1, 1], [-0.0, 0.0]), sparse([1, 2], [1, 1], [0.0, -0.0]),
	sparse([1, 2], [1, 1], [-0.0, 0.0], 3, 1), sparse([1, 2], [1, 1], [0.0, -0.0], 3, 1),
	sparse([1, 3], [1, 1], [-0.0, 0.0], 3, 1), sparse([1, 3], [1, 1], [0.0, -0.0], 3, 1),
	sparse([1, 2, 3], [1, 1, 1], [-1, 0, 1], 3, 1), sparse([1, 2, 3], [1, 1, 1], [-1.0, -0.0, 1.0], 3, 1),
	sparse([1, 3], [1, 1], [-1, 0], 3, 1), sparse([1, 2], [1, 1], [-1, 0], 3, 1)
	]
	for a in vals
	b = Array(a)
	@test hash(convert(Array{Any}, a)) == hash(b)
	@test hash(convert(Array{supertype(eltype(a))}, a)) == hash(b)
	@test hash(convert(Array{Float64}, a)) == hash(b)
	@test hash(sparse(a)) == hash(b)
	if !any(x -> isequal(x, -0.0), a)
	@test hash(convert(Array{Int}, a)) == hash(b)
	if all(x -> typemin(Int8) <= x <= typemax(Int8), a)
	@test hash(convert(Array{Int8}, a)) == hash(b)
	end
	end
	end

findnext/findprev:

julia/stdlib/SparseArrays/test/sparse.jl

Lines 2248 to 2258 in 10141d3

	y = [0 0 0 0 0;
	1 0 1 0 0;
	1 0 0 0 1;
	0 0 1 0 0;
	1 0 1 1 0]
	y_sp = sparse(y)
	for i in keys(y)
	@test findnext(!iszero, y,i) == findnext(!iszero, y_sp,i)
	@test findprev(!iszero, y,i) == findprev(!iszero, y_sp,i)
	end

[KlausC]

Also, while it basically looks good to me, it would be good to get an extra pair of eyes on this PR.

@andreasnoack, who would be interested?

[ViralBShah]

Maybe one of @KristofferC or @mbauman ?

[nalimilan]

AFAICT tests should include something like findnext(iszero, y, k) to cover theif pred(zero(eltype(A))) path. I also suspect they should test a matrix with stored zeros (both 0.0 and -0.0).

[nalimilan]

Why define this here rather than above with other _idx functions?

[nalimilan]

findfirst and findlast aren't overloaded here. Also, this file seems to use Base.f rather than import Base: f.

[KlausC]

Thanks for the review - I added the missing tests and made recommended changes.

[KlausC]

bump :-)

[ViralBShah]

Actually, I realized that @mbauman had a similar PR. @mbauman Can you review this - even though it is merged?

[mbauman]

Yes, this looks quite good to me. My only review comment would be in the naming of some of those _ functions (I want a slightly better word for that index into nzval than idx), but they're underscore functions and I don't have any great ideas so I suppose I can't complain too much. :)

Thanks @KlausC!

[tkluck]

Hi there -- this pull request seems related to a noticeable slowdown I'm observing in my own use case. Could you help me see if we can fix that?

My use case is sparse matrices of polynomials, but the same issue exists for sparse matrices of e.g. BigInt or any other type that exists on the heap. The reason is that this PR introduces the following:

• z = zero(<type>); ....; isequal(z, <something>).

Any reason not to use iszero?

• Replacing dispatch based on f::typeof(!iszero) by an in-code check for pred(zero(<type>)). This latter thing is much more general, but in the case of BigInt (or my personal case of polynomials), it can't currently be elided by the compiler.

In my case, both of these things work together in such a way that the allocation of zero(<type>) is now by far the slowest part of my findnext calls. Eyeballing the flamegraph says it takes 80-90% of the time:

Disclose, I wrote the previous incarnations of this in #23317, so in that sense it's not a surprise that my use case works better with the code I optimized for it. Would you mind helping me to make both cases work?

Two more profiling screenshots to illustrate this:

(FWIW, this looks like it fulfills a very similar function to _sparse_findnextnz.)

[tkluck]

It actually might make sense to look a bit deeper and see what's the difference between this PR and #23317 at all. It seems like they're both very similar and duplicate behaviour. My gut feeling is that it should have been possible to reach the stated objective of hash performance with only minor fixes to that other one.

[tkluck]

I feel really bad, but I just submitted a revert of this pull request + a "fixed" (from my point of view) implementation in #32007 . I'd appreciate any thoughts you have either here or there.