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

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

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Sep 5, 2024

🦟 Bug fix

Fixes #

Summary

I was investigating
gazebosim/gazebo_test_cases#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

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • 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 and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

I was investigating
gazebosim/gazebo_test_cases#1576 , in my
investigation it came to my notice that `sdf::Element` takes forever to
destroy (We should open a ticket somewhere about this). If we are
skipping serialization we might as well not create and destroy an
SDF Element. This hack greatly speeds up the load time for gazebo.

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
@azeey
Copy link
Contributor

azeey commented Sep 5, 2024

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
Copy link
Contributor

caguero commented Sep 5, 2024

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
Copy link
Contributor Author

arjo129 commented Sep 5, 2024

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: gazebosim/sdformat#1478

@arjo129 arjo129 linked an issue Sep 6, 2024 that may be closed by this pull request
@azeey
Copy link
Contributor

azeey commented Sep 6, 2024

@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
Copy link
Contributor Author

arjo129 commented Sep 6, 2024

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.

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
@arjo129 arjo129 marked this pull request as ready for review October 1, 2024 04:26
@arjo129 arjo129 changed the title A hack to greatly improve load times [Do not merge] Improve load times by skipping serialization of entities when unnecessary. Oct 1, 2024
@arjo129 arjo129 changed the title Improve load times by skipping serialization of entities when unnecessary. Improve load times by skipping serialization of entities when unecessary. Oct 1, 2024
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
@iche033
Copy link
Contributor

iche033 commented Oct 11, 2024

the Examples test failure is unrelated, and should be fixed by #2649

@arjo129 arjo129 enabled auto-merge (squash) October 13, 2024 11:22
Copy link

codecov bot commented Oct 13, 2024

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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@scpeters
Copy link
Member

scpeters commented Oct 14, 2024

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 arjo129 changed the base branch from gz-sim9 to main October 14, 2024 09:26
@arjo129
Copy link
Contributor Author

arjo129 commented Oct 14, 2024

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

🤦 Thanks for spotting that!

@arjo129 arjo129 added 🪵 jetty Gazebo Jetty and removed 🏛️ ionic Gazebo Ionic labels Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪵 jetty Gazebo Jetty
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

gz-sim: 3k_shapes.sdf
5 participants