ktprime / emhash

Fast and memory efficient c++ flat hash map/set
MIT License
435 stars 30 forks source link

Might be an issue of the code. #24

Closed hitdshu closed 1 year ago

hitdshu commented 1 year ago

Hi,

Very excellent projects. I used your map in my numerical visual-inertial odometry project, and it works much better than std::unordered_map.

However, I encountered one problem. I use Eigen library to perform matrix calculations. And when I enable -march=native in GCC, the emhash7::HashMap<int, Eigen::Matrix4d> will emit segmentation fault.

I found that Eigen will assume an aligned memory if -march=native is enabled. And for C++17 and after, Eigen will take care of the alignment, and we need to do nothing. https://eigen.tuxfamily.org/dox/group__TopicStlContainers.html

Use std::unordered_map<int, Eigen::Matrix4d> or ankerl::unordered_dense::unordered_map<int, Eigen::Matrix4d> works for me. I think it might because your code does not use an allocator?

Anyway, it's a very tedious problem. It might not be your problem, but I decide to report it to you.

Best, Deshun

hitdshu commented 1 year ago

You don't need to fix it, I just want to let you know about this problem.

This should a very rare use case of your code, and I took a lot of time to find this bug.

Besides, use emhash7 or anker::unordered_dense have the similar performance in my final code, and both are significantly better than std::unordered_map

ktprime commented 1 year ago

Can you reproduce it or offer any coredump stack? only C "malloc/free" memory function used in emhash without using std::allocator. Maybe I should add std::allocator to emash to fix the alignment issiue or implement aligned_malloc

ktprime commented 1 year ago

emhash8(hash_table8.hpp) maybe be a better choice without alignment issiue.

hitdshu commented 1 year ago

Okay, I will provide an example this weekend, as I have to do other things in work days.

ktprime commented 1 year ago
some way to fix your issiue .
1. change your code emhash7::HashMap<int, Eigen::Matrix4d> to  emhash7::HashMap<int64_t, Eigen::Matrix4d> 

2.  change the source code in emhash7
template <typename First, typename Second>
 struct entry alignas(16) {
hitdshu commented 1 year ago

Well, sorry for the late reply.

Turns out, there are many interesting to do in holidays...

Thanks for your advice, and I'll provide you an example befor 1/30/2023.

Deshun

hitdshu commented 1 year ago

Well, I wrote a demo program, please check it. map_eigen_test.zip

ktprime commented 1 year ago

I can reproduce it now, the problem is align issue by malloc(default 16 byte align). but in your test code, Eigen::Matrix4d need 32 bytes align.

https://stackoverflow.com/questions/46301140/32-byte-aligned-allocator-for-aligned-eigen-map

To fix this issiue emhash need some redesign. all malloc can be replaced by aligned_alloc

c++11: _pairs = (PairT*)aligned_alloc(32, AllocSize(_num_buckets));

ktprime commented 1 year ago

this is a fix version hash_table7.zip

hitdshu commented 1 year ago

Thanks for your timely reply. It works. Thanks.