From d0131c8cbe114f56ae9d27ffd36eac04f97f0715 Mon Sep 17 00:00:00 2001 From: "Addisu Z. Taddese" Date: Thu, 11 Sep 2025 12:27:11 -0500 Subject: [PATCH] Optimize load time and memory consumption by reusing element descriptions (#1589) In most usages of `GetElementDescription`, the returned `sdf::ElementPtr` is not mutated, thus it is possible to implement a copy-on-write mechanism to avoid unnecessary clones of element descriptions. This PR removes clones of element description during the loading of an SDFormat file and introduces APIs to get immutable element descriptions. It also provides mutable element descriptions which clone the element description when called. * Do not clone element descriptions * Add MutableElementDescription and deprecate GetElementDescription * Add tests * Narrower use of GZ_UTILS_WARN_* macros * Reword comments, Migration guide * Remove duplicate line in python test * Add / adjust //// lines Signed-off-by: Steve Peters --------- Signed-off-by: Addisu Z. Taddese Signed-off-by: Steve Peters Co-authored-by: Steve Peters (cherry picked from commit db6bdbb076332b4c5ba8c3fffc95d54372956d26) --- Migration.md | 16 +++++++ include/sdf/Element.hh | 48 +++++++++++++++++-- python/src/sdf/pyElement.cc | 47 ++++++++++++++++-- python/test/pyElement_TEST.py | 34 +++++++++++++ src/Element.cc | 90 +++++++++++++++++++++++++++++++---- src/Element_TEST.cc | 47 ++++++++++++++++-- src/ParamPassing.cc | 2 +- src/parser.cc | 8 ++-- 8 files changed, 268 insertions(+), 24 deletions(-) diff --git a/Migration.md b/Migration.md index f16b810aa..dd4283fa8 100644 --- a/Migration.md +++ b/Migration.md @@ -22,6 +22,22 @@ but with improved human-readability.. package.xml package name. Use `find_package(sdformat)` instead of `find_package(sdformatX)` going forward. +### Additions + +- **sdf/Element.hh**: + + `sdf::ElementConstPtr ElementDescription(unsigned int) const` + + `sdf::ElementConstPtr ElementDescription(const std::string &) const` + + `sdf::ElementPtr MutableElementDescription(unsigned int)` + + `sdf::ElementPtr MutableElementDescription(const std::string &)` + +### Deprecations +- **sdf/Element.hh**: + + Mutable access to an element description via `Element::GetElementDescription` has been deprecated. Please use `Element::MutableElementDescription` instead. For immutable access, please use `Element::ElementDescription`. + + ***Deprecation:*** `sdf::ElementPtr GetElementDescription(unsigned int)` + + ***Replacement:*** `sdf::ElementConstPtr ElementDescription(unsigned int) const` + + ***Deprecation:*** `sdf::ElementPtr GetElementDescription(const std::string &)` + + ***Replacement:*** `sdf::ElementConstPtr ElementDescription(const std::string &) const` + ### Removals - **sdf/config.hh**: diff --git a/include/sdf/Element.hh b/include/sdf/Element.hh index cfd4adb3e..7867b77dd 100644 --- a/include/sdf/Element.hh +++ b/include/sdf/Element.hh @@ -20,8 +20,10 @@ #include #include #include +#include #include #include +#include #include #include @@ -378,12 +380,40 @@ namespace sdf /// \brief Get an element description using an index /// \param[in] _index the index of the element description to get. /// \return An Element pointer to the found element. - public: ElementPtr GetElementDescription(unsigned int _index) const; + /// \deprecated See ElementDescription + public: + GZ_DEPRECATED(16) + ElementPtr GetElementDescription(unsigned int _index) const; /// \brief Get an element description using a key /// \param[in] _key the key to use to find the element. /// \return An Element pointer to the found element. - public: ElementPtr GetElementDescription(const std::string &_key) const; + /// \deprecated See ElementDescription + public: + GZ_DEPRECATED(16) + ElementPtr GetElementDescription(const std::string &_key) const; + + /// \brief Get an element description using an index + /// \param[in] _index the index of the element description to get. + /// \return An Element pointer to the found element. + /// \sa MutableElementDescription + public: ElementConstPtr ElementDescription(unsigned int _index) const; + + /// \brief Get an element description using a key + /// \param[in] _key the key to use to find the element. + /// \return An Element pointer to the found element. + /// \sa MutableElementDescription + public: ElementConstPtr ElementDescription(const std::string &_key) const; + + /// \brief Get a mutable element description using an index + /// \param[in] _index the index of the element description to get. + /// \return An Element pointer to the found element. + public: ElementPtr MutableElementDescription(unsigned int _index); + + /// \brief Get a mutable element description using a key + /// \param[in] _key the key to use to find the element. + /// \return An Element pointer to the found element. + public: ElementPtr MutableElementDescription(const std::string &_key); /// \brief Return true if an element description exists. /// \param[in] _name the name of the element to find. @@ -935,6 +965,10 @@ namespace sdf /// \brief XML path of this element. public: std::string xmlPath; + /// \brief Used to keep track of which element descriptions we have cloned + /// in order to provide a mutable pointer. + public: std::unordered_set clonedElementDescriptions; + /// \brief Generate the string (XML) for the attributes. /// \param[in] _includeDefaultAttributes flag to include default attributes. /// \param[in] _config Configuration for printing attributes. @@ -952,6 +986,14 @@ namespace sdf bool _includeDefaultAttributes, const PrintConfig &_config, std::ostringstream &_out) const; + + /// \brief Helper function to get the index of an element description + /// identified by a given key + /// \param[in] _key Key of the element description + /// \return An optional index. nullopt of the key was not found + public: + std::optional ElementDescriptionIndex( + const std::string &_key); }; /////////////////////////////////////////////// @@ -1038,7 +1080,7 @@ namespace sdf } else if (this->HasElementDescription(_key)) { - result.first = this->GetElementDescription(_key)->Get(_errors); + result.first = this->ElementDescription(_key)->Get(_errors); } else { diff --git a/python/src/sdf/pyElement.cc b/python/src/sdf/pyElement.cc index 935b95187..66c487e7b 100644 --- a/python/src/sdf/pyElement.cc +++ b/python/src/sdf/pyElement.cc @@ -21,6 +21,7 @@ #include #include +#include #include "pyParam.hh" #include "pybind11_helpers.hh" @@ -59,6 +60,10 @@ void defineElement(py::object module) { using PyClassElement = py::class_; + // TODO(azeey): GetElementDescription has been deprecated. Remove in sdformat17 + auto warnings = py::module::import("warnings"); + auto builtins = py::module::import("builtins"); + auto elemClass = PyClassElement(module, "Element"); elemClass.def(py::init<>()) .def("clone", ErrorWrappedCast<>(&Element::Clone, py::const_), @@ -143,13 +148,47 @@ void defineElement(py::object module) .def("get_element_description_count", &Element::GetElementDescriptionCount, "Get the number of element descriptions.") - .def("get_element_description", - py::overload_cast(&Element::GetElementDescription, + .def( + "get_element_description", + [warnings, builtins](const Element &_self, unsigned int _index) + { + warnings.attr("warn")( + "GetElementDescription is deprecated. Use ElementDescription " + "or MutableElementDescription instead", + builtins.attr("DeprecationWarning")); + GZ_UTILS_WARN_IGNORE__DEPRECATED_DECLARATION + return _self.GetElementDescription(_index); + GZ_UTILS_WARN_RESUME__DEPRECATED_DECLARATION + }, + "Get an element description using an index") + .def( + "get_element_description", + [warnings, builtins](const Element &_self, const std::string &_key) + { + warnings.attr("warn")( + "GetElementDescription is deprecated. Use ElementDescription " + "or MutableElementDescription instead", + builtins.attr("DeprecationWarning")); + + GZ_UTILS_WARN_IGNORE__DEPRECATED_DECLARATION + return _self.GetElementDescription(_key); + GZ_UTILS_WARN_RESUME__DEPRECATED_DECLARATION + }, + "Get an element description using a key") + .def("element_description", + py::overload_cast(&Element::ElementDescription, py::const_), "Get an element description using an index") - .def("get_element_description", + .def("element_description", + py::overload_cast(&Element::ElementDescription, + py::const_), + "Get an element description using a key") + .def("mutable_element_description", + py::overload_cast(&Element::MutableElementDescription), + "Get an element description using an index") + .def("mutable_element_description", py::overload_cast( - &Element::GetElementDescription, py::const_), + &Element::MutableElementDescription), "Get an element description using a key") .def("has_element_description", &Element::HasElementDescription, "Return true if an element description exists.") diff --git a/python/test/pyElement_TEST.py b/python/test/pyElement_TEST.py index 2af47d3a1..70420cf8b 100644 --- a/python/test/pyElement_TEST.py +++ b/python/test/pyElement_TEST.py @@ -15,6 +15,7 @@ from sdformat import Element, SDFErrorsException from gz.math import Vector3d import unittest +import warnings class ElementTEST(unittest.TestCase): @@ -475,6 +476,39 @@ def test_find_element(self): self.assertEqual("first_child", child_elem_b.get_attribute("name").get_as_string()) + def test_mutable_element_description(self): + elem = Element() + desc = Element() + desc.set_name("radius") + desc.add_value("double", "1", True, "radius") + elem.add_element_description(desc) + + elem_clone = elem.clone() + self.assertEqual(elem_clone.element_description("radius"), desc) + self.assertEqual(elem_clone.element_description(0), desc) + + with warnings.catch_warnings(): + warnings.simplefilter("ignore") + self.assertEqual(elem_clone.get_element_description("radius"), desc) + self.assertEqual(elem_clone.get_element_description(0), desc) + + newDesc = elem_clone.mutable_element_description("radius") + self.assertNotEqual(newDesc, desc) + ## If we call mutable_element_description multiple times, it shouldn't create clone new Elements + self.assertEqual(newDesc, elem_clone.mutable_element_description("radius")) + self.assertEqual(newDesc, elem_clone.mutable_element_description(0)) + + # After mutable_element_description is called, calling the read-only + # element_description will return a different pointer than the original + # description + self.assertNotEqual(elem_clone.element_description("radius"), desc) + self.assertNotEqual(elem_clone.element_description(0), desc) + + + # Resetting the element clears the element descriptions. + elem_clone.reset() + self.assertIsNone(elem_clone.element_description("radius")) + self.assertIsNone(elem_clone.mutable_element_description("radius")) if __name__ == '__main__': unittest.main() diff --git a/src/Element.cc b/src/Element.cc index 5795cb44e..67c670814 100644 --- a/src/Element.cc +++ b/src/Element.cc @@ -17,6 +17,9 @@ #include #include +#include +#include +#include #include #include @@ -270,7 +273,7 @@ ElementPtr Element::Clone(sdf::Errors &_errors) const for (eiter = this->dataPtr->elementDescriptions.begin(); eiter != this->dataPtr->elementDescriptions.end(); ++eiter) { - clone->dataPtr->elementDescriptions.push_back((*eiter)->Clone(_errors)); + clone->dataPtr->elementDescriptions.push_back(*eiter); } clone->dataPtr->elements.reserve(this->dataPtr->elements.size()); @@ -348,11 +351,12 @@ void Element::Copy(const ElementPtr _elem, sdf::Errors &_errors) } this->dataPtr->elementDescriptions.clear(); + this->dataPtr->clonedElementDescriptions.clear(); for (ElementPtr_V::const_iterator iter = _elem->dataPtr->elementDescriptions.begin(); iter != _elem->dataPtr->elementDescriptions.end(); ++iter) { - this->dataPtr->elementDescriptions.push_back((*iter)->Clone(_errors)); + this->dataPtr->elementDescriptions.push_back(*iter); } this->dataPtr->elements.clear(); @@ -690,6 +694,21 @@ void ElementPrivate::PrintAttributes(sdf::Errors &_errors, } } +///////////////////////////////////////////////// +std::optional ElementPrivate::ElementDescriptionIndex( + const std::string &_key) +{ + for (auto iter = this->elementDescriptions.begin(); + iter != this->elementDescriptions.end(); ++iter) + { + if ((*iter)->GetName() == _key) + { + return std::distance(this->elementDescriptions.begin(), iter); + } + } + return std::nullopt; +} + ///////////////////////////////////////////////// void Element::PrintValues(std::string _prefix, const PrintConfig &_config) const @@ -896,6 +915,18 @@ size_t Element::GetElementDescriptionCount() const ///////////////////////////////////////////////// ElementPtr Element::GetElementDescription(unsigned int _index) const +{ + return std::const_pointer_cast(this->ElementDescription(_index)); +} + +///////////////////////////////////////////////// +ElementPtr Element::GetElementDescription(const std::string &_key) const +{ + return std::const_pointer_cast(this->ElementDescription(_key)); +} + +///////////////////////////////////////////////// +ElementConstPtr Element::ElementDescription(unsigned int _index) const { ElementPtr result; if (_index < this->dataPtr->elementDescriptions.size()) @@ -906,7 +937,7 @@ ElementPtr Element::GetElementDescription(unsigned int _index) const } ///////////////////////////////////////////////// -ElementPtr Element::GetElementDescription(const std::string &_key) const +ElementConstPtr Element::ElementDescription(const std::string &_key) const { ElementPtr_V::const_iterator iter; for (iter = this->dataPtr->elementDescriptions.begin(); @@ -921,6 +952,48 @@ ElementPtr Element::GetElementDescription(const std::string &_key) const return ElementPtr(); } +///////////////////////////////////////////////// +ElementPtr Element::MutableElementDescription(unsigned int _index) +{ + auto desc = static_cast(this)->ElementDescription(_index); + if (!desc) + { + return ElementPtr(); + } + + // To improve the performance of libsdformat, element descriptions are + // shared among elements of the same type. If the user requests a mutable + // element description, we make a clone here so that other elements are not + // affected by the potential modifications the user makes. We keep track of + // the clones we've made so that a clone is made at most once for each + // element description. + if (this->dataPtr->clonedElementDescriptions.count(desc) == 0) + { + auto clonedDesc = desc->Clone(); + this->dataPtr->clonedElementDescriptions.insert(clonedDesc); + this->dataPtr->elementDescriptions[_index] = clonedDesc; + return clonedDesc; + } + else + { + return std::const_pointer_cast(desc); + } +} + +///////////////////////////////////////////////// +ElementPtr Element::MutableElementDescription(const std::string &_key) +{ + auto index = this->dataPtr->ElementDescriptionIndex(_key); + if (index) + { + return this->MutableElementDescription(*index); + } + else + { + return sdf::ElementPtr(); + } +} + ///////////////////////////////////////////////// ParamPtr Element::GetValue() const { @@ -1179,7 +1252,7 @@ void Element::InsertElement(ElementPtr _elem, bool _setParentToSelf) ///////////////////////////////////////////////// bool Element::HasElementDescription(const std::string &_name) const { - return this->GetElementDescription(_name) != ElementPtr(); + return this->ElementDescription(_name) != ElementConstPtr(); } ///////////////////////////////////////////////// @@ -1204,7 +1277,7 @@ ElementPtr Element::AddElement(const std::string &_name, sdf::Errors &_errors) for (unsigned int i = 0; i < parent->GetElementDescriptionCount(); ++i) { this->dataPtr->elementDescriptions.push_back( - parent->GetElementDescription(i)->Clone(_errors)); + parent->dataPtr->elementDescriptions[i]); } } @@ -1313,6 +1386,7 @@ void Element::Reset() } this->dataPtr->elements.clear(); this->dataPtr->elementDescriptions.clear(); + this->dataPtr->clonedElementDescriptions.clear(); this->dataPtr->value.reset(); @@ -1480,10 +1554,10 @@ std::any Element::GetAny(sdf::Errors &_errors, const std::string &_key) const } else { - tmp = this->GetElementDescription(_key); - if (tmp != ElementPtr()) + auto desc = this->ElementDescription(_key); + if (desc != ElementConstPtr()) { - result = tmp->GetAny(_errors); + result = desc->GetAny(_errors); } else { diff --git a/src/Element_TEST.cc b/src/Element_TEST.cc index 2d96e1bbc..394c5d2b3 100644 --- a/src/Element_TEST.cc +++ b/src/Element_TEST.cc @@ -17,6 +17,8 @@ #include +#include + #include "sdf/Element.hh" #include "sdf/Filesystem.hh" #include "sdf/Param.hh" @@ -350,17 +352,17 @@ TEST(Element, AddElementReferenceSDF) ASSERT_EQ(child->GetElementDescriptionCount(), 1UL); - sdf::ElementPtr check = child->GetElementDescription(4); + sdf::ElementConstPtr check = child->ElementDescription(4); ASSERT_EQ(check, sdf::ElementPtr()); - check = child->GetElementDescription(0); + check = child->ElementDescription(0); ASSERT_NE(check, sdf::ElementPtr()); ASSERT_EQ(check->GetName(), "parent"); - check = child->GetElementDescription("bad"); + check = child->ElementDescription("bad"); ASSERT_EQ(check, sdf::ElementPtr()); - check = child->GetElementDescription("parent"); + check = child->ElementDescription("parent"); ASSERT_NE(check, sdf::ElementPtr()); ASSERT_EQ(check->GetName(), "parent"); @@ -1090,3 +1092,40 @@ TEST(Element, FindElement) EXPECT_EQ("first_child", childElemB->GetAttribute("name")->GetAsString()); } } + +TEST(Element, MutableElementDescription) +{ + sdf::ElementPtr elem = std::make_shared(); + sdf::ElementPtr desc = std::make_shared(); + desc->SetName("radius"); + desc->AddValue("double", "1", true, "radius"); + elem->AddElementDescription(desc); + + auto elemClone = elem->Clone(); + + EXPECT_EQ(elemClone->ElementDescription("radius"), desc); + EXPECT_EQ(elemClone->ElementDescription(0), desc); + + GZ_UTILS_WARN_IGNORE__DEPRECATED_DECLARATION + EXPECT_EQ(elemClone->GetElementDescription("radius"), desc); + EXPECT_EQ(elemClone->GetElementDescription(0), desc); + GZ_UTILS_WARN_RESUME__DEPRECATED_DECLARATION + + auto newDesc = elemClone->MutableElementDescription("radius"); + EXPECT_NE(newDesc, desc); + // If we call MutableElementDescription multiple times, it shouldn't create + // clone new Elements + EXPECT_EQ(newDesc, elemClone->MutableElementDescription("radius")); + EXPECT_EQ(newDesc, elemClone->MutableElementDescription(0)); + + // After MutableElementDescription is called, calling the read-only + // ElementDescription will return a different pointer than the original + // description + EXPECT_NE(elemClone->ElementDescription("radius"), desc); + EXPECT_NE(elemClone->ElementDescription(0), desc); + + // Resetting the element clears the element descriptions. + elemClone->Reset(); + EXPECT_EQ(nullptr, elemClone->ElementDescription("radius")); + EXPECT_EQ(nullptr, elemClone->MutableElementDescription("radius")); +} diff --git a/src/ParamPassing.cc b/src/ParamPassing.cc index f30eeb7ea..2d836d729 100644 --- a/src/ParamPassing.cc +++ b/src/ParamPassing.cc @@ -461,7 +461,7 @@ void handleIndividualChildActions(const ParserConfig &_config, continue; } - ElementPtr elemChild = elemDesc->GetElementDescription(elemName); + ElementPtr elemChild = elemDesc->MutableElementDescription(elemName); if (!xmlToSdf(_config, _source, xmlChild, elemChild, _errors)) { diff --git a/src/parser.cc b/src/parser.cc index 992893987..d33c0de11 100644 --- a/src/parser.cc +++ b/src/parser.cc @@ -1615,7 +1615,7 @@ static void validateIncludeElement(tinyxml2::XMLElement *_xml, for (unsigned int descCounter = 0; descCounter != _sdf->GetElementDescriptionCount(); ++descCounter) { - ElementPtr elemDesc = _sdf->GetElementDescription(descCounter); + ElementConstPtr elemDesc = _sdf->ElementDescription(descCounter); if (elemDesc->GetName() == _xml->Value()) { ElementPtr element = elemDesc->Clone(); @@ -1957,7 +1957,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf, } auto includeSDFFirstElem = includeSDF->Root()->GetFirstElement(); - auto includeDesc = _sdf->GetElementDescription("include"); + auto includeDesc = _sdf->ElementDescription("include"); if (includeDesc) { // Store the contents of the tag as the includeElement of @@ -1981,7 +1981,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf, for (descCounter = 0; descCounter != _sdf->GetElementDescriptionCount(); ++descCounter) { - ElementPtr elemDesc = _sdf->GetElementDescription(descCounter); + ElementConstPtr elemDesc = _sdf->ElementDescription(descCounter); if (elemDesc->GetName() == elemXml->Value()) { std::string elemXmlPath = _sdf->XmlPath() + "/" + elemXml->Value(); @@ -2047,7 +2047,7 @@ bool readXml(tinyxml2::XMLElement *_xml, ElementPtr _sdf, for (unsigned int descCounter = 0; descCounter != _sdf->GetElementDescriptionCount(); ++descCounter) { - ElementPtr elemDesc = _sdf->GetElementDescription(descCounter); + ElementConstPtr elemDesc = _sdf->ElementDescription(descCounter); if (elemDesc->GetRequired() == "1" || elemDesc->GetRequired() == "+") {