Replace use of D1Matrix64F.get/set by more performant alternatives if possible

[GoogleCodeExporter]

For performance reasons internal classes in the EJML library (e.g. in 
MatrixMatrixMult) should not call D1Matrix64F.get() / .set() but use more 
efficient versions of these methods where available, e.g. 
DenseMatrix64F.unsafe_get() / .unsafe_set().

Internal classes are properly unit-tested so using unsafe versions of getters 
and setters is a safe way to improve performance. This would obviously require 
different implementations of an algorithm for different concrete matrix types.

Original issue reported on code.google.com by kaspar.thommen@gmail.com on 23 Nov 2011 at 10:34

[GoogleCodeExporter]

1. Clarification: I was referring to the get(int, int) and set(int, int) 
methods.

2. Correction: MatrixMatrixMult was a bad exmaple as it does not contain calls 
to these methods, but many other classes in org.ejml.alg.dense contain 
references to get(int, int) or set(int, int), which could be replaced by 
unsafe_get(int, int) and unsafe_set(int, int) for DenseMatrix64, the most 
commonly used matrix type.

Original comment by kaspar.thommen@gmail.com on 23 Nov 2011 at 10:58

[GoogleCodeExporter]

Do you see any place where calls to set(int,int)/get(int,int) are being called 
in the inner most loops?  Just noticed that insert/extract does that and that 
is being changed.  

In an ideal world, all calls would be made to those routines since it is 
possible to catch simple errors that would otherwise go unnoticed.  There might 
be a few places where those calls were left in place because it was thought 
that it would not effect performance.

Original comment by peter.ab...@gmail.com on 23 Nov 2011 at 12:24

[GoogleCodeExporter]

There are other instances too, e.g. in CommonOpts, MatrixComponent, 
MatrixFeatures, plus most decomposition classes.

Original comment by kaspar.thommen@gmail.com on 23 Nov 2011 at 12:36

[GoogleCodeExporter]

Some places inside of CommonOps might benefit from the change.  Other places, 
like in MatrixComponent, would not.  Inside of MatrixComponent it works with 
Graphics2D at the pixel level, which is "slowish" and rendering for 
visualization tends to not be a time critical task.  The general rule is that 
unless there is a good reason to change it is better to use the safe method.

Interested in check out some functions inside of CommonOps and benchmarking 
them across a range of matrix sizes to see if they would benefit from the 
change?

For examples of how to benchmark see "benchmarks/src/org/ejml/*". 

Original comment by peter.ab...@gmail.com on 24 Nov 2011 at 5:59

[GoogleCodeExporter]

I agree on MatrixComponent, but I think MatrixFeatures, e.g. isSymmetric(), 
could benefit if the method is called often.

I kinda disagree with "don't change because it's safer" for the following 
reasons:
- The bounds checks made in get(int, int) and set(int, int) can be moved 
outside of the  inner loops without sacrificing safety at all.
- It is library code, and library code "is allowed" to use unsafe methods 
because the library "knows what it's doing".
- Library code should be very well unit-tested to avoid any errors introduced 
by using unsafe methods.

Anyway, up to you really, but I think the changes might make a difference in 
benchmarks (I haven't run any), and by simply moving the bound check out of the 
inner loops you don't sacrifice any safety.

Original comment by kaspar.thommen@gmail.com on 25 Nov 2011 at 9:27

[GoogleCodeExporter]

Its very hard to prove that code is correct in all situations.  In EJML I try 
to test as many situations as possible but even then I do occasionally find 
nasty bugs in code that was deemed to be correct.  If you want to have fun take 
a moderate sized C/C++ program which is "bug free" that no one has run a memory 
checker on and see what valgrind finds.  valgrind does a good job of detecting 
buffer overruns and bad pointers.

I'm going to close this ticket, but there are probably some simple functions 
that would be improved with this change.

Original comment by peter.ab...@gmail.com on 2 Dec 2011 at 2:56

• Changed state: Done