search

sofa-framework / sofa

Real-time multi-physics simulation with an emphasis on medical simulation.
https://www.sofa-framework.org
GNU Lesser General Public License v2.1
871 stars 297 forks source link

[Sofa.Type] Fix/clean and speed up of Mat #3280

Closed alxbilger closed 1 year ago

alxbilger commented 1 year ago

Explicit instantiations of common Mat sizes led to compilation errors. For example, z() throws a static_assert for Mat<2,2,> because there is only 2 elements. Now, x, y, z and w are compiled conditionally. Other methods, such as Identity, transpose etc, are also concerned for squared matrices. The static_assert are therefore no longer useful.

Identity has been changed so it is static constexpr and it returns a reference. So there is no need to compute the identity matrix each time it is called. s_identity is now deprecated. The static variable is now included in the Identity method. It is better for static initialization fiasco and required for a constexpr context. Also, s_identity does not make sense for rectangular matrices. Since it is not possible to conditionally add a class member, it is hidden in the conditionally compiled method Identity.

In fixed_array, the operator< is moved out of the class. This allows to compile this operator only when used, and not in explicit instantiations. This is necessary because the operator on fixed_array<Vec3> is not implemented (and does not make sense).

The 3x3 matrix product and the multTranspose operation are specialized for a speedup. https://github.com/alxbilger/SofaBenchmark was used. Results show that this implementation is closer to the Eigen implementation.

Unit tests are added.

[ci-depends-on https://github.com/sofa-framework/SofaPython3/pull/302]

By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).

Reviewers will merge this pull-request only if

sofabot commented 1 year ago

[ci-depends-on] detected during build #1.

To unlock the merge button, you must

damienmarchal commented 1 year ago

I think the [depend-on] is used in the wrong "direction".

alxbilger commented 1 year ago

Before the changes:

------------------------------------------------------------------------------
Benchmark                                    Time             CPU   Iterations
------------------------------------------------------------------------------
BM_Matrix_typematf_matmult<3>/128         1.27 us         1.28 us       560000
BM_Matrix_typematf_matmult<3>/256         2.53 us         2.51 us       280000
BM_Matrix_typematf_matmult<3>/512         5.11 us         5.16 us       100000
BM_Matrix_eigenmatf_matmult<3>/128       0.923 us        0.900 us       746667
BM_Matrix_eigenmatf_matmult<3>/256        1.83 us         1.84 us       373333
BM_Matrix_eigenmatf_matmult<3>/512        3.67 us         3.68 us       186667

After the changes:

------------------------------------------------------------------------------
Benchmark                                    Time             CPU   Iterations
------------------------------------------------------------------------------
BM_Matrix_typematf_matmult<3>/128         1.00 us         1.00 us       640000
BM_Matrix_typematf_matmult<3>/256         1.98 us         1.97 us       373333
BM_Matrix_typematf_matmult<3>/512         3.93 us         3.90 us       172308
BM_Matrix_eigenmatf_matmult<3>/128        1.00 us         1.00 us       746667
BM_Matrix_eigenmatf_matmult<3>/256        1.99 us         2.01 us       373333
BM_Matrix_eigenmatf_matmult<3>/512        3.99 us         4.01 us       179200
sofabot commented 1 year ago

[ci-depends-on] detected during build #2.

To unlock the merge button, you must

fredroy commented 1 year ago

It does not come from your PR, but while trying to compile this branch with the newest gcc, there is an interesting (and useful) warning:

/home/fred/sofa/src/current/Sofa/framework/Type/src/sofa/type/Mat.h:408:58: warning: array subscript 2 is above array bounds of ‘const sofa::type::VecNoInit<3, double> [2]’ [-Warray-bounds]
  408 |                 if( rabs( this->elems[i][j] - this->elems[j][i] ) > EQUALITY_THRESHOLD ) return false;

Indeed, there is no compile-time test about the fact that the matrix is square or not (so the symmetry could be irrelevant)

sofabot commented 1 year ago

[ci-depends-on] detected during build #3.

To unlock the merge button, you must

sofabot commented 1 year ago

[ci-depends-on] detected during build #4.

To unlock the merge button, you must

alxbilger commented 1 year ago

It does not come from your PR, but while trying to compile this branch with the newest gcc, there is an interesting (and useful) warning:

/home/fred/sofa/src/current/Sofa/framework/Type/src/sofa/type/Mat.h:408:58: warning: array subscript 2 is above array bounds of ‘const sofa::type::VecNoInit<3, double> [2]’ [-Warray-bounds]
  408 |                 if( rabs( this->elems[i][j] - this->elems[j][i] ) > EQUALITY_THRESHOLD ) return false;

Indeed, there is no compile-time test about the fact that the matrix is square or not (so the symmetry could be irrelevant)

I added compile-time check that the matrix is square. Thanks for spotting it.

alxbilger commented 1 year ago

Now the multTranspose operation is faster than Eigen:

-----------------------------------------------------------------------------------
Benchmark                                         Time             CPU   Iterations
-----------------------------------------------------------------------------------
BM_Matrix_typemat3x3f_multTranspose/128       0.731 us        0.711 us       746667
BM_Matrix_typemat3x3f_multTranspose/256        1.53 us         1.54 us       497778
BM_Matrix_typemat3x3f_multTranspose/512        2.83 us         2.85 us       235789
BM_Matrix_eigenmat3x3f_multTranspose/128      0.842 us        0.837 us       746667
BM_Matrix_eigenmat3x3f_multTranspose/256       1.73 us         1.73 us       407273
BM_Matrix_eigenmat3x3f_multTranspose/512       3.46 us         3.45 us       194783
sofabot commented 1 year ago

[ci-depends-on] detected during build #5.

To unlock the merge button, you must

alxbilger commented 1 year ago

New benchmark:

------------------------------------------------------------------------------
Benchmark                                    Time             CPU   Iterations
------------------------------------------------------------------------------
BM_Matrix_typematf_matmult<3>/128        0.689 us        0.684 us      1120000
BM_Matrix_typematf_matmult<3>/256         1.28 us         1.29 us       497778
BM_Matrix_typematf_matmult<3>/512         2.76 us         2.73 us       263529
BM_Matrix_eigenmatf_matmult<3>/128        1.01 us         1.03 us       746667
BM_Matrix_eigenmatf_matmult<3>/256        1.93 us         1.95 us       344615
BM_Matrix_eigenmatf_matmult<3>/512        3.81 us         3.81 us       172308

We are faster than Eigen!

alxbilger commented 1 year ago

[ci-build][with-all-tests]

sofabot commented 1 year ago

[ci-depends-on] detected during build #6.

To unlock the merge button, you must

hugtalbot commented 1 year ago

very nice work @alxbilger and review @fredroy :+1:

damienmarchal commented 1 year ago

Ready, merge if [ci-build][with-all-tests] agrees.

sofabot commented 1 year ago

[ci-depends-on] detected during build #7.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1:

damienmarchal commented 1 year ago

Ready but conflicting with a recent merge... @alxbilger

sofabot commented 1 year ago

[ci-depends-on] detected during build #8.

All dependencies are merged/closed and all ExternalProject pointers are up-to-date. Congrats! :+1:

alxbilger commented 1 year ago

Two unit tests failing, probably solved in https://github.com/sofa-framework/sofa/pull/3322