cartographer-project / cartographer

Cartographer is a system that provides real-time simultaneous localization and mapping (SLAM) in 2D and 3D across multiple platforms and sensor configurations.
Apache License 2.0
7.09k stars 2.25k forks source link

Mutex warning when using optimization_problem_->Solve #1126

Open gaschler opened 6 years ago

gaschler commented 6 years ago

PoseGraph doesn't hold a mutex when calling OptimizationProblem::Solve, there is a comment https://github.com/googlecartographer/cartographer/blob/master/cartographer/mapping/internal/3d/pose_graph_3d.cc#L600.

Maybe it should at least hold a smaller mutex that guards the input to the optimization problem.

cartographer/cartographer/mapping/internal/3d/pose_graph_3d.cc:604:32:
warning: passing variable 'constraints_' by reference requires holding mutex 'mutex_' [-Wthread-safety-reference]
  optimization_problem_->Solve(constraints_, frozen_trajectories_,
                               ^
cartographer/cartographer/mapping/internal/3d/pose_graph_3d.cc:604:46:
warning: passing variable 'frozen_trajectories_' by reference requires holding mutex 'mutex_' [-Wthread-safety-reference]
  optimization_problem_->Solve(constraints_, frozen_trajectories_,
                                             ^
cartographer/cartographer/mapping/internal/3d/pose_graph_3d.cc:605:32:
warning: passing variable 'landmark_nodes_' by reference requires holding mutex 'mutex_' [-Wthread-safety-reference]
                               landmark_nodes_);
ojura commented 6 years ago

Looks like the mutex annotation for these three should be removed from the header?

https://github.com/googlecartographer/cartographer/blob/c46fe073b48dd1e33d38c285163fbda8fca4fefa/cartographer/mapping/internal/3d/pose_graph_3d.h#L264

gaschler commented 6 years ago

Removing the annotation would only hide the issue. If we want one thread (main) to dispatch work items while a background thread is supposed to optimize, then the proper solution is to have more fine-grained mutexes and not just one for PoseGraph, I think.