std container types of Eigen matrix types missing eigen allocators

[catskul]

Per https://eigen.tuxfamily.org/dox-devel/group__TopicStlContainers.html

Using STL containers on fixed-size vectorizable Eigen types, or classes having members of such types, requires special handling.

Either std container types should use the Eigen::aligned_allocator (preferred)

or a container specialization that has the same effect.

A first attempt to grep for instances that are missing aligned allocators yields a number of apparent cases where std::vector requires an aligned allocator.

$ grep -r "std::\w*<Eigen" 
cartographer/mapping_2d/ray_casting.cc:  std::vector<Eigen::Array2i> ends;
cartographer/mapping_2d/scan_matching/correlative_scan_matcher.h:typedef std::vector<Eigen::Array2i> DiscreteScan;
cartographer/mapping_3d/hybrid_grid.h:    const std::pair<Eigen::Array3i, ValueType> operator*() const {
cartographer/mapping_3d/hybrid_grid.h:      return std::pair<Eigen::Array3i, ValueType>(GetCellIndex(), GetValue());
cartographer/mapping_3d/submaps.h:  std::vector<Eigen::Array4i> voxel_indices_and_probabilities_;
cartographer/mapping_3d/scan_matching/fast_correlative_scan_matcher.h:  std::vector<std::vector<Eigen::Array3i>> cell_indices_per_depth;
cartographer/mapping_3d/scan_matching/fast_correlative_scan_matcher.cc:  std::vector<std::vector<Eigen::Array3i>> cell_indices_per_depth;
cartographer/mapping_3d/scan_matching/fast_correlative_scan_matcher.cc:  std::vector<Eigen::Array3i> full_resolution_cell_indices;
cartographer/mapping_3d/kalman_local_trajectory_builder_test.cc:  std::vector<Eigen::Vector3f> bubbles_;
cartographer/kalman_filter/unscented_kalman_filter.h:      std::function<Eigen::Matrix<FloatType, K, 1>(const StateType&)> h,
cartographer/kalman_filter/unscented_kalman_filter.h:    std::vector<Eigen::Matrix<FloatType, K, 1>> Z;
cartographer/sensor/point_cloud.h:typedef std::vector<Eigen::Vector3f> PointCloud;
cartographer/sensor/point_cloud.h:typedef std::vector<Eigen::Vector2f> PointCloud2D;
cartographer/sensor/laser_test.cc:  std::vector<std::pair<Eigen::Vector3f, int>> pairs;
cartographer/sensor/laser_test.cc:              Contains(PairApproximatelyEquals(std::pair<Eigen::Vector3f, int>(
cartographer/sensor/laser_test.cc:              Contains(PairApproximatelyEquals(std::pair<Eigen::Vector3f, int>(
cartographer/sensor/laser_test.cc:              Contains(PairApproximatelyEquals(std::pair<Eigen::Vector3f, int>(

I'll try to produce a pull request fixing this issue unless someone can indicate not an issue in this case for some reason unaware of.

[wohe]

As you might have expected, it was a conscious decision to not add these to the code. Here are our current thoughts:

1. Our focus was on x86-64 where heap and stack allocations are 16-byte aligned which is all that Eigen needs and we have not seen any crashes.
2. Compiling for 32-bit would not provide the best performance anyway, and limits the amount of memory usable by the SLAM stack. If we have a use case for 32-bit x86 and this is no concern, a simpler alternative would be to just detect the platform and define EIGEN_DONT_VECTORIZE and EIGEN_DISABLE_UNALIGNED_ARRAY_ASSERT.
3. Future versions of Eigen or needs of our users might make us change this decision. Future versions of C++ might make the whole alignment problem be solved at the Eigen library/compiler level.
4. If we were to add EIGEN_MAKE_ALIGNED_OPERATOR_NEW this would only solve the problem for the Cartographer library itself. Users of the library would also have to add EIGEN_MAKE_ALIGNED_OPERATOR_NEW to classes containing vectorized Cartographer classes. This sounds like a maintenance nightmare.

So my question would be where you have seen crashes: Do you use a 32-bit platform? What operating system? Which compiler?

[catskul]

I've not yet seen any Cartographer crashes, but had seen alignment related crashes in the past when using Eigen but not using aligned allocators.

The finer points on when it may and may not matter are not something I've dived deep on yet so if you're confident it's not an issue, I certainly defer.

[jlblancoc]

The alignment macro is required whenever a **fixed-size** Eigen container is used as a member of classes that will be allocated dynamically in the heap. For local objects in the stack, it may be required to add a different compiler attribute that ensures the proper 16-byte alignment, Eigen has its own macro for this as well. As said above 64bit systems normally do allocate dynamic memory with the alignment required by SSE Eigen optimizations, so this is only a real issue if the maintainers really want to support 32bit builds... Cheers, PS: thank you Google for sharing this project!!

[catskul]

@jlblancoc I opened a separate issue and pull request specifically regarding EIGEN_MAKE_ALIGNED_NEW (#36, #39). There are a dozen or so classes/structs which have data members which are fixed-size eigen types.

I'm unaware of the fine detail on when it's ok not to heed the warnings of the Eigen team, so I defer to those who are confident that they do.

[damonkohler]

As discussed and documented in the system requirements, we currently target only 64-bit systems where this is not necessary. I'll close this now.