gazebosim / gz-sim

Open source robotics simulator. The latest version of Gazebo.
https://gazebosim.org
Apache License 2.0
719 stars 272 forks source link

Improve load times by skipping serialization of entities when unecessary. #2596

Closed arjo129 closed 4 days ago

arjo129 commented 2 months ago

🦟 Bug fix

Fixes #

Summary

I was investigating https://github.com/gazebosim/gazebo_test_cases/issues/1576 , in my investigation it came to my notice that sdf::Root takes a long time to be constructed. While I am submitting changes upstream to reduce the impact of the creation of sdf::Root, in the event that we don't serialize a model, I'm proposing we just send an empty string accross. This should minimize both network traffic and make load times more manageable.

Checklist

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

πŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”ΈπŸ”Έ

azeey commented 2 months ago

I remember we added this serializing of sdf::Model to support scaling on the GUI, but that was never implemented. Maybe @caguero remembers more. I'm not sure if we need it for anything else.

caguero commented 2 months ago

I remember we added this serializing of sdf::Model to support scaling on the GUI, but that was never implemented. Maybe @caguero remembers more. I'm not sure if we need it for anything else.

All that I remembered was that it started motivated to support scaling. I'm not sure if after that any other piece is using it.

arjo129 commented 2 months ago

cc: @caguero

Should we remove the serialization all together then? The speed up from removing it is quite a bit. 3k_worlds.sdf take well over 300s (5minutes) to load with serialization, if I remove it we can load in 22seconds an almost 13x improvement.

Each model instance takes about 0.1s to deserialize (which is not very good).

I do think we should eventually address the root cause which I've documented here: https://github.com/gazebosim/sdformat/issues/1478

azeey commented 2 months ago

@arjo129 having long load times is not great, but since this has been an issue for a long time and only affects loading large files, I would not consider it a critical bug. I'd rather not make this change while we are in code freeze.

arjo129 commented 2 months ago

Sure lets revisit this after the release. That being said, I would say that 3k worlds only loads on the largest computer I have access to (which happens to be a cloud instance). It fails to load on any of my local machines.

This fix also makes loading openrmf demo worlds a lot faster.

iche033 commented 1 month ago

the Examples test failure is unrelated, and should be fixed by https://github.com/gazebosim/gz-sim/pull/2649

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 68.78%. Comparing base (712643f) to head (ccb8546). Report is 16 commits behind head on main.

Files with missing lines Patch % Lines
include/gz/sim/components/Model.hh 87.50% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #2596 +/- ## ========================================== - Coverage 68.80% 68.78% -0.02% ========================================== Files 341 341 Lines 33037 33052 +15 ========================================== + Hits 22730 22736 +6 - Misses 10307 10316 +9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

scpeters commented 1 month ago

this branch is based on main (see changes to README) but targeted at gz-sim9. It should either be retargeted to main or rebased on gz-sim9

arjo129 commented 1 month ago

this branch is based on main (see changes to README) but targeted at gz-sim9. It should either be retargeted to main or rebased on gz-sim9

:facepalm: Thanks for spotting that!

arjo129 commented 4 days ago

Not sure why CI is still :x: on this. Seems unrelated.

scpeters commented 4 days ago

@osrf-jenkins run tests please

scpeters commented 4 days ago

Not sure why CI is still ❌ on this. Seems unrelated.

merging forward https://github.com/gazebosim/gz-sim/pull/2673 in https://github.com/gazebosim/gz-sim/pull/2680 which should fix some CI

arjo129 commented 3 days ago

https://github.com/Mergifyio backport gz-sim9 gz-sim8 ign-gazebo6

mergify[bot] commented 3 days ago

backport gz-sim9 gz-sim8 ign-gazebo6

βœ… Backports have been created

* [#2682 Improve load times by skipping serialization of entities when unecessary. (backport #2596)](https://github.com/gazebosim/gz-sim/pull/2682) has been created for branch `gz-sim9` * [#2683 Improve load times by skipping serialization of entities when unecessary. (backport #2596)](https://github.com/gazebosim/gz-sim/pull/2683) has been created for branch `gz-sim8` * [#2684 Improve load times by skipping serialization of entities when unecessary. (backport #2596)](https://github.com/gazebosim/gz-sim/pull/2684) has been created for branch `ign-gazebo6` but encountered conflicts