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

WIP: use mutating matrix multiplication in eigs #7907

Closed
wants to merge 1 commit into from

Conversation

Jutho
Copy link
Contributor

@Jutho Jutho commented Aug 8, 2014

Currently, the arpack wrapper of eigs allocates way too much memory. Firstly, no mutating methods are used, so every matrix multiplication creates (allocates) a new output, which is then copied to the position requested by Arpack. Secondly, for every new matrix multiplication, a new inputvector x=workd[load_idx] is created (allocated). So that is two vector allocations per matrix multiplication, both of which could be avoided. The second one will automatically go away when indexing creates a view. The first one requires to switch to mutating methods whenever possible (e.g. whenever defined for the type of A).

This is a work in progress, since so far only the standard mode is covered (i.e. no shift-invert, no generalised eigenvalue problems, so this will fail the tests). Also, I currently use pointer and pointer_to_array to create a view, but a simple slice would probably have been ok.

@ViralBShah
Copy link
Member

Thanks for tackling this. This is much needed!

@ViralBShah
Copy link
Member

Does using SubArray not work instead of pointer_to_array? It seems that the various mul methods do work with SubArray, so it should work out.

@Jutho
Copy link
Contributor Author

Jutho commented Aug 8, 2014

Yes it would. I was just doing a lot of pointer work in some code of mine, where sub or slice would not work, and while doing so I also changed this and did not even think of using SubArray.

I will fix this if I complete this PR. I guess there is no need to hurry with this, i.e. it will be a 0.4 thing, together with eigs accepting function handles? I don't have much time for it right now. I just patched the standard case now since that's what I needed.

@ViralBShah
Copy link
Member

Yes, definitely 0.4. Main things are this performance enhancement, operator functions instead of matrix inputs, and then svds. That would complete this whole feature set. Once we have multi-threading, the matvecs could be easily parallel by default.

@jiahao jiahao force-pushed the master branch 2 times, most recently from 2ef98c5 to 0388647 Compare October 5, 2014 00:57
@ViralBShah
Copy link
Member

@Jutho Do you think this PR should be updated to use SubArrays or should we merge it as is? I am assuming the performance difference is notable.

@ViralBShah ViralBShah added performance Must go faster linear algebra Linear algebra labels Oct 8, 2014
@ViralBShah ViralBShah added this to the 0.4 milestone Oct 8, 2014
@Jutho
Copy link
Contributor Author

Jutho commented Oct 8, 2014

This is far from finished. If I have some time, I will try to finish it, but it will probably not be until next week.

@ViralBShah
Copy link
Member

I believe that with the recent subarray work and indexing operators defaulting to views, this should no longer be necessary.

@Jutho
Copy link
Contributor Author

Jutho commented Dec 21, 2014

Correct.

@Jutho
Copy link
Contributor Author

Jutho commented Dec 21, 2014

Although, with the current implementation, every matrix vector multiplication will still allocate a new vector for the result (but no longer for the input, which is obtained by indexing into a larger array), and only then copy it to the correct position. So there would still be the benefit of using A_mul_B! for matrices and these types which support it.

@ViralBShah
Copy link
Member

I see - matvecA will allocate. We really must use A_mul_B! and A_div_B!.

@ViralBShah
Copy link
Member

Some svds discussion for reference: https://groups.google.com/forum/#!topic/julia-users/YEg_k2JjtmY

@Jutho
Copy link
Contributor Author

Jutho commented Feb 6, 2015

This is on my todo list. I'll make some time for this after I have finished the improved cat PR. It will also be interesting to see how much can still be gained from this now that we have the new GC.

@ViralBShah
Copy link
Member

I was wondering exactly the same thing myself about the new GC. Perhaps it is worth setting up some performance benchmarks, which will otherwise be good to have too - before doing a whole bunch of work here.

@ViralBShah
Copy link
Member

For this problem, I see almost the same time and memory allocation:

Julia 0.3: 28 seconds (5.4GB allocated)
Julia 0.4-dev: 25 seconds (5.1GB allocated)

julia> s = sprand(10^5,10^5,5/10^5);
julia> @time eigs(s);
elapsed time: 25.280660173 seconds (5181 MB allocated, 1.70% gc time in 234 pauses with 0 full sweep)

Octave took 24 seconds, but I am not sure if Julia and octave are running the same number of iterations, and are comparable.

@tkelman
Copy link
Contributor

tkelman commented May 12, 2016

closing in favor of #16294

@tkelman tkelman closed this May 12, 2016
@Jutho Jutho deleted the jh/eigsmutating branch August 23, 2018 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra performance Must go faster
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants