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 top level and static to speed up physics system #656

Merged
merged 2 commits into from
Mar 1, 2021

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Feb 26, 2021

🦟 Bug fix

Summary

Profiling a world as follows:

  • 3000 non-static simple shapes sitting on a static ground plane
  • running with unbounded RTF (-z 1000000000)
  • running headless (with -s and only the Physics system)
  • Using TPE as the physics engine

I noticed that the PhysicsPrivate::UpdateSim was taking way too long for what it does. All that function is doing is getting resulting data from the ign-physics step and populating the ECM, and that takes ~70% of an iteration:

Before:

image

I think there's a lot of room for improvement there, but here are just a couple of little improvements. Caching a couple of values that never change (top-level model and static), instead of reevaluating them at every iteration, we can reduce that to closer to 60%:

After:

topmodel_static_cached

For static, one thing to consider is leveraging the ECM and adding Static components to links that are static, so that the Each call can iterate only over static links.

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

@chapulina chapulina added the performance Runtime performance label Feb 26, 2021
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Feb 26, 2021
@codecov
Copy link

codecov bot commented Feb 26, 2021

Codecov Report

Merging #656 (407614a) into ign-gazebo4 (302f5ed) will decrease coverage by 12.61%.
The diff coverage is 59.17%.

Impacted file tree graph

@@               Coverage Diff                @@
##           ign-gazebo4     #656       +/-   ##
================================================
- Coverage        77.37%   64.76%   -12.62%     
================================================
  Files              217      232       +15     
  Lines            12217    16601     +4384     
================================================
+ Hits              9453    10751     +1298     
- Misses            2764     5850     +3086     
Impacted Files Coverage Δ
include/ignition/gazebo/EntityComponentManager.hh 100.00% <ø> (ø)
include/ignition/gazebo/rendering/SceneManager.hh 100.00% <ø> (ø)
.../plugins/component_inspector/ComponentInspector.cc 6.11% <0.00%> (-2.50%) ⬇️
.../plugins/component_inspector/ComponentInspector.hh 28.57% <ø> (ø)
src/gui/plugins/scene3d/Scene3D.cc 8.72% <0.00%> (ø)
src/systems/diff_drive/DiffDrive.hh 100.00% <ø> (ø)
src/systems/sensors/Sensors.cc 74.75% <0.00%> (ø)
src/rendering/SceneManager.cc 22.92% <8.79%> (ø)
src/systems/joint_controller/JointController.cc 77.17% <12.50%> (-6.16%) ⬇️
...int_position_controller/JointPositionController.cc 71.29% <12.50%> (-4.71%) ⬇️
... and 38 more

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 8e29cda...407614a. Read the comment docs.

@nkoenig
Copy link
Contributor

nkoenig commented Mar 1, 2021

This worked great on SubT. Performance went from 65-70% to around 95% when running a single robot in a world.

Copy link
Contributor

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

Changes seem okay to me. It seems like there wasn't much of an improvement if I ran this with the 3000 shapes world with static models instead of non-static, which was a little surprising to me. But I think that this is a step in the right direction, and we will probably need to continue making changes like this.

Also, I didn't see any issues in the code, but I'm not too familiar with the physics system, so if someone else wants to take a look at the changes as a sanity check, that might be a good idea (I think @nkoenig may have already done this).

@chapulina chapulina merged commit 2a73431 into ign-gazebo4 Mar 1, 2021
@chapulina chapulina deleted the chapulina/4/physics_perf branch March 1, 2021 21:28
adlarkin added a commit that referenced this pull request Aug 25, 2021
Signed-off-by: Ashton Larkin <[email protected]>
adlarkin added a commit that referenced this pull request Aug 25, 2021
Signed-off-by: Ashton Larkin <[email protected]>
@adlarkin adlarkin mentioned this pull request Aug 25, 2021
chapulina pushed a commit that referenced this pull request Oct 11, 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.

3 participants