search

fzi-forschungszentrum-informatik / gpu-voxels

GPU-Voxels is a CUDA based library which allows high resolution volumetric collision detection between animated 3D models and live pointclouds from 3D sensors of all kinds.
Other
302 stars 85 forks source link

No need for synchronization? #96

Open acf986 opened 5 years ago

acf986 commented 5 years ago

For the updateOccupancy function in the file ProbabilisticVoxel.hpp (line 44). I found it is possible that this function will be called from different threads to update the value of m_occupancy.

Will it be dangerous if the write operation in this function is not synchronized?

cjue commented 5 years ago

Hi Nyaruko, yes, there is no lock preventing multiple threads to call updateOccupancy on the same voxel simultaneously. In the case of BitVectorVoxel or DistanceVoxel this does not lead to problems e.g. during Point-Cloud insertion, as all threads are writing the same data.

But you are correct in raising this issue: in the case of ProbabilisticVoxel and CountingVoxel this creates a race-condition where multiple threads try to update the contents of the same voxel during insertPointCloud. This race-condition can lead to the loss of some the inserted information.

Example: https://github.com/fzi-forschungszentrum-informatik/gpu-voxels/blob/master/packages/gpu_voxels/src/gpu_voxels/voxelmap/kernels/VoxelMapOperations.hpp#L243

I will look into ways to minimize or avoid the risk of information loss, without impacting overall performance significantly.

Side-Note: There are some map-level functions that are already thread-safe, like TemplateVoxelMap::insertPointCloud and clearMap.

On the voxel-level It's usually in the responsibility of your own code to avoid this kind of race condition from accesses to the same voxel, while the library functions are mostly designed to avoid multiple write accesses to the same voxel by multiiple threads in the same launch.

acf986 commented 5 years ago

Thanks for your notice. I ran into this problem when trying to use the raycaster in the ProbablisticVoxel as shown here. They raycaster would easily access the same voxel from multiple threads when the rays are dense (like in the case from a Lidar).

Squelsh commented 5 years ago

Valid point here. I started this with the probabilistic voxels in mind, when I used them for environmental mapping over a longer period of time... I knew about the race condition but I did not interpret it as "broken" for two reasons: 1) The Raycaster traverses the rays from obstacle to the sensor. So if you have obstacles at various distances, the probability for synchronous hits in the passed voxels are small. 2) I interpreted the problem as probabilistically "correct". If you see the same situation over a bunch of frames, with slight variances only, the result is almost correct, even if you missed some voxels due to race conditions.

i tried it with a simple thread-lock, but performance was not usable for my scenario then (had several cams in parallel).

So either we have to put a clear warning in the docs or we could offer a second "correct" function. Or @cjue wraps his head around it an finds an elegant and well performing solution :grimacing:

acf986 commented 5 years ago

@cjue @Squelsh , just one final question, if all the threads write to the same location with the same value, is this safe?

cjue commented 5 years ago

@Nyaruko: yes, to my understanding of the CUDA architecture this will always have the expected result, the location will contain the expected value.

A side-node on my earlier comment regarding point-cloud insertion: For VoxelLists this is usually not an issue. For example, CountingVoxelList::insertPointCloud will first create a voxel for each point and then merge all voxels with identical coordinates in a safe way during the VoxelList "make_unique" step.