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

Make FCL shape cache thread-local #1316

Merged
merged 4 commits into from Jan 30, 2019

Conversation

Projects
None yet
3 participants
@simonschmeisser
Copy link
Contributor

simonschmeisser commented Jan 23, 2019

Currently there are two shared caches of FCLGeometry objects, one for world objects
and one for attached objects. If a Shape is not found in the matching one, it is either
moved from the other one or created (and stored) if it does not exist. For an existing
entry the shared_ptr is checked to be unique, if it isn't a new Geometry is created from
scratch as well.

This all works quite well in a single threaded world but if there are many threads cache
entries will basically always be in use by some other thread.

This PR replaces the global caches with thread-local ones. All locking can then be
dropped as well. A new cache will be created per thread automatically and is freed once
the threads finishes.

Rationale

We use MoveIt! to do random bin picking, that is we need to check many possible grasps and movements in a very short time. Most of them involve (instances of) the same shape. That means we first check some motion primitives in a thread pool and fall back to OMPL if necessary. I did some profiling of a typical workflow using hotspot/perf and about 30% of the time were spent in createCollisionGeometry. Out of this almost everything was spent in fcl::BVHModel::endModel ie optimizing the mesh for fast collision checking. (One other reason could have been lock congestion). Applying this patch, createCollisionGeometry no longer shows up in the profiler and its mostly down to coordinate transforms in Eigen (this is fcl 0.5, maybe that is improved with 0.6 using Eigen::Isometry3d ?).

So how does this patch affect more typical MoveIt! workflows? Unfortunately OMPL does not seem to use a thread pool so the cache will be recreated for each planning request. However as OMPLs planners typically are multi-threaded they will still see an improvement.

@v4hn @Levi-Armstrong this fixes the issues discussed in #488

Make FCL shape cache thread-local
Currently there are two shared caches of FCLGeometry objects, one for world objects
and one for attached objects. If a Shape is not found in the matching one, it is either
moved from the other one or created (and stored) if it does not exist. For an existing
entry the shared_ptr is checked to be unique, if it isn't a new Geometry is created from
scratch as well.

This all works quite well in a single threaded world but if there are many threads cache
entries will basically always be in use by some other thread.

This commit replaces the global caches with thread-local ones. All locking can then be
dropped as well. A new cache will be created per thread automatically and is freed once
the threads finishes.
@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Jan 23, 2019

This is awesome!

Question: does this work duplicate the optimization that @Levi-Armstrong made in Tesseract, and is there any value in bringing all those optimizations into MoveIt! also? I totally understand if you don't have time for that, though.

simonschmeisser added some commits Jan 25, 2019

@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Jan 25, 2019

Looks good. Could you please create a small unit test that verifies the multi-thread behavior doesn't regress in the future?

}

fcl::CollisionGeometryd* cg_g = nullptr;
if (shape->type == shapes::PLANE) // shapes that directly produce CollisionGeometry

This comment has been minimized.

@rhaschke

rhaschke Jan 30, 2019

Contributor

Great cleanup!

@rhaschke
Copy link
Contributor

rhaschke left a comment

Didn't know about the static thread_local keyword before. Looks pretty useful in other scenarios as well 😄
Thanks for this cleanup and performance improvement!

@rhaschke

This comment has been minimized.

Copy link
Contributor

rhaschke commented Jan 30, 2019

Could you please create a small unit test that verifies the multi-thread behavior doesn't regress in the future?

@davetcoleman Of course, tests would be a nice-to-have. But we didn't have tests for the locking mechanism before as well. So, this PR is a clear performance + code improvement and should be accepted as is, if @simonschmeisser doesn't have cycles to work on a test.

@simonschmeisser

This comment has been minimized.

Copy link
Contributor Author

simonschmeisser commented Jan 30, 2019

Didn't know about the static thread_local keyword before. Looks pretty useful in other scenarios as well

if you were pedantic it is actually just thread_local, the static is implied and redundant. But as most people don't know thread_local so I thought its easier to understand this way.

Looks good. Could you please create a small unit test that verifies the multi-thread behavior doesn't regress in the future?

Honestly I have no idea of a meaningful test to start with ...

@rhaschke

This comment has been minimized.

Copy link
Contributor

rhaschke commented Jan 30, 2019

Honestly I have no idea of a meaningful test to start with ...

Fully understand you. Testing that race conditions like mutual access to a shared resource don't occur, can only be done by spawning thousands of threads. But this doesn't prove that there is no race condition. I'm going to merge this now as @davetcoleman generally approved this as well.

@rhaschke rhaschke merged commit 549fe7c into ros-planning:melodic-devel Jan 30, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Jan 31, 2019

In general, I think we reviewers need to get better at requesting adding test coverage when low level changes like this get proposed. if we don't start adding tests now, when?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.