Skip to content

Improve Model Sdf deserialization performance #3082

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.

Sign up for GitHub

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

Draft
wants to merge 9 commits into
base: gz-sim10
Choose a base branch
from
Draft

Improve Model Sdf deserialization performance #3082

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
37ff8a0
testing deserialization
iche033 Sep 9, 2025
5886229
Cleanup
iche033 Sep 9, 2025
416c15d
update comment
iche033 Sep 9, 2025
051bf6b
fix build
iche033 Sep 9, 2025
b2fa33c
Merge branch 'gz-sim10' into iche033/deserialize_model_sdf
iche033 Sep 23, 2025
910732c
Merge branch 'gz-sim10' into iche033/deserialize_model_sdf
iche033 Sep 29, 2025
b6b0a22
use mutex to protect deserialization
iche033 Oct 7, 2025
729f2cc
make mutex static
iche033 Oct 7, 2025
a152e74
lint
iche033 Oct 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions include/gz/sim/components/Model.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,13 @@
#ifndef GZ_SIM_COMPONENTS_MODEL_HH_
#define GZ_SIM_COMPONENTS_MODEL_HH_

#include <memory>
#include <mutex>
#include <string>

#include <sdf/Model.hh>
#include <sdf/Root.hh>
#include <sdf/parser.hh>

#include <gz/sim/components/Factory.hh>
#include <gz/sim/components/Component.hh>
Expand Down Expand Up @@ -103,12 +106,34 @@ namespace serializers
return _in;
}

// Its super expensive to create an SDFElement for some reason
sdf::Root root;
sdf::Errors errors = root.LoadSdfString(sdf);
{
// It's super expensive to create an sdf::SDFPtr object.
// Workaround this by making it a static object so we only initialize it
// once.
// https://github.com/gazebosim/sdformat/issues/1478
sdf::Errors errors;
static std::mutex mutex;
static sdf::SDFPtr sdfParsed;
Copy link
Contributor

@arjo129 arjo129 Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@iche033 iche033 Oct 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

std::lock_guard<std::mutex> lock(mutex);
if (!sdfParsed)
{
sdfParsed = std::make_shared<sdf::SDF>();
sdf::init(sdfParsed);
}
auto config = sdf::ParserConfig::GlobalConfig();
// Read an SDF string, and store the result in sdfParsed.
if (!sdf::readString(sdf, config, sdfParsed, errors))
{
gzwarn << "Unable to read SDF while deserializing sdf::Model "
<< sdf << std::endl;
return _in;
}
root.Load(sdfParsed, config);
}
if (!root.Model())
{
gzwarn << "Unable to deserialize sdf::Model " << sdf<< std::endl;
gzwarn << "Unable to deserialize sdf::Model " << sdf << std::endl;
return _in;
}

Expand Down
Loading