Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No need for synchronization? #96

Open
acf986 opened this issue Mar 9, 2019 · 5 comments
Open

No need for synchronization? #96

acf986 opened this issue Mar 9, 2019 · 5 comments
Labels

Comments

@acf986
Copy link

acf986 commented Mar 9, 2019

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
Copy link
Contributor

cjue commented Mar 10, 2019

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.

@cjue cjue added the bug label Mar 10, 2019
@acf986
Copy link
Author

acf986 commented Mar 10, 2019

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
Copy link
Contributor

Squelsh commented Mar 10, 2019

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 😬

@acf986
Copy link
Author

acf986 commented Mar 10, 2019

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

@cjue
Copy link
Contributor

cjue commented Mar 10, 2019

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants