eigen allocation bug in tasks, rbdyn, sva, etc

[ahundt]

It seems there is a small flaw in many of the classes as per the eigen documentation on memory alignment:

They should all be declaring the following:

class Foo
{
  ...
  Eigen::Vector2d v;
  ...
public:
  EIGEN_MAKE_ALIGNED_OPERATOR_NEW
};
...
Foo *foo = new Foo;

This problem only crops up sporadically in practice but when it does nothing runs! I might submit a pull request fixing this in quite a few places.

The same applies to all STL containers, their allocator must be set:

Using an aligned allocator

STL containers take an optional template parameter, the allocator type. When using STL containers on fixed-size vectorizable Eigen types, you need tell the container to use an allocator that will always allocate memory at 16-byte-aligned locations. Fortunately, Eigen does provide such an allocator: Eigen::aligned_allocator.

For example, instead of

std::map<int, Eigen::Vector4f>
you need to use

std::map<int, Eigen::Vector4f, std::less<int>, 
         Eigen::aligned_allocator<std::pair<const int, Eigen::Vector4f> > >
Note that the third parameter "std::less<int>" is just the default value, but we have to include it because we want to specify the fourth parameter, which is the allocator type.

The case of std::vector

The situation with std::vector was even worse (explanation below) so we had to specialize it for the Eigen::aligned_allocator type. In practice you must use the Eigen::aligned_allocator (not another aligned allocator), and #include <Eigen/StdVector>.

Here is an example:

#include<Eigen/StdVector>
/* ... */
std::vector<Eigen::Vector4f,Eigen::aligned_allocator<Eigen::Vector4f> >

[ahundt]

Another question, do you think this will have any effect on when data is fed to any of the fortran solver backends?

[ahundt]

All the pull requests referenced above fix the crash in my software. I still need to add the correct allocator to std::vector and std::map instances. A side effect of these pull requests might also include improved performance. :-)

[haudren]

Thanks for the PR! Just to confirm, you are working on MacOS ? It might explain the differences in behaviour...

[ahundt]

Yes I ran into this on Mac OS, I run on both mac and linux. I believe this can crop up on any OS. I ran into the problem when I updated to eigen 3.3, although this requirement has been present for quite a long time as I've encountered it in other unrelated code as well on both linux and mac.

[ahundt]

Still TODO is adding the allocator to the STL containers...

[ahundt]

Do you think std:: containers of non-eigen types should also use the eigen alignment allocator? Might provide a performance boost plus help ensure compatibility but on the other hand it isn't exactly pretty to have the extra template parameter.

This issue might also apply to types like std::map<std::string, sva::PTransformd> as well, probably should specify the allocator there as well?

probably something like this:

std::map<std::string, sva::PTransformd, std::less<std::string>, 
         Eigen::aligned_allocator<std::pair<const std::string, sva::PTransformd> > >
``
Note that the third parameter "std::less<int>" is just the default value, but we have to include it because we want to specify the fourth parameter, which is the allocator type.

[haudren]

No, I'm not a fan of adding unnecessary template parameters and I'm not sure it would really help performance.

[ahundt]

The thing is it may cause the same kind of crash... I'm not completely sure of the detail of alignment of a struct/class containing aligned data. Now that I think on it, I'm guessing that most likely needs aligned allocation too to be guaranteed to work properly.

[ahundt]

@gergondet will these fixes be possible to merge? I've been running them for a few days now and they've fixed the relevant crashes for me.

[ahundt]

Continuing Discussion from @gergondet here.
#24 (comment)

This post covers a number of possibilities for the std::make_shared issue:
RobotLocomotion/drake#1854 (comment)

Either way, there is a known bug that these pull requests resolve, so one way or another I don't think the code should be left as-is on master. Are you proposing I go through and remove the allocator lines everywhere there isn't a fixed size eigen type?

Also, perhaps the way I specified the aligned allocators for every class is a bit conservative, but a much simpler rule like that makes it easier to avoid the case where somebody isn't aware of, or forgets, the rule and then adds a fixed size element to a class.

[gergondet]

Are you proposing I go through and remove the allocator lines everywhere there isn't a fixed size eigen type?

Yes. But again, it should be vectorizable fixed size Eigen types, not just any fixed size Eigen types. In particular, the issue does not affect SpaceVecAlg and RBDyn at all since all fixed size objects used there are non vectorizable (i.e. Matrix<T, 3, 1> and Matrix<T, 3, 3>).

Also, perhaps the way I specified the aligned allocators for every class is a bit conservative, but a much simpler rule like that makes it easier to avoid the case where somebody isn't aware of, or forgets, the rule and then adds a fixed size element to a class.

I think we don't need to worry about people who write code within the library. First of all, the code will be reviewed before its inclusion by people who should be aware of the issue. We can also probably come up with a test that has a good chance to trigger the alignment issue.

On the other hand, what I am more worried about is people who write new libraries based on our work as EIGEN_MAKE_ALIGNED_OPERATOR_NEW has a strong implication of "if you don't create this object properly, you will trigger alignment issues in some cases" that are, according to drake discussion, not currently enforceable. So whenever you have members that have this alignment issues you contaminate your own class. In that sense, I actually kind-of-like the idea of enforcing the creation of these objects through a factory function, but if we want to make it reasonable it needs to be limited to the few problematic cases.

[haudren]

@gergondet : If we want to avoid class contamination, the "best" way is probably to always encapsulate those containers as (const) member pointers i.e. in QPTasks, use a tasks::GazeTask* instead of the current tasks::GazeTask.

Overall, I agree, as this macro needs to be propagated to every class that has a member that contains the macro, it would be best to keep it as localized as possible.

[gergondet]

I've pushed the topic/FixAlignmentIssue branch that does that on a minimal number of tasks.

I've followed @haudren suggestion regarding the const pointer member as this neatly allows to stop the propagation of EIGEN_MAKE_ALIGNED_OPERATOR_NEW everywhere. A neat side-effect is that it also allow to create the objects (tasks::qp::GazeTask for example) through std::make_shared without issue. Actually, we might even consider to make the problematic Eigen members as const pointer. (there is sufficiently few instances for it to make sense imho)

I've identified the following tasks that should pose a problem:

• GazeTask (Tasks.h and QPTasks.h)
• PositionBasedVisServoTask (Tasks.h and QPTasks.h)
• ImageConstr (QPConstr.h)
• MultiRobotTransformTask (QPTasks.h)

I've taken care of the first two in my branch as a proof of concept. I have a simple test that fails (on 32 bits systems) when the allocation is not good but I don't think it's useful to add it to the repository atm.

[gergondet]

I've pushed a more complete fix to the topic/FixAlignmentIssue branch. I've also pushed a test that should trigger alignment issues (I can confirm it segfaults on a 32 bits platform before the alignment patches)

It should fix the alignment issue we are facing.

@ahundt can you test it on your current setup and check whether you face alignment issues or not?

[ahundt]

Sure I will might need a few days before I have the time to confirm.

[ahundt]

@gergondet looks good! Seems like it will make sense to merge now

± make test
Running tests...
Test project /Users/athundt/source/git/jrl-umi3218/Tasks/build
    Start 1: QPSolverTestUnit
1/4 Test #1: QPSolverTestUnit .................   Passed   56.48 sec
    Start 2: QPMultiRobotTestUnit
2/4 Test #2: QPMultiRobotTestUnit .............   Passed    8.65 sec
    Start 3: TasksTestUnit
3/4 Test #3: TasksTestUnit ....................   Passed    1.40 sec
    Start 4: AllocationTestUnit
4/4 Test #4: AllocationTestUnit ...............   Passed    0.02 sec

100% tests passed, 0 tests failed out of 4

Total Test time (real) =  66.56 sec

sorry had lost track of this so my time estimate was way off :-)

[haudren]

Seems good, @gergondet I think you can merge! This will help with #28