Improve Model Sdf deserialization performance

[iche033]

🦟 Bug fix

Summary

Alternative to #3070. Instead of completely skipping the model serialization / materialization step, this PR uses a workaround to speed up the deserialization process. This is done by implementing a modified version of sdf::Root::LoadSdfString. It uses a static sdf::SDFPtr object to avoid multiple expensive sdf::init calls. Note that this workaround is similar to the one done in sdf/src/pasrer.cc.

We could also implement this workaround directly in sdformat but this I chose to implement it here to reduce the impact of this change during the code freeze period.

For the Jetty demo world , the load times are:

• Before any changes: 55s
• Skip model sdf serialization (Skip Model SDF serialization #3070): 42s
• this pull request: 48s

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
• Was GenAI used to generate this PR? If so, make sure to add "Generated-by" to your commits. (See this policy for more info.)

Generated-by: Remove this if GenAI was not used.

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

[iche033]

Testing this with the Jetty demo world and gazebosim/sdformat#1589, I see that this PR still improves performance.

The initial load time dropped from 33 seconds to 22 seconds on my machine, and resuming to play state from paused state reduced from 17 to 3 seconds.

[iche033]

changed from draft to ready status because I think this PR still helps with performance, especially when changing from paused to play state in the jetty demo world.

[arjo129]

Thanks Ian! Just to clarify Im assuming we are sure that serialization and deserialization only ever happen in a single threaded context

[iche033]

Thanks Ian! Just to clarify Im assuming we are sure that serialization and deserialization only ever happen in a single threaded context

Not sure about serialization but this changes only the deserialization part. In the current gz-sim use case, I believe deserialization only happens in a single thread. Deserialization happens when the gui receives a state from the server, i.e. in GuiRunner::OnStateQt

gz-sim/src/gui/GuiRunner.cc

Line 299 in fefd16b

this->dataPtr->ecm.SetState(_msg.state());

which then calls Deserialize

gz-sim/src/EntityComponentManager.cc

Line 1981 in fefd16b

newComp->Deserialize(istr);

The state callback function does not happen in multiple threads.

[arjo129]

Our tests do rely on multi-threading and this patch would introduce a flake in our tests.

[arjo129]

Suggested change

	static sdf::SDFPtr sdfParsed;
	static sdf::SDFPtr sdfParsed;

So there are definitely places where this seems to introduce regressions:

https://build.osrfoundation.org/job/gz_sim-ci-pr_any-homebrew-amd64/2111/consoleFull#-140582033379332c8d-713a-4d09-92ce-eb424edb41b6

These tests will need to be fixed before we merge.

[azeey]

This would also have to be static as well since, otherwise, each instance of the component will have it's own mutex.

Suggested change

	std::mutex mutex;
	static std::mutex mutex;

[arjo129]

Simply adding static to a mutex may or maynot work. I can see failures on mac OS where the mutex is throwing an exception in our tests. Apparently according to this issue on another project, this may have to do with destruction. I dont think itll impact UX but it'll make our tests more flaky.

In hind-sight perhaps #3070 may be a better option for future releases of gazebo ( GZ K and up)

[iche033]

I saw the test failure and haven't been able to resolve it yet. I wasn't able to reproduce the crash locally on my mac. I'm also testing different workarounds and rerunning homebrew builds and will see if I have any luck.

Yeah, we can retarget #3070 to main as it causes a behavior change.