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

arjo129 · 2024-09-05T08:48:31Z

🦟 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 <[email protected]>

azeey · 2024-09-05T15:30:58Z

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 · 2024-09-05T15:45:15Z

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 · 2024-09-05T23:41:36Z

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

azeey · 2024-09-06T18:11:14Z

@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 · 2024-09-06T23:17:21Z

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 <[email protected]>

include/gz/sim/components/Model.hh

Signed-off-by: Arjo Chakravarty <[email protected]>

…//github.com/gazebosim/gz-sim into arjo/fix/a_hack_to_greatly_improve_loadtimes

iche033 · 2024-10-11T20:59:29Z

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

…eatly_improve_loadtimes

codecov · 2024-10-13T12:28:40Z

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 · 2024-10-14T04:03:46Z

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 · 2024-10-14T09:27:36Z

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!

Signed-off-by: Arjo Chakravarty <[email protected]>

arjo129 · 2024-11-18T09:28:25Z

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

scpeters · 2024-11-18T13:16:59Z

@osrf-jenkins run tests please

scpeters · 2024-11-18T14:29:47Z

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

merging forward #2673 in #2680 which should fix some CI

arjo129 · 2024-11-19T03:16:25Z

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

mergify · 2024-11-19T03:16:30Z

backport gz-sim9 gz-sim8 ign-gazebo6

✅ Backports have been created

#2682 Improve load times by skipping serialization of entities when unecessary. (backport #2596) has been created for branch gz-sim9
#2683 Improve load times by skipping serialization of entities when unecessary. (backport #2596) has been created for branch gz-sim8
#2684 Improve load times by skipping serialization of entities when unecessary. (backport #2596) has been created for branch ign-gazebo6 but encountered conflicts

…ary. (#2596) * A hack to greatly improve load times 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 <[email protected]> * Remove magic word Signed-off-by: Arjo Chakravarty <[email protected]> * Send empty string instead of using sentinel value. Signed-off-by: Arjo Chakravarty <[email protected]> * Style Signed-off-by: Arjo Chakravarty <[email protected]> * fix custom sensor system example build (#2649) Signed-off-by: Ian Chen <[email protected]> * remove stray change Signed-off-by: Arjo Chakravarty <[email protected]> --------- Signed-off-by: Arjo Chakravarty <[email protected]> Signed-off-by: Ian Chen <[email protected]> Co-authored-by: Ian Chen <[email protected]> (cherry picked from commit 1a88131)

…ary. (#2596) * A hack to greatly improve load times 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 <[email protected]> * Remove magic word Signed-off-by: Arjo Chakravarty <[email protected]> * Send empty string instead of using sentinel value. Signed-off-by: Arjo Chakravarty <[email protected]> * Style Signed-off-by: Arjo Chakravarty <[email protected]> * fix custom sensor system example build (#2649) Signed-off-by: Ian Chen <[email protected]> * remove stray change Signed-off-by: Arjo Chakravarty <[email protected]> --------- Signed-off-by: Arjo Chakravarty <[email protected]> Signed-off-by: Ian Chen <[email protected]> Co-authored-by: Ian Chen <[email protected]> (cherry picked from commit 1a88131) # Conflicts: # include/gz/sim/components/Model.hh

…ary. (#2596) (#2682) * A hack to greatly improve load times 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 <[email protected]> Signed-off-by: Ian Chen <[email protected]> Co-authored-by: Ian Chen <[email protected]> (cherry picked from commit 1a88131)

…ary. (#2596) (#2683) * A hack to greatly improve load times 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 <[email protected]> Signed-off-by: Ian Chen <[email protected]> Co-authored-by: Ian Chen <[email protected]> (cherry picked from commit 1a88131)

…ary. (backport #2596) (#2684) * Improve load times by skipping serialization of entities when unecessary. (#2596) * A hack to greatly improve load times 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 <[email protected]> Co-authored-by: Arjo Chakravarty <[email protected]> Co-authored-by: Arjo Chakravarty <[email protected]>

github-actions bot added the 🏛️ ionic Gazebo Ionic label Sep 5, 2024

This was referenced Sep 5, 2024

Poor performance of SDF Element Pointer Construction gazebosim/sdformat#1478

Open

gz-sim: 3k_shapes.sdf gazebosim/gazebo_test_cases#1576

Closed

arjo129 linked an issue Sep 6, 2024 that may be closed by this pull request

gz-sim: 3k_shapes.sdf gazebosim/gazebo_test_cases#1576

Closed

arjo129 added 2 commits September 12, 2024 13:34

Remove magic word

da543ab

Signed-off-by: Arjo Chakravarty <[email protected]>

Send empty string instead of using sentinel value.

beadb78

Signed-off-by: Arjo Chakravarty <[email protected]>

arjo129 marked this pull request as ready for review October 1, 2024 04:26

arjo129 requested a review from mjcarroll as a code owner October 1, 2024 04:26

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

Merge branch 'gz-sim9' into arjo/fix/a_hack_to_greatly_improve_loadtimes

4a833ac

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

iche033 reviewed Oct 10, 2024

View reviewed changes

include/gz/sim/components/Model.hh Outdated Show resolved Hide resolved

arjo129 added 2 commits October 11, 2024 08:33

Style

b4865d5

Signed-off-by: Arjo Chakravarty <[email protected]>

Merge branch 'arjo/fix/a_hack_to_greatly_improve_loadtimes' of https:…

018e86d

…//github.com/gazebosim/gz-sim into arjo/fix/a_hack_to_greatly_improve_loadtimes

iche033 approved these changes Oct 11, 2024

View reviewed changes

arjo129 added 2 commits October 13, 2024 12:04

Merge remote-tracking branch 'origin/main' into arjo/fix/a_hack_to_gr…

e0d28aa

…eatly_improve_loadtimes

Merge branch 'gz-sim9' into arjo/fix/a_hack_to_greatly_improve_loadtimes

ccb8546

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

scpeters disabled auto-merge October 14, 2024 04:02

arjo129 changed the base branch from gz-sim9 to main October 14, 2024 09:26

arjo129 added 2 commits October 29, 2024 09:58

Merge branch 'main' into arjo/fix/a_hack_to_greatly_improve_loadtimes

99b80a8

remove stray change

a290bbf

Signed-off-by: Arjo Chakravarty <[email protected]>

arjo129 added 🪵 jetty Gazebo Jetty and removed 🏛️ ionic Gazebo Ionic labels Nov 1, 2024

arjo129 enabled auto-merge (squash) November 11, 2024 02:21

Merge branch 'main' into arjo/fix/a_hack_to_greatly_improve_loadtimes

5ddd9af

arjo129 merged commit 1a88131 into main Nov 18, 2024
9 checks passed

arjo129 deleted the arjo/fix/a_hack_to_greatly_improve_loadtimes branch November 18, 2024 20:46

mergify bot mentioned this pull request Nov 19, 2024

Improve load times by skipping serialization of entities when unecessary. (backport #2596) #2682

Merged

8 tasks

This was referenced Nov 19, 2024

Improve load times by skipping serialization of entities when unecessary. (backport #2596) #2683

Merged

Improve load times by skipping serialization of entities when unecessary. (backport #2596) #2684

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

arjo129 commented Sep 5, 2024 •

edited

Loading

azeey commented Sep 5, 2024

caguero commented Sep 5, 2024

arjo129 commented Sep 5, 2024 •

edited

Loading

azeey commented Sep 6, 2024

arjo129 commented Sep 6, 2024 •

edited

Loading

iche033 commented Oct 11, 2024

codecov bot commented Oct 13, 2024 •

edited

Loading

scpeters commented Oct 14, 2024 •

edited

Loading

arjo129 commented Oct 14, 2024

arjo129 commented Nov 18, 2024

scpeters commented Nov 18, 2024

scpeters commented Nov 18, 2024

arjo129 commented Nov 19, 2024

mergify bot commented Nov 19, 2024 •

edited

Loading

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

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

Conversation

arjo129 commented Sep 5, 2024 • edited Loading

🦟 Bug fix

Summary

Checklist

azeey commented Sep 5, 2024

caguero commented Sep 5, 2024

arjo129 commented Sep 5, 2024 • edited Loading

azeey commented Sep 6, 2024

arjo129 commented Sep 6, 2024 • edited Loading

iche033 commented Oct 11, 2024

codecov bot commented Oct 13, 2024 • edited Loading

Codecov Report

scpeters commented Oct 14, 2024 • edited Loading

arjo129 commented Oct 14, 2024

arjo129 commented Nov 18, 2024

scpeters commented Nov 18, 2024

scpeters commented Nov 18, 2024

arjo129 commented Nov 19, 2024

mergify bot commented Nov 19, 2024 • edited Loading

✅ Backports have been created

arjo129 commented Sep 5, 2024 •

edited

Loading

arjo129 commented Sep 5, 2024 •

edited

Loading

arjo129 commented Sep 6, 2024 •

edited

Loading

codecov bot commented Oct 13, 2024 •

edited

Loading

scpeters commented Oct 14, 2024 •

edited

Loading

mergify bot commented Nov 19, 2024 •

edited

Loading