Re: [eigen] Mapping array of scalars into quaternions

[Mathieu Gautier <mathieu.gautier@xxxxxx>]

> Everytime you add a EIGEN_STRONG_INLINE on a function that has a
> nontrivial body of its own (rather than just forwarding the call to
> another function), I need to see some justification (as far as I can
> see, that would have to be justified by performance measurements). I'm
> by no means an expert, i'm really honestly asking. My fear is about
> uselessly increasing code size (and killing instruction caches).

OK, I perform some benchmark on simple operations with gcc4.4, icc and vc++

1/ q = q1*q2;
2/ q = q1*q2*q3.inverse()*q4;
3/ v = q._transformVector(v1);

(v : vector, q : quaternion)

using EIGEN_STRONG_INLINE didn't improve the performance with vc++. The 
asm generated is never completly inlined both with inline and 
EIGEN_STRONG_INLINE. It's also have no influence with gcc, since the asm 
generated is always fully inlined.

with icc (disabling any SSE extension) I have the following results for 
10 trials of 100,000,000 operations on a Intel xeon X5550:

mean for 100,000,000 operations

    inline | EIGEN_STRONG_INLINE
1/   2.22s |	1.70s
2/   8.21s |	6.24s
3/   2.63s |	2.02s

the ratio is roughly the same (0.76).

using EIGEN_STRONG_INLINE instead of inline, inline properly all 
function call.

But, we don't use this kind of function very often in our project. So, I 
 have not a strong requirement for using EIGEN_STRONG_INLINE. If you 
think it's too harmfull to use this instruction, its not a problem for me.

 > I also have a few superficial comments:
> First, "inline" keywords need only be put on function definitions, no
> need to put them on function definitions [1]. I realize that
> Quaternion.h already had this problem before your patch. I just am
> noticing it now, because EIGEN_STRONG_INLINE takes more space on my
> screen.

:) When using icc, EIGEN_STRONG_INLINE is only effective if put in class 
body. It must be specific to the "__force_inline" keywords. "inline" 
keyword may be remove from the class body and only put in front of 
method definition.

> Second, in the unit test (thanks for writing it), you could just
> reserve static arrays with EIGEN_ALIGN16, see what we do in
> MatrixStorage.h.
ok

> Third, when you significantly extend/rewrite a file, add yourself a
> copyright line. I already added you one on Quaternion.h, also do it on
> quaternion.cpp.

I'll change these to points and I'm waiting a reply about inlining to 
send a new patch.

--
Mathieu