Eigen alloc #24
Eigen alloc #24
Conversation
There was a problem hiding this comment.
This looks great! It actually solved a problem I had today on the robot with the GazeTask. I'll just wait for @gergondet to submit feedback 😈
| @@ -1 +1,41 @@ | |||
| # Clang code formatting file | |||
There was a problem hiding this comment.
Is it really necessary to add a .gitignore? I'm not sure we really want this in the repo, especially since some of the files are OSX specific...
There was a problem hiding this comment.
This helps me on both linux and mac... it is almost entirely binaries which shouldn't be committed anyway.
| @@ -69,6 +69,8 @@ class TASKS_DLLAPI LSSOLQPSolver : public GenQPSolver | |||
| Eigen::VectorXd C_; | |||
|
|
|||
| int nrALines_; | |||
| public: | |||
| EIGEN_MAKE_ALIGNED_OPERATOR_NEW | |||
There was a problem hiding this comment.
Do you know if it's possible to simply declare EIGEN_MAKE_ALIGNED_OPERATOR_NEW in the parent class? It would prevent a lot of repetition...
There was a problem hiding this comment.
just being conservative on this... I assumed no unfortunately but haven't verified.
binding/python/generate.py
Outdated
| 'std::vector<Eigen::VectorXd>', 'vector') | ||
| tasks.add_container('std::vector<Eigen::Vector3d, Eigen::aligned_allocator<Eigen::Vector3d> >', 'Eigen::Vector3d', 'vector') | ||
| tasks.add_container('std::vector<Eigen::Matrix3d, Eigen::aligned_allocator<Eigen::Matrix3d> >', 'Eigen::Matrix3d', 'vector') | ||
| tasks.add_container('std::vector<Eigen::VectorXd, Eigen::aligned_allocator<Eigen::VectorXd> >', 'Eigen::VectorXd', 'vector') |
There was a problem hiding this comment.
This is wrong, you should not use aligned allocation for dynamic size types... Even for some fixed-size objects, such care is not necessary. See here:
http://eigen.tuxfamily.org/dox-devel/group__TopicFixedSizeVectorizable.html
Long story short: things that have an even number of doubles in them (Vector2d, Vector4d, Matrix2d...).
|
actually... it does compile for eigen 3.3. I'll change it back anyway for the dynamic objects. |
|
Hi, As Hervé already pointed out, this is indeed an important issue that can be particularly Thanks for all the work you put into this PR but I think most of it is unnecessary. An important thing to note is that I will leave a comment here but this applies to the similar PRs on SVA and RBDyn. I will start by citing this part of Eigen documentation:
Then this page defines what are fixed-size vectorizable Eigen types. According to this page and to the code you changed it would seem most of the Similarly, it seems superfluous on most cases for Tasks but there is indeed some places where it should be required (e.g. Finally, I'd prefer if we could find a better solution than what is suggested by Eigen because it does not work well in a C++11 context, e.g.: std::shared_ptr<tasks::qp::GazeTask> qt = std::make_shared<tasks::qp::GazeTask>(mbs, 0, bodyName, pt, 1.0, sva::PTransformd::Identity());can (and does) trigger alignment issues even with |
|
I'm closing this since #29 superseeds it. |
#23