search

kzampog / cilantro

A lean C++ library for working with point cloud data
MIT License
1.03k stars 208 forks source link

Race conditions in grid accumulator? #31

Closed wannesvanloock closed 5 years ago

wannesvanloock commented 5 years ago

I'm having issues with cilantro's grid accumulator. Occasionally, I'm getting strange results such as a normal = (0, 0, 0). I've narrowed down the issues to the parallelism the grid_accumulator.hpp. It seems to me that there are race conditions specifically in L150-154. I don't understand all the bits of the parallelization here but I believe the accumulator shouldn't be shared across threads as this can cause undefined behavior and a reduction should be used instead. I will try to create a minimal example if necessary for debugging. For now, setting ENABLE_NON_DETERMINISTIC_PARALLELISM=OFF fixes the issue.

kzampog commented 5 years ago

Thank you so much for reporting this! There were also performance issues in the parallel version of GridAccumulator on Windows (#29), so there's definitely some digging to be done there. A minimal example with data would be extremely helpful!

kzampog commented 5 years ago

Hi again,

Please have a look at ea57e4790cf3e97b6b689bf6989b0d93ccd0dd34. Normal consistency was not properly enforced in the parallel version (when merging accumulators), so this might be the reason you were getting zero-valued normals. I think this should fix it, and performance is very close to what it was without the additional checks. In any case, I will refactor this to something cleaner in the future.

Please let me know of your findings!

wannesvanloock commented 5 years ago

Awesome, that fixed it indeed. Thanks for the lightning fast fix :slightly_smiling_face:

To be honest, this part of the code is difficult to understand for me, so I didn't feel confident to tackle the issue myself. Looking forward to a refactor!

kzampog commented 5 years ago

Awesome, glad that fixed it! :D

Yes, that part is a bit messy. I'll try to improve it and make it cleaner!