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

[planning_interface] Release Python GIL for C++ calls #1947

Merged
merged 1 commit into from
Mar 11, 2020

Conversation

gleichdick
Copy link
Contributor

@gleichdick gleichdick commented Mar 10, 2020

Description

Fixes #1580. (C)Python has a Global Interpreter Lock (GIL) which should be released when doing long-lasting C++ calls.

Checklist

  • Required by CI: Code is auto formatted using clang-format
  • Extend the tutorials / documentation reference
  • Document API changes relevant to the user in the MIGRATION.md notes
  • Create tests, which fail without this PR reference
  • Include a screenshot if changing a GUI
  • While waiting for someone to review your request, please help review another open pull request to support the maintainers

@codecov-io
Copy link

codecov-io commented Mar 10, 2020

Codecov Report

Merging #1947 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1947      +/-   ##
==========================================
+ Coverage   49.82%   49.82%   +<.01%     
==========================================
  Files         313      314       +1     
  Lines       24714    24717       +3     
==========================================
+ Hits        12313    12316       +3     
  Misses      12401    12401
Impacted Files Coverage Δ
...ove_group_interface/src/wrap_python_move_group.cpp 43.03% <ø> (ø) ⬆️
...ls/include/moveit/py_bindings_tools/gil_releaser.h 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5327e41...be0de02. Read the comment docs.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution. I'm not sure, though we should release and reacquire the GIL for each and every C++ function call. Saving and restoring the thread state comes with its own costs and should be balanced versus the cost of the function call.

@gleichdick gleichdick force-pushed the master branch 2 times, most recently from 53ea5b9 to 5d895b6 Compare March 11, 2020 09:23
@gleichdick
Copy link
Contributor Author

Good point, I reduced the usage of the GILReleaser to the following functions:

  • MoveGroupInterface::execute()
  • MoveGroupInterface::move()
  • MoveGroupInterface::plan()
  • MoveGroupInterface::computeCartesianPath()
  • MoveGroupInterfaceWrapper::retimeTrajectory()
  • RobotInterfacePython::ensureCurrentState()

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

This also makes it straightforward to add more places if it should become relevant.

Annoying nitpick on the license header.

/*********************************************************************
* Software License Agreement (BSD License)
*
* Copyright (c) 2020, Willow Garage, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add your name/ organization.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

* copyright notice, this list of conditions and the following
* disclaimer in the documentation and/or other materials provided
* with the distribution.
* * Neither the name of Willow Garage nor the names of its
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from Michael's nitpick this looks good to me.

@rhaschke rhaschke merged commit caf5ef1 into moveit:master Mar 11, 2020
v4hn pushed a commit to v4hn/moveit that referenced this pull request Mar 30, 2020
v4hn pushed a commit to v4hn/moveit that referenced this pull request Mar 31, 2020
v4hn pushed a commit to v4hn/moveit that referenced this pull request Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Python GIL not released before entering the C++ library
4 participants