Re: [eigen] ThreadSafety of Eigen?

[Benoit Jacob <jacob.benoit.1@xxxxxxxxx>]

2010/3/31 Björn Piltz <bjornpiltz@xxxxxxxxxxxxxx>:
> Some thoughts on the subject:
> Benoit is saying that Eigen is reentrant - as it should be. That is what
> most people mean by thread-safe, even if it isn't thread safe in the sense
> that different threads can safely manipulate the same matrix.
> However, this isn't entirely true.
> <begin nitpicking>
> There are a couple of problematic uses of static data in Eigen.
> There are numerous uses of const static global or member variables. e.g
> static _EIGEN_DECLARE_CONST_FAST_Packet4f(ZERO, 0);

OK, i didn't think of that. As you say it's not problematic as it
doesn't change.

> or
> class MatrixFunction{
>   static const int Rows = Traits::RowsAtCompileTime;

That one is a mistake: it should be an enum. Let's fix that. Making it
a "static const int" is just giving the compiler the additional work
of having to understand that it can optimize it away at compile time.

> These are unproblematic, since these variables are initialized at program
> start and never change. One only has to consider the "static initialization
> order fiasco" - but that is hardly a risk here - and the runtime overhead at
> startup, which should be minimal for these examples. There are a couple of
> globals in src/Core/arch/AltiVec/PacketMath.h which haven't been marked as
> const even though they should be, but that's an easy fix.
> The finer subtleties occur in examples where static variables are declared
> in a function. These variables will be initialized lazily. i.e. the first
> time the function is called.
> The use of static variables in blueNorm() in src/core/stablenorm.h is just
> plain wrong. The good news is that this is the only example I found of
> blatant non-reentrancy.

(pinging Gael as I believe he copied that code from elsewhere)

> MatrixBase<Derived>::blueNorm() const
> {
>   static int nmax = -1;
>   static RealScalar b1, b2, s1m, s2m, overfl, rbig, relerr;
>   if(nmax <= 0)
>   {
>     // calculate b1, b2, etc
> ...
>     nmax = nbig;
>   }

OK indeed this is not reentrant! Thanks for spotting it. For Gael.

> An even subtler example would be:
> QuaternionBase<Derived>::slerp(Scalar t, const QuaternionBase<OtherDerived>&
> other) const
> {
>   static const Scalar one = Scalar(1) - NumTraits<Scalar>::epsilon();
> ...
> This example is almost certainly safe, at least as long Scalar is a POD. The
> problem is that either the expression is so simple the compiler optimizes it
> away - in which case there is no reason declaring it static - or it isn't..

This one for Gael too :)

Benoit

> Then it might actually be inefficient, because  there is a runtime check,
> which involves locking(GCC and MSVC treat these cases differently), to see
> if the variable has been initialized or not.
> The fact is that *there is no way to use lazily initialized non-POD static
> variables that does not include locks or some atomic API*.
> For reference,
> see http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf and http://stackoverflow.com/questions/6915/thread-safe-lazy-contruction-of-a-singleton-in-c
> So, to return to blueNorm(), I see two ways of fixing this
> A: Mark the function as non-reentrant. This would be Eigen's only such
> function then, or
> B: Put b1, b2 etc in a class, a global instance of which will be initialized
> at startup. I think this initialization might be optimized away in programs
> that never call the function(that needs to be checked).
> Don't be tempted to stick to lazily initialization(through double-checked
> locking or something equally clever), since it just isn't thread-safe.
> To end this mail on a positive note: reentrancy is actually one of the
> reasons I decided to switch to Eigen in the first place, after having used
> numerous other libraries lacking in this respect.
> best
> Björn