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

Cache link poses to improve performance #669

Merged
merged 1 commit into from
Mar 8, 2021

Conversation

adlarkin
Copy link
Contributor

@adlarkin adlarkin commented Mar 4, 2021

Signed-off-by: Ashton Larkin [email protected]

🦟 Bug fix

Related to gazebosim/gz-physics#219.

Summary

This PR is motivated by #656 and gazebosim/gz-physics#219.

gazebosim/gz-physics#219 points out that the UpdateSim method in the physics system goes over all links since the physics system doesn't know which links changed. For links that belong to non-static models, we are always performing pose updates, even if the pose hasn't changed. I'm proposing that we cache each non-static link's pose from the previous iteration and only perform a pose update if the link's pose is different from the previous iteration.

Testing this change by profiling a world as done in #656 (3000 non-static simple shapes, unbounded RTF, headless, and TPE for the physics engine) seems to result in a noticeable performance improvement.

Before link caching, UpdateSim takes around 8.25ms. Local pose, which contains the pose updates, takes around 5.2ms, which is more than half of the UpdateSim time:

before_link_caching

After link caching, UpdateSim takes around 5.7ms, and Local pose now takes around 2.4ms:

after_link_caching

Taking a look at the some of the other code in UpdateSim, I am wondering if we can do something similar for non-static link velocities and accelerations. @chapulina what do you think about this?

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

@github-actions github-actions bot added the 🔮 dome Ignition Dome label Mar 4, 2021
@adlarkin adlarkin force-pushed the adlarkin/cache_link_poses branch from 1857cf2 to 4614265 Compare March 4, 2021 19:55
Copy link
Contributor Author

@adlarkin adlarkin left a comment

Choose a reason for hiding this comment

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

A few questions for the PR reviewers (cc @azeey).

@adlarkin adlarkin requested review from chapulina, azeey and iche033 March 4, 2021 20:09
@adlarkin adlarkin marked this pull request as ready for review March 4, 2021 20:09
@codecov
Copy link

codecov bot commented Mar 4, 2021

Codecov Report

Merging #669 (71264a8) into ign-gazebo4 (1b9ad9a) will increase coverage by 0.04%.
The diff coverage is 86.36%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo4     #669      +/-   ##
===============================================
+ Coverage        64.96%   65.01%   +0.04%     
===============================================
  Files              232      232              
  Lines            16640    16645       +5     
===============================================
+ Hits             10811    10822      +11     
+ Misses            5829     5823       -6     
Impacted Files Coverage Δ
src/systems/physics/Physics.cc 70.07% <86.36%> (+0.17%) ⬆️
src/SimulationRunner.cc 93.95% <0.00%> (+1.06%) ⬆️

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 1b9ad9a...58cc8d3. Read the comment docs.

@chapulina chapulina added the performance Runtime performance label Mar 4, 2021
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

nice, changes look good to me. Just one question about the non-static link pose comment

@adlarkin adlarkin force-pushed the adlarkin/cache_link_poses branch from e85f4d9 to 58cc8d3 Compare March 8, 2021 15:55
@adlarkin adlarkin merged commit 58cc8d3 into ign-gazebo4 Mar 8, 2021
@adlarkin adlarkin deleted the adlarkin/cache_link_poses branch March 8, 2021 17:19
adlarkin added a commit that referenced this pull request Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔮 dome Ignition Dome performance Runtime performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants